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

SwaggerBake Schema and SwaggerBake Routes executes even when hotReload is false #543

Closed
manuelpeiso opened this issue Feb 26, 2024 · 6 comments

Comments

@manuelpeiso
Copy link
Contributor

Describe the bug
A possible bottleneck was identified in Swagger using DebugKit:
CleanShot 2024-02-14 at 15 38 26

This happens even when hotReload is set to false.
After some code analysis I realized that the swagger instance is created through the SwaggerFactory. The SwaggerFactory creates an instance of RouteScanner and also an instance of ModelScanner.
No matter what, the RouteScanner is going to read all our routes and the ModelScanner is going to iterate over all our tables.
I don't know is there is a reason for that, but I think that if hotReload is set to false we just need to read the swagger.json and do nothing with the routes and schema because it is going to use unnecessary time and resources, am I right?

I also understand that RouteScanner and ModelScanner classes are going to be used when the swagger bake commands are executed. So it is not enough including a condition like
if (!$this->config->isHotReload()){}

I have an idea thought, if the hotReload is set to false and this is not responding to a CLI request we don't need route and schema analysis at all:

 if (!$this->config->isHotReload() && PHP_SAPI !== 'cli') {
			return $return;
		}

line 49 of ModelScanner.php

if (!$this->config->isHotReload() && PHP_SAPI !== 'cli') {
$this->routes = $routes;
return;
}
line 61 of RouteScanner.php

With this changes the time of response of the swagger when the hotReload is set to false is almost instantaneous:
Screenshot from 2024-02-26 14-29-55

The bin/cake swagger bake command still works properly because the PHP_SAPI !== 'cli' condition is not meet and the routes and schema are loaded as expected to created tha swagger.json file.

Version and Platform (please complete the following information):

  • OS/Platform: Ubuntu 22.04
  • CakePHP: 4.5
  • SwaggerBake Version: 2.5.9

If you think this changes can be accurate to improve the performance I can create a PR for it. Looking forward to hear your thoughts.

@manuelpeiso
Copy link
Contributor Author

#544

@cnizzardini
Copy link
Owner

cnizzardini commented Feb 27, 2024

I believe the reason is, even if with hotReload set to off, a user may still run this code via the CLI. In fact, running via the CLI (e.g. bin/cake swagger bake) is the recommended way to deploy to production.

I see your PR has a condition for SAPI CLI, but I am highly worried about introducing this change. Why? I don't know, other than I have a hunch this will cause problems for other users.

@cnizzardini
Copy link
Owner

cnizzardini commented Feb 27, 2024

This is a valid issue and I will explore it more. However, I find it unlikely I will accept the PR for the reasons previously stated. For your specific case (I recall from slack you have something like 160+ routes), I would advise simply replacing SwaggerController with your own. The logic in there is very light:

https://github.com/cnizzardini/cakephp-swagger-bake/blob/2.x/src/Controller/SwaggerController.php

You may extract out the logic from the injected service (https://github.com/cnizzardini/cakephp-swagger-bake/blob/2.x/src/Lib/Service/OpenApiControllerService.php) and place it directly in your controller or define your own service and it inject it per: https://book.cakephp.org/4/en/development/dependency-injection.html

Is this a reasonable workaround to the performance issues your application is experiencing?

If you are keen on getting that PR passed, I will post a potential solution I might be okay with accepting in the PR.

@cnizzardini
Copy link
Owner

I'm also curious if you identified the actual source of the performance issue. Knowing the specific cause, that is, the actual line or lines of code causing the performance issues is necessary for a resolution.

@cnizzardini
Copy link
Owner

Also have you tried this: #454

@cnizzardini
Copy link
Owner

For now. I am recommending users upgrade to version 3.

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

No branches or pull requests

2 participants