Skip to content

Remove a useless #exists? in NPM PackagePresenter

David Fernandez requested to merge 10io-remove-not-so-useful-empty into master

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

Availability and Testing

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
Edited by David Fernandez

Merge request reports