Skip to content

Refactor NPM metadata endpoint

Radamanthus Batnag requested to merge 422372-refactor-npm-metadata-endpoint into master

NOTE: Given the blast radius (NPM is the most-used package format) I thought of putting the code changes behind a feature flag. But because the code changes moving code into/out of modules, we'll need to do feature flag evaluation at the class level, which is against our backend feature flag guidelines. I ended up not gating the changes behind a feature flag. Suggestions on how to gate the changes behind a feature flag are much appreciated! 🙇

💣 Problem

The NPM metadata endpoint code for the project level has diverged from the code for the instance and group level. It is currently a Concern shared among the project, group, and instance endpoint classes. But because of the diverging behavior, there are a number of if endpoint_scope... checks sprinkled throughout the endpoint code.

We could simplify the logic of the metadata endpoint if we define it separately on the project level and use a shared metadata endpoint for group and instance level, since it's still very similar.

🚑 Solution

  • Copy the metadata endpoint definition from the concern into the project endpoint file, lib/api/npm_project_packages.rb
  • On the project endpoint:
    • Remove the if endpoint_scope checks and delete the unused code branch
  • Move the metadata endpoint definition from the concern into a new module, (NpmNamespaceEndpoints) and include this module in lib/api/npm_instance_packages.rb and lib/api/npm_group_packages.rb
    • Remove the if endpoint_scope checks and delete the unused code branch in NpmNamespaceEndpoints

The changes can be hard to review by looking at the diffs view. It might be easier to compare side by side the master and MR versions of these files:

  • lib/api/concerns/packages/npm_endpoints.rb
  • lib/api/concerns/packages/npm_namespace_endpoints.rb (new file introduced by MR)
  • lib/api/npm_project_packages.rb

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

No UI changes 🌈

How to set up and validate locally

Check for regressions in npm install (uses the metadata endpoint) for all three levels:

NOTE: You'll need to install a package at the appropriate level first

Related to #422372 (closed)

Edited by Radamanthus Batnag

Merge request reports