Skip to content

skip to next user on OutOfBoundsException#28805

Closed
butonic wants to merge 1 commit into
masterfrom
skip-user-on-outofboundsexception-during-sync
Closed

skip to next user on OutOfBoundsException#28805
butonic wants to merge 1 commit into
masterfrom
skip-user-on-outofboundsexception-during-sync

Conversation

@butonic

@butonic butonic commented Aug 25, 2017

Copy link
Copy Markdown
Contributor

on large ldap instances a missing uuid or username currently aborts the whole sync. This PR allows backends to indicate these problems and fail with an OutOfBoundsException. For now core will log the exception and continue with the next user.

In the future we should expose this to the cli.

together with owncloud/user_ldap#125 this skips problematic ldap users

$this->mapper->insert($a);
// clean the user's preferences
$this->cleanPreferences($uid);
} catch (\OutOfBoundsException $e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OutOfBounds is a bad naming choice if the exception is expected to be thrown when we can access to the property. I'd rather use a custom public exception here.

['app' => self::class]
);
continue;
try {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could move this try / catch block to another function. This way we can avoid this nested try / catch, and also make this function more compact.

@PVince81

Copy link
Copy Markdown
Contributor

@butonic any update ? See comments above.

Else would defer to "planned" for the next release.

@PVince81

PVince81 commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

moving to "planned" as discussed with @butonic

@PVince81 PVince81 modified the milestones: planned, development Sep 5, 2017
$this->logger->logException($e, ['app' => self::class]);
}
// clean the user's preferences
$this->cleanPreferences($uid);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed?

@DeepDiver1975

Copy link
Copy Markdown
Member

@ownclouders rebase

@ownclouders

Copy link
Copy Markdown
Contributor

Hey! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost!

@ownclouders

Copy link
Copy Markdown
Contributor

Automated rebase was successful!

@ownclouders ownclouders force-pushed the skip-user-on-outofboundsexception-during-sync branch from 374a03d to bc3cfbb Compare November 24, 2017 08:44
@codecov

codecov Bot commented Nov 24, 2017

Copy link
Copy Markdown

Codecov Report

Merging #28805 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #28805      +/-   ##
============================================
- Coverage     62.15%   62.15%   -0.01%     
- Complexity    17516    17517       +1     
============================================
  Files          1045     1045              
  Lines         57740    57742       +2     
============================================
  Hits          35890    35890              
- Misses        21850    21852       +2
Impacted Files Coverage Δ Complexity Δ
lib/private/User/SyncService.php 32.18% <0%> (-0.76%) 31 <0> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d750519...bc3cfbb. Read the comment docs.

@PVince81 PVince81 modified the milestones: development, planned Jan 12, 2018
@butonic

butonic commented Apr 30, 2018

Copy link
Copy Markdown
Contributor Author

closing, as the current code already continues on all Exceptions. logging errors to the console tracked in #31310

@butonic butonic closed this Apr 30, 2018
@butonic butonic deleted the skip-user-on-outofboundsexception-during-sync branch April 30, 2018 14:59
@phil-davis

Copy link
Copy Markdown
Contributor

backport-request label removed, since that is not relevant to a closed PR.

@PVince81 PVince81 modified the milestones: development, QA Jun 13, 2018
@lock

lock Bot commented Jul 31, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants