use empty control when trying to get existing home in user sync by karakayasemi · Pull Request #36365 · owncloud/core

karakayasemi · 2019-11-04T10:29:50Z

Description

The server is producing unintended error logs in every user creation. This bug fixed with proper condition control. Also, since we are making lots of add/delete users in test scenarios, acceptance tests log files are full of this log, like https://drone.owncloud.com/owncloud/encryption/977/60/11 . This PR will make test log files more readable.

Related Issue

Motivation and Context

More readable log file without false positives.

How Has This Been Tested?

Use test steps given in the related issues. The error log will not be generated.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

lib/private/User/SyncService.php

codecov · 2019-11-04T11:21:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@eef2875). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #36365   +/-   ##
=========================================
  Coverage          ?   64.86%           
  Complexity        ?    19788           
=========================================
  Files             ?     1271           
  Lines             ?    74730           
  Branches          ?     1309           
=========================================
  Hits              ?    48477           
  Misses            ?    25867           
  Partials          ?      386

Flag	Coverage Δ	Complexity Δ
#javascript	`54% <ø> (?)`	`0 <ø> (?)`
#phpunit	`66.06% <100%> (?)`	`19788 <1> (?)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/SyncService.php	`83.81% <100%> (ø)`	`62 <0> (?)`
lib/private/User/Account.php	`76% <100%> (ø)`	`10 <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 eef2875...246a068. Read the comment docs.

VicDeo · 2019-11-04T13:02:15Z

lib/private/User/SyncService.php

 		// Home is handled differently, it should only be set on account creation, when there is no home already set
 		// Otherwise it could change on a sync and result in a new user folder being created
-		if ($a->getHome() === null) {
+		if (empty($a->getHome()) && $a->getHome() !== '0') {


The best way to go is not expect null from the method with signature * @method string getHome() I understand that Account::getHome is more tricky to fix. But as for me I'd better cast $a->getHome() result to string and use a strict comparison here: if ((string) $a->getHome() === '') {

VicDeo

@karakayasemi did you consider adding

	public function getHome() {
		return (string) $this->getter('home');
	}

into OC\User\Account class to ensure that getHome can't be null?

karakayasemi · 2019-11-05T06:56:00Z

@VicDeo I tried to solve the return type problem in Entity class level, because Entity class has addType method for a similar purpose. However, this unnecessarily extended the scope of the PR, also some of the unit tests failed due to the changes in Entity.

Finally, I applied your suggestion to ensure getHome will return always string. Please, review one more time, thanks.

…r sync

karakayasemi · 2019-11-05T10:41:04Z

Drone tests passed. I rebased the branch to trigger the missing codecov report.

phil-davis

LGTM. I will look in the CI logs when CI is done.

codecov-io · 2019-11-05T14:40:24Z

Codecov Report

Merging #36365 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #36365      +/-   ##
============================================
+ Coverage     64.86%   64.87%   +<.01%     
- Complexity    19788    19789       +1     
============================================
  Files          1271     1271              
  Lines         74729    74731       +2     
  Branches       1309     1309              
============================================
+ Hits          48476    48478       +2     
  Misses        25867    25867              
  Partials        386      386

Flag	Coverage Δ	Complexity Δ
#javascript	`54% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.06% <100%> (ø)`	`19789 <1> (+1)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/SyncService.php	`83.81% <100%> (ø)`	`62 <0> (ø)`	⬇️
lib/private/User/Account.php	`76% <100%> (+2.08%)`	`10 <1> (+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 eef2875...246a068. Read the comment docs.

phil-davis · 2019-11-06T09:45:32Z

An older example acceptance test run: https://drone.owncloud.com/owncloud/core/21169/39/11
has lots of:

{"reqId":"MDDmcuBjrFUVyBHlRHF4","level":3,"time":"2019-11-05T11:15:27+00:00","remoteAddr":"","user":"--","app":"no app in context","method":"--","url":"--","message":"User backend OC\\User\\Database is returning home: \/drone\/src\/data\/admin for user: admin which differs from existing value: "}

The same acceptance test suite log from today, e.g. https://drone.owncloud.com/owncloud/core/21178/39/11 does not have those entries.

Good

karakayasemi · 2019-11-06T10:29:08Z

Good to see these results. I realized the problem from acceptance test suite logs. Make them readable was one of the aims.

phil-davis reviewed Nov 4, 2019

View reviewed changes

lib/private/User/SyncService.php Outdated Show resolved Hide resolved

phil-davis reviewed Nov 4, 2019

View reviewed changes

lib/private/User/SyncService.php Outdated Show resolved Hide resolved

karakayasemi force-pushed the fix/32438 branch from 3f6b173 to 3991fea Compare November 4, 2019 11:21

karakayasemi force-pushed the fix/32438 branch from 3991fea to eb3d97a Compare November 4, 2019 11:23

karakayasemi marked this pull request as ready for review November 4, 2019 11:26

karakayasemi requested review from VicDeo, mmattel and phil-davis November 4, 2019 11:26

karakayasemi self-assigned this Nov 4, 2019

karakayasemi added the 3 - To Review label Nov 4, 2019

karakayasemi requested a review from micbar November 4, 2019 12:51

VicDeo reviewed Nov 4, 2019

View reviewed changes

karakayasemi force-pushed the fix/32438 branch 4 times, most recently from 98dd573 to d67b536 Compare November 5, 2019 04:44

karakayasemi added 2 - Developing and removed 3 - To Review labels Nov 5, 2019

karakayasemi force-pushed the fix/32438 branch from d67b536 to 80a3e1a Compare November 5, 2019 06:49

karakayasemi requested a review from VicDeo November 5, 2019 06:56

use correct condition control when trying to get existing home in use…

246a068

…r sync

karakayasemi force-pushed the fix/32438 branch from 80a3e1a to 246a068 Compare November 5, 2019 10:39

karakayasemi added 3 - To Review and removed 2 - Developing labels Nov 5, 2019

VicDeo approved these changes Nov 5, 2019

View reviewed changes

phil-davis approved these changes Nov 5, 2019

View reviewed changes

karakayasemi merged commit 27d4865 into master Nov 5, 2019

karakayasemi deleted the fix/32438 branch November 5, 2019 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use empty control when trying to get existing home in user sync#36365

use empty control when trying to get existing home in user sync#36365
karakayasemi merged 1 commit intomasterfrom
fix/32438

karakayasemi commented Nov 4, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 4, 2019 •

edited

Loading

Uh oh!

VicDeo Nov 4, 2019

Uh oh!

VicDeo left a comment

Uh oh!

karakayasemi commented Nov 5, 2019

Uh oh!

karakayasemi commented Nov 5, 2019

Uh oh!

phil-davis left a comment

Uh oh!

codecov-io commented Nov 5, 2019 •

edited

Loading

Uh oh!

phil-davis commented Nov 6, 2019

Uh oh!

karakayasemi commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

karakayasemi commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VicDeo Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

VicDeo left a comment

Choose a reason for hiding this comment

Uh oh!

karakayasemi commented Nov 5, 2019

Uh oh!

karakayasemi commented Nov 5, 2019

Uh oh!

phil-davis left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

phil-davis commented Nov 6, 2019

Uh oh!

karakayasemi commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karakayasemi commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 4, 2019 •

edited

Loading

codecov-io commented Nov 5, 2019 •

edited

Loading