Skip to content

feat(AppFramework): Add Route attribute #42801

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.

Sign up for GitHub

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

Jump to bottom
Merged
provokateurin merged 5 commits into from
Feb 21, 2024
Merged

feat(AppFramework): Add Route attribute #42801

provokateurin merged 5 commits into from
Feb 21, 2024

Conversation

@provokateurin
Copy link
Member

@provokateurin provokateurin commented Jan 15, 2024 •
edited
Loading

Summary

Depends on openapi-extractor: nextcloud/openapi-extractor#73
Documentation: nextcloud/documentation#11442

A new routing method that should make it easier for new developers. It avoids having to manually verify the connection between the layers of the routes and the controllers.

The changes to the openapi.json file are annoying but required because the order of the routes changed. You can verify there were no changes using http://play.jd-tool.io (with unordered set option). Since there is no diff we can also be sure I migrated all routes correctly.

I did some benchmarking and would consider the slight drop in performance to be acceptable. It is about 2-5ms which is not noticable and there are many other factors in a remote HTTP connection that cause way more delay (tests were done on localhost). I have some ideas which could be tried to improve the performance a bit (but there is no guarantee that they will).

Only the 99th percentile is really relevant as the longest request can also be a fluke (see in the baseline testing for the capabilities endpoint). All tests were done with 10k requests on the standalone PHP webserver with SQLite and without any caching configured.

The core test is to check for changes in parsing the routes and the test with Talk (which has not been migrated) is to check the impact on apps with many routes. The total number of enabled apps has no performance impact on this new mechanism.

Before:

$ ab -n 10000 -H "OCS-APIRequest: true" http://localhost:8080/ocs/v2.php/cloud/capabilities
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 1000 requests
Completed 2000 requests
Completed 3000 requests
Completed 4000 requests
Completed 5000 requests
Completed 6000 requests
Completed 7000 requests
Completed 8000 requests
Completed 9000 requests
Completed 10000 requests
Finished 10000 requests


Server Software:
Server Hostname:        localhost
Server Port:            8080

Document Path:          /ocs/v2.php/cloud/capabilities
Document Length:        1220 bytes

Concurrency Level:      1
Time taken for tests:   143.307 seconds
Complete requests:      10000
Failed requests:        0
Total transferred:      25310186 bytes
HTML transferred:       12200000 bytes
Requests per second:    69.78 [#/sec] (mean)
Time per request:       14.331 [ms] (mean)
Time per request:       14.331 [ms] (mean, across all concurrent requests)
Transfer rate:          172.48 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    12   14   4.0     13     354
Waiting:       12   14   4.0     13     353
Total:         12   14   4.0     13     354

Percentage of the requests served within a certain time (ms)
  50%     13
  66%     14
  75%     14
  80%     15
  90%     17
  95%     19
  98%     21
  99%     22
 100%    354 (longest request)

$ ab -n 10000 -H "OCS-APIRequest: true" -H "Authorization: Bearer bgN7T-A4q9p-QTT5C-QcFEK-57wfc" http://localhost:8080/ocs/v2.php/apps/spreed/api/v4/room
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 1000 requests
Completed 2000 requests
Completed 3000 requests
Completed 4000 requests
Completed 5000 requests
Completed 6000 requests
Completed 7000 requests
Completed 8000 requests
Completed 9000 requests
Completed 10000 requests
Finished 10000 requests


Server Software:
Server Hostname:        localhost
Server Port:            8080

Document Path:          /ocs/v2.php/apps/spreed/api/v4/room
Document Length:        7612 bytes

Concurrency Level:      1
Time taken for tests:   260.035 seconds
Complete requests:      10000
Failed requests:        0
Total transferred:      89919180 bytes
HTML transferred:       76120000 bytes
Requests per second:    38.46 [#/sec] (mean)
Time per request:       26.003 [ms] (mean)
Time per request:       26.003 [ms] (mean, across all concurrent requests)
Transfer rate:          337.69 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    23   26   4.2     24     269
Waiting:       23   26   4.2     24     267
Total:         23   26   4.2     24     269

Percentage of the requests served within a certain time (ms)
  50%     24
  66%     26
  75%     27
  80%     28
  90%     31
  95%     33
  98%     35
  99%     37
 100%    269 (longest request)

Now:

$ ab -n 10000 -H "OCS-APIRequest: true" http://localhost:8080/ocs/v2.php/cloud/capabilities
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 1000 requests
Completed 2000 requests
Completed 3000 requests
Completed 4000 requests
Completed 5000 requests
Completed 6000 requests
Completed 7000 requests
Completed 8000 requests
Completed 9000 requests
Completed 10000 requests
Finished 10000 requests


Server Software:
Server Hostname:        localhost
Server Port:            8080

Document Path:          /ocs/v2.php/cloud/capabilities
Document Length:        1220 bytes

Concurrency Level:      1
Time taken for tests:   147.590 seconds
Complete requests:      10000
Failed requests:        0
Total transferred:      25309812 bytes
HTML transferred:       12200000 bytes
Requests per second:    67.76 [#/sec] (mean)
Time per request:       14.759 [ms] (mean)
Time per request:       14.759 [ms] (mean, across all concurrent requests)
Transfer rate:          167.47 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    13   15   2.5     14      42
Waiting:       12   14   2.5     13      41
Total:         13   15   2.5     14      42

Percentage of the requests served within a certain time (ms)
  50%     14
  66%     14
  75%     15
  80%     16
  90%     18
  95%     20
  98%     22
  99%     24
 100%     42 (longest request)

$ ab -n 10000 -H "OCS-APIRequest: true" -H "Authorization: Bearer bgN7T-A4q9p-QTT5C-QcFEK-57wfc" http://localhost:8080/ocs/v2.php/apps/spreed/api/v4/room
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 1000 requests
Completed 2000 requests
Completed 3000 requests
Completed 4000 requests
Completed 5000 requests
Completed 6000 requests
Completed 7000 requests
Completed 8000 requests
Completed 9000 requests
Completed 10000 requests
Finished 10000 requests


Server Software:
Server Hostname:        localhost
Server Port:            8080

Document Path:          /ocs/v2.php/apps/spreed/api/v4/room
Document Length:        7612 bytes

Concurrency Level:      1
Time taken for tests:   279.000 seconds
Complete requests:      10000
Failed requests:        0
Total transferred:      89920016 bytes
HTML transferred:       76120000 bytes
Requests per second:    35.84 [#/sec] (mean)
Time per request:       27.900 [ms] (mean)
Time per request:       27.900 [ms] (mean, across all concurrent requests)
Transfer rate:          314.74 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    24   28   5.3     26     360
Waiting:       24   27   5.2     26     359
Total:         24   28   5.3     27     360

Percentage of the requests served within a certain time (ms)
  50%     27
  66%     27
  75%     28
  80%     29
  90%     34
  95%     38
  98%     39
  99%     42
 100%    360 (longest request)

Checklist

@provokateurin provokateurin added this to the Nextcloud 29 milestone Jan 15, 2024
@provokateurin
Copy link
Member Author

provokateurin commented Jan 15, 2024

No idea why OpenAPI changes (seems to use the old version?), but the openapi-extractor PR needs to be merged first anyway.

nickvergessen
nickvergessen reviewed Jan 15, 2024
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

We discussed at another point recently that all future API work should be using OCS routes so APIs can also be used by mobile/desktop clients.

Could make this more clear by biasing the constant names.

Other than that, as mentioned I love the idea as it will reduce conflicts, amount of file and bring the 2 things that depend on each other closer together.

@nickvergessen
Copy link
Member

nickvergessen commented Jan 15, 2024

Since we use the Symfony router under the hood. Would there be an issue extending/using it's attribute as a base? We ship it already: https://github.com/nextcloud/3rdparty/blob/8a78b41c955bb3530a5c25f187e3dc96091dadd7/symfony/routing/Annotation/Route.php

Ref https://symfony.com/doc/current/routing.html

That could even allow us to remove more routing code in the future?

@provokateurin
Copy link
Member Author

provokateurin commented Jan 15, 2024 •
edited
Loading

Would there be an issue extending/using it's attribute as a base?

I didn't know about this. Removing routing code sounds good, but this attribute also supports a lot of stuff that we don't support in our routing yet (and it has a slightly different signature than our current routing parameters e.g. verb vs methods). I don't think that is a good idea :/

@provokateurin
Copy link
Member Author

provokateurin commented Jan 15, 2024

(I only migrated core and one other app to ensure it works for both since they are handled a bit different in the router)

@ChristophWurst ChristophWurst added pending documentation This pull request needs an associated documentation update and removed pending documentation This pull request needs an associated documentation update labels Jan 16, 2024
@ChristophWurst
Copy link
Member

ChristophWurst commented Jan 16, 2024

Blackfire analysis:

Every comparison is primed with a simple curl to warm caches.
Every test is run with 10 samples to get more accurate numbers.

@provokateurin
Copy link
Member Author

provokateurin commented Jan 16, 2024

@ChristophWurst thanks for the profiling. When I did my testing I found that even with 1k requests I got very different results every time. At 10k it was quite stable.

To everyone: Do you think the performance hit is acceptable or not?

@provokateurin provokateurin force-pushed the feat/appframework/route-attribute branch from 2e69f6e to 45d52cc Compare January 17, 2024 10:00
This was referenced Jan 24, 2024
Merged
Closed
julien-nc
julien-nc requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Code looks good and works fine.

The performance hit seems acceptable to me.

@provokateurin
Copy link
Member Author

provokateurin commented Feb 5, 2024

This branch is currently broken if navigate to the root of the instance. Any other URL will work fine. I'm looking into it already.

@provokateurin provokateurin force-pushed the feat/appframework/route-attribute branch from 45d52cc to 91bdf72 Compare February 8, 2024 10:23
@provokateurin
Copy link
Member Author

provokateurin commented Feb 8, 2024

Hi everyone, so after a debugging session with @julien-nc we found the problem:
Symfony allows you to define the route names (e.g. in routes.php) using a different casing than the name of the controller class. This same casing then has to be used when using the URLGenerator.
This lead to the problem that my Route attribute generates the route names automatically from the controller class name.
For example previously a route was registered as login#showLoginForm, but the Route attribute automatically generated Login#showLoginForm. This made the link to the route core.login.showLoginForm no longer work because of the casing.
In the end this is an oversight in symfony as it simply shouldn't allow registering routes with the wrong casing in the name, but we have to live with that. To workaround it and not break everything the only solution we found was to lowercase all route names all the time. In PHP this is save because methods or classes with the same name but different casing will still collide.
The alternative would have been to add another attribute that allows overriding the controller name that is used for generating the routes, but the whole idea behind this is to reduce load on developers and not add more.

@ChristophWurst @come-nc @nickvergessen @juliushaertl please review again, this is finally ready.

come-nc
come-nc reviewed Feb 12, 2024
View reviewed changes
@provokateurin provokateurin force-pushed the feat/appframework/route-attribute branch from 63aa6b5 to 57d9218 Compare February 12, 2024 10:34
@provokateurin
Copy link
Member Author

provokateurin commented Feb 12, 2024

CI failure is fixed, it was just another case of the casing problem with matching legacy routes.

julien-nc
julien-nc approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

👍

come-nc
come-nc approved these changes Feb 12, 2024
View reviewed changes
@julien-nc julien-nc mentioned this pull request Feb 13, 2024
Merged
nickvergessen
nickvergessen reviewed Feb 15, 2024
View reviewed changes
@provokateurin provokateurin force-pushed the feat/appframework/route-attribute branch from e3453b6 to b84c1eb Compare February 21, 2024 10:29
@provokateurin provokateurin force-pushed the feat/appframework/route-attribute branch from b84c1eb to 2c51933 Compare February 21, 2024 11:08
@provokateurin provokateurin merged commit fa8b4f3 into master Feb 21, 2024
@provokateurin provokateurin deleted the feat/appframework/route-attribute branch February 21, 2024 13:09
@provokateurin provokateurin mentioned this pull request Feb 21, 2024
Closed
8 tasks
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
@provokateurin provokateurin mentioned this pull request Sep 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@julien-nc julien-nc julien-nc approved these changes

@come-nc come-nc come-nc approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@bigcat88 bigcat88 Awaiting requested review from bigcat88

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

No one assigned

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Milestone

Nextcloud 29

Development

Successfully merging this pull request may close these issues.

7 participants

@provokateurin @nickvergessen @ChristophWurst @juliusknorr @julien-nc @come-nc