Fix version comparison with minor and patch level requirements #9024

ChristophWurst · 2018-03-29T07:50:49Z

If an app requires a specific minor or path level server version,
the version_compare prevented the installation as only the major
version had been compared and that checks obviously returns false.

php > var_dump(version_compare('13', '13.0.1', '>='));
bool(false)
php > var_dump(version_compare('13.0.2', '13.0.1', '>='));
bool(true)

Now the full version is used for comparison, making it possible to
release apps for a specific minor or patch level version of Nextcloud.

This is a quick fix for discussion. There are probably some tests that will fail now.

cc @rullzer @nickvergessen as discussed.

nickvergessen · 2018-03-29T07:55:54Z

lib/private/App/AppStore/Fetcher/AppFetcher.php

+						// Version is bigger or equals to the minimum version of the app
+						version_compare($ncVersion, $version->getMinimumVersion(), '>=')
+						// Version is smaller or equals to the maximum version of the app
+						&& version_compare($ncVersion, $version->getMaximumVersion(), '<=')


Ah here is what breaks, all apps currently have max-version="13"
So we need to keep the major only check for the max version.

That's disputable. I think if someone specifies the full version requirement like 13.0.1 (for whatever reason) we should do the full check. But of course, if max is set to 13, we should allow any version of Nextcloud 13.

Couldn't we align the comparison to the same amount of digits? Then use the <= and >= operators and it would be fine. If max version is 13.0.2 we would compare against the full version if it is 13 we would only use the major version.

codecov · 2018-04-03T12:53:20Z

Codecov Report

Merging #9024 into master will increase coverage by 0.02%.
The diff coverage is 95.34%.

@@             Coverage Diff              @@
##             master    #9024      +/-   ##
============================================
+ Coverage     51.91%   51.94%   +0.02%     
+ Complexity    25325    25323       -2     
============================================
  Files          1603     1605       +2     
  Lines         95180    95169      -11     
  Branches       1387     1387              
============================================
+ Hits          49414    49433      +19     
+ Misses        45766    45736      -30

Impacted Files	Coverage Δ	Complexity Δ
lib/private/App/CompareVersion.php	`100% <100%> (ø)`	`8 <8> (?)`
lib/private/App/AppStore/Fetcher/AppFetcher.php	`89.47% <84.61%> (-2.69%)`	`17 <0> (+2)`
apps/user_ldap/lib/Mapping/AbstractMapping.php	`67.3% <0%> (-3.64%)`	`26% <0%> (-3%)`
apps/files_trashbin/lib/Expiration.php	`90.32% <0%> (-1.62%)`	`29% <0%> (ø)`
settings/routes.php	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
lib/private/User/Manager.php	`84.34% <0%> (ø)`	`78% <0%> (ø)`	⬇️
apps/user_ldap/lib/AccessFactory.php	`0% <0%> (ø)`	`2% <0%> (ø)`	⬇️
core/Controller/LoginController.php	`79.02% <0%> (ø)`	`38% <0%> (ø)`	⬇️
lib/private/User/Session.php	`74.84% <0%> (ø)`	`117% <0%> (ø)`	⬇️
apps/user_ldap/ajax/clearMappings.php	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
... and 8 more

ChristophWurst · 2018-04-03T12:56:24Z

I've added a new version comparison class to compare app requirements. I chose to move the logic into a new class because it's likely to be used in other places too and because it makes testing a lot easier. Ref #9024 (comment).

There's one issue: the existing test cases fail now, because they use a 11.0.0.2 server version for testing. Is that even a valid server version? Should I adjust the test or my logic? In case of the latter: what would be the semantics of x.x.x.x version comparisons? cc @MorrisJobke @nickvergessen

MorrisJobke · 2018-04-03T14:30:38Z

There's one issue: the existing test cases fail now, because they use a 11.0.0.2 server version for testing. Is that even a valid server version? Should I adjust the test or my logic? In case of the latter: what would be the semantics of x.x.x.x version comparisons? cc @MorrisJobke @nickvergessen

Yes it is. The 4th digit is our internal version number to be able to trigger DB migrations ;) But they should now be used outside of our server core. So the info.xml should at most hold 3 parts.

ChristophWurst · 2018-04-04T08:52:51Z

Yes it is. The 4th digit is our internal version number to be able to trigger DB migrations ;) But they should now be used outside of our server core. So the info.xml should at most hold 3 parts.

Fixed. Please see the test data to verify I implemented the compatibility logic correctly 😉

ChristophWurst · 2018-04-04T09:06:40Z

lib/private/App/AppStore/Fetcher/AppFetcher.php

+					$min = $version->getMinimumVersion();
+					$max = $version->getMaximumVersion();
+					$minFulfilled = $this->compareVersion->isCompatible($ncVersion, $min, '>=');
+					$maxFulfilled = $max !== '' &&


FYI this is merely for backwards compatibility. Nowadays apps have to specify their min and max requirements, thus this check is not necessary anymore. However, the test data contains apps that have no max-requirement set and thus this code would fail. Since I'm not 100% sure if the app store would distribute these apps, I took the safe path and added this check defensively.

MorrisJobke · 2018-04-04T10:24:16Z

lib/private/App/CompareVersion.php

+	public function isCompatible(string $actual, string $required,
+		string $comparator = '>='): bool {
+
+		if (!preg_match(self::REGEX_SERVER, $actual)) {


Wouldn't it make sense to do then only a check agains REGEX_MAJOR_MINOR_PATCH because it is excluded below anyways (in the else branch)?

Below only $required is checked, not $actual. And the server version is in x.x.x(.x) format, hence the special regex with the optional forth-level version.

MorrisJobke · 2018-04-04T13:10:29Z

lib/private/App/AppStore/Fetcher/AppFetcher.php

+					$ncVersion = $this->getVersion();
+					$min = $version->getMinimumVersion();
+					$max = $version->getMaximumVersion();
+					$minFulfilled = $this->compareVersion->isCompatible($ncVersion, $min, '>=');


Also catch the exceptions and log them - otherwise this gets messy and user complain about broken app pages and stuff like that.

Also catch the exceptions and log them - otherwise this gets messy and user complain about broken app pages and stuff like that.

What shall we do in case of a thrown exception? Ignore the app? Install anyway? IMO this one is unexpected for normal use and is only thrown if the class is used wrongly.

It should ignore the app - would be horrible if you have one broken app installed and the app management would not work anymore.

catch it => Log it

And remove from the app list so people don't even see it (so can't install)

FYI: while we could add that (IMO ugly) check, it's not actually possible that an app would have an invalid version requirement at the moment because it's validated with the info.xml schema:

<xs:simpleType name="version"> <xs:restriction base="limited-string"> <xs:pattern value="[0-9]+(\.[0-9]+){0,2}"/> </xs:restriction> </xs:simpleType>

MorrisJobke

Fine now 👍

Fixed

MorrisJobke · 2018-04-10T22:07:26Z

@ChristophWurst Please have a final look at my changes 😉

ChristophWurst

🐘

MorrisJobke · 2018-04-11T00:10:16Z

Dumping the autoloader is missing

If an app requires a specific minor or path level server version, the version_compare prevented the installation as only the major version had been compared and that checks obviously returns `false`. Now the full version is used for comparison, making it possible to release apps for a specific minor or patch level version of Nextcloud. Signed-off-by: Christoph Wurst <[email protected]>

Signed-off-by: Morris Jobke <[email protected]>

Signed-off-by: Roeland Jago Douma <[email protected]>

ChristophWurst added bug 2. developing Work in progress feature: apps management labels Mar 29, 2018

ChristophWurst added this to the Nextcloud 14 milestone Mar 29, 2018

ChristophWurst self-assigned this Mar 29, 2018

nickvergessen approved these changes Mar 29, 2018

View reviewed changes

nickvergessen previously requested changes Mar 29, 2018

View reviewed changes

ChristophWurst mentioned this pull request Apr 3, 2018

Release 1.5.2 (Backport for Nextcloud 13) nextcloud/twofactor_u2f#129

Merged

3 tasks

ChristophWurst force-pushed the fix/app-fetcher-major-minor-versions branch from fb323da to 385404d Compare April 4, 2018 08:52

ChristophWurst commented Apr 4, 2018

View reviewed changes

MorrisJobke reviewed Apr 4, 2018

View reviewed changes

ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 4, 2018

MorrisJobke reviewed Apr 4, 2018

View reviewed changes

ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 5, 2018

ChristophWurst mentioned this pull request Apr 10, 2018

Also list releases for minor server versions nextcloud/appstore#560

Closed

rullzer approved these changes Apr 10, 2018

View reviewed changes

ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 10, 2018

MorrisJobke approved these changes Apr 10, 2018

View reviewed changes

ChristophWurst commented Apr 10, 2018

View reviewed changes

MorrisJobke and others added 2 commits April 11, 2018 10:08

Log possible exception caused by wrong version number

3d80ade

Signed-off-by: Morris Jobke <[email protected]>

Bump autoloader

52f1f04

Signed-off-by: Roeland Jago Douma <[email protected]>

rullzer force-pushed the fix/app-fetcher-major-minor-versions branch from 0cb1286 to 52f1f04 Compare April 11, 2018 08:09

MorrisJobke merged commit 88c1b8e into master Apr 11, 2018

MorrisJobke deleted the fix/app-fetcher-major-minor-versions branch April 11, 2018 13:48

ChristophWurst mentioned this pull request May 14, 2018

Version on apps.nextcloud.com not updated nextcloud/twofactor_u2f#164

Closed

ChristophWurst mentioned this pull request Jun 7, 2022

Fix @since comment in IManager #32739

Closed

Uh oh!

Fix version comparison with minor and patch level requirements #9024

Fix version comparison with minor and patch level requirements #9024

Uh oh!

Conversation

ChristophWurst commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ChristophWurst commented Apr 3, 2018

Uh oh!

MorrisJobke commented Apr 3, 2018

Uh oh!

ChristophWurst commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Apr 10, 2018

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Apr 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Apr 3, 2018 •

edited

Loading