Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a default user record if json is broken #17494

Merged
merged 2 commits into from
Dec 13, 2019

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Oct 9, 2019

Fix #16340

It's possible that json_decode returns null. Mostly the json is broken.
AddMissingDefaultValues expects an array. Pass null will fail.

How to reproduce: Modify the json in oc_accounts.

Not sure if we need some repair job to kill those broken records. Looks like an edge case to me.

@kesselb kesselb added bug 3. to review Waiting for reviews labels Oct 9, 2019
@kesselb kesselb added this to the Nextcloud 18 milestone Oct 9, 2019
@wiswedel wiswedel removed their request for review October 9, 2019 16:43
This was referenced Dec 11, 2019
kesselb and others added 2 commits December 13, 2019 12:31
It's possible that json_decode returns null. Mostly the json is broken.
AddMissingDefaultValues expects an array. Pass null will fail.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 for @kesselb's fix.

Added some logging so this doesn't happen unnoticed.

@ChristophWurst ChristophWurst force-pushed the fix/16340/ignore-invalid-json branch from c9511c6 to b97d90e Compare December 13, 2019 11:40
@@ -137,6 +146,11 @@ public function getUser(IUser $user) {
}

$userDataArray = json_decode($result[0]['data'], true);
$jsonError = json_last_error();
if ($userDataArray === null || $jsonError !== JSON_ERROR_NONE) {
$this->logger->critical("User data of $uid contained invalid JSON (error $jsonError), hence falling back to a default user record");
Copy link
Contributor Author

@kesselb kesselb Dec 13, 2019

Choose a reason for hiding this comment

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

json_last_error_msg() would log a readable error message. But the numeric code is also fine for me.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't know about that!

For the sake of finally getting this in for 18 I'll leave it as is :)

@kesselb
Copy link
Contributor Author

kesselb commented Dec 13, 2019

CI looks good to me.

@blizzz blizzz merged commit f7674c5 into master Dec 13, 2019
@blizzz blizzz deleted the fix/16340/ignore-invalid-json branch December 13, 2019 14:43
@blizzz
Copy link
Member

blizzz commented Dec 13, 2019

/backport to stable17

@blizzz
Copy link
Member

blizzz commented Dec 13, 2019

/backport to stable16

@backportbot-nextcloud
Copy link

The backport to stable17 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable16 failed. Please do this backport manually.

@kesselb
Copy link
Contributor Author

kesselb commented Dec 13, 2019

18 is fine for me. We can backport it later if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to delete User
3 participants