Remove a useless #exists? in NPM PackagePresenter
What does this MR do?
(From the rails performance training session 3, following the rule of thumb: each table should be hit only once per request)
This MR removes a #exists?
from the Packages::Npm::PackagePresenter
. The presenter has to go through all the package dependencies to build a hash out of them. There is no need to guard this part with an #exists?
statement as we can let the query run. If it's empty, a loop is avoided and an empty hash is returned. If it's not empty, the loop will run will run as usual.
The benefit here, is that we save one SQL request to the database for NPM packages with dependencies.
This presenter is used when npm
or yarn
request the metadata of a given package to the GitLab NPM registry.
Screenshots
Below, the console log of the presenter execution truncated to present only the SQL generated by the presenter.
NPM package without dependencies
Before this MR
Packages::Package Load (0.5ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' GROUP BY "packages_packages"."version")
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::PackageFile Load (0.2ms) SELECT "packages_package_files".* FROM "packages_package_files" WHERE "packages_package_files"."package_id" = 148
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::DependencyLink Exists? (0.7ms) SELECT 1 AS one FROM "packages_dependency_links" WHERE "packages_dependency_links"."package_id" = 148 LIMIT 1
↳ ee/app/presenters/packages/npm/package_presenter.rb:63:in `build_package_dependencies'
Packages::Tag Load (0.6ms) SELECT "packages_tags".* FROM "packages_tags" WHERE "packages_tags"."package_id" IN (SELECT "packages_packages"."id" FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' GROUP BY "packages_packages"."version")) ORDER BY "packages_tags"."updated_at" DESC LIMIT 200
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Packages::Package Load (0.2ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."id" = 148
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Notice the Packages::DependencyLink Exists?
statement
After this MR
Packages::Package Load (0.4ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' GROUP BY "packages_packages"."version")
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::PackageFile Load (1.0ms) SELECT "packages_package_files".* FROM "packages_package_files" WHERE "packages_package_files"."package_id" = 148
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::DependencyLink Load (0.7ms) SELECT "packages_dependency_links".* FROM "packages_dependency_links" WHERE "packages_dependency_links"."package_id" = 148 AND "packages_dependency_links"."dependency_type" IN (1, 2, 3, 4) ORDER BY "packages_dependency_links"."id" ASC LIMIT 1000
↳ ee/app/presenters/packages/npm/package_presenter.rb:68:in `build_package_dependencies'
Packages::Tag Load (0.6ms) SELECT "packages_tags".* FROM "packages_tags" WHERE "packages_tags"."package_id" IN (SELECT "packages_packages"."id" FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/faerie' GROUP BY "packages_packages"."version")) ORDER BY "packages_tags"."updated_at" DESC LIMIT 200
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Packages::Package Load (0.2ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."id" = 148
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
The Packages::DependencyLink Exists?
has been replaced with Packages::DependencyLink Load
. The same number of SQL request are done for this case.
NPM package with dependencies
Before this MR
Packages::Package Load (0.8ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' GROUP BY "packages_packages"."version")
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::PackageFile Load (0.5ms) SELECT "packages_package_files".* FROM "packages_package_files" WHERE "packages_package_files"."package_id" = 176
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::DependencyLink Exists? (0.4ms) SELECT 1 AS one FROM "packages_dependency_links" WHERE "packages_dependency_links"."package_id" = 176 LIMIT 1
↳ ee/app/presenters/packages/npm/package_presenter.rb:63:in `build_package_dependencies'
Packages::DependencyLink Load (0.5ms) SELECT "packages_dependency_links".* FROM "packages_dependency_links" WHERE "packages_dependency_links"."package_id" = 176 AND "packages_dependency_links"."dependency_type" IN (1, 2, 3, 4) ORDER BY "packages_dependency_links"."id" ASC LIMIT 1000
↳ ee/app/presenters/packages/npm/package_presenter.rb:70:in `build_package_dependencies'
Packages::Dependency Load (0.4ms) SELECT "packages_dependencies".* FROM "packages_dependencies" WHERE "packages_dependencies"."id" IN (2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43)
↳ ee/app/presenters/packages/npm/package_presenter.rb:70:in `build_package_dependencies'
Packages::Tag Load (1.0ms) SELECT "packages_tags".* FROM "packages_tags" WHERE "packages_tags"."package_id" IN (SELECT "packages_packages"."id" FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' GROUP BY "packages_packages"."version")) ORDER BY "packages_tags"."updated_at" DESC LIMIT 200
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Packages::Package Load (0.3ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."id" = 176
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Notice the Packages::DependencyLink Exists?
followed by the Packages::DependencyLink Load
.
After this MR
Packages::Package Load (1.0ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' GROUP BY "packages_packages"."version")
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::PackageFile Load (0.5ms) SELECT "packages_package_files".* FROM "packages_package_files" WHERE "packages_package_files"."package_id" = 176
↳ ee/app/presenters/packages/npm/package_presenter.rb:20:in `versions'
Packages::DependencyLink Load (0.6ms) SELECT "packages_dependency_links".* FROM "packages_dependency_links" WHERE "packages_dependency_links"."package_id" = 176 AND "packages_dependency_links"."dependency_type" IN (1, 2, 3, 4) ORDER BY "packages_dependency_links"."id" ASC LIMIT 1000
↳ ee/app/presenters/packages/npm/package_presenter.rb:68:in `build_package_dependencies'
Packages::Dependency Load (0.5ms) SELECT "packages_dependencies".* FROM "packages_dependencies" WHERE "packages_dependencies"."id" IN (2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43)
↳ ee/app/presenters/packages/npm/package_presenter.rb:68:in `build_package_dependencies'
Packages::Tag Load (1.0ms) SELECT "packages_tags".* FROM "packages_tags" WHERE "packages_tags"."package_id" IN (SELECT "packages_packages"."id" FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' AND "packages_packages"."id" IN (SELECT MAX(id) AS id FROM "packages_packages" WHERE "packages_packages"."project_id" = 1 AND "packages_packages"."package_type" = 2 AND "packages_packages"."name" = '@gitlab-org/mermaid' GROUP BY "packages_packages"."version")) ORDER BY "packages_tags"."updated_at" DESC LIMIT 200
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Packages::Package Load (0.3ms) SELECT "packages_packages".* FROM "packages_packages" WHERE "packages_packages"."id" = 176
↳ ee/app/presenters/packages/npm/package_presenter.rb:39:in `map'
Packages::DependencyLink Exists?
is avoided and thus avoid an additional SQL request.
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team