Derive npm and Helm cache storage keys on create only

What does this MR do and why?

Packages::Npm::MetadataCache and Packages::Helm::MetadataCache run set_object_storage_key before every validation, and attr_readonly :object_storage_key keeps the key the record was created with. If the attribute the key is derived from (package_name for npm, channel for Helm) changes on a saved cache, the callback computes a new key in memory and the database never receives it. The uploader's store_dir is that key (Packages::BaseMetadataCacheUploader#dynamic_segment), so the next file goes under the unsaved key while the saved record keeps reading the previous one.

This MR adds on: :create to both callbacks, as Packages::Rubygems::SpecFile (since !229329 (merged)) and Packages::Nuget::Symbol already do, so the four Packages:: models in the issue's table all set the key once, on create. I found no code path that changes package_name or channel on a saved cache today: both CreateMetadataCacheService classes find the cache by those attributes and update only file and size. So nothing changes for users now; the guard keeps the key and the stored file together if such a path appears.

Spec changes:

  • The object_storage_key callbacks shared example gains a case that changes an attribute the key is derived from on a saved record and expects valid? to leave the key as it was. Each caller names the attribute in key_source_attributes. The new case fails for npm and Helm on master and passes for Rubygems, which already has the guard. The existing does not update object_storage_key example calls touch, which runs no validation, so it passes with or without the guard.
  • The object_storage_key readonly attributes shared example reads the key back with model.reload, as suggested on the issue. It checked the in-memory value, which matched only because the callback used to overwrite it on update. With the guard and without the reload, it fails for npm and Helm with got: "object/storage/updated_key", as it did for Rubygems in !229329 (merged).
  • That example now gives the record a new file along with the new key, as CreateMetadataCacheService does with update!(file:, size:). A reloaded record looks its file up under its key, so reassigning the key alone made save! fail with File can't be blank; with the new file the presence validation reads that file, and the npm and Helm specs keep let_it_be_with_reload.

Closes #597733 (closed)

References

Screenshots or screen recordings

A saved cache whose key attribute changes, then update! with a new file, as CreateMetadataCacheService does:

master This branch
Key in memory after the change a new key, never saved the saved key
Where the new file is written under the unsaved key under the saved key
What the record reads after find the previous file the new file
How I measured it

A throwaway spec (not part of this MR) creates an npm and a Helm cache with their factories, changes package_name or channel, calls update!(file: CarrierWaveStringFile.new('the next file'), size: ...), and then reads the record again with find. On master the file is written under .../packages/metadata_caches/npm/410c4942... while the reloaded record reads .../packages/metadata_caches/npm/b2e9d13d... and gets the factory's fixture back; Helm behaves the same. On this branch both paths are the saved key's and the reloaded record reads "the next file".

The File can't be blank failure comes from the same lookup: a cache reloaded with find, given object_storage_key = 'object/storage/updated_key', reports its file path as shared/packages/object/storage/updated_key/metadata.json, which does not exist, and valid? is false. The record create returned is still valid after the same assignment.

How to set up and validate locally

  1. bin/rspec spec/models/packages/npm/metadata_cache_spec.rb spec/models/packages/helm/metadata_cache_spec.rb spec/models/packages/rubygems/spec_file_spec.rb: 83 examples, 0 failures on this branch.
  2. With the two model changes reverted, the same command fails the new when an attribute the key is derived from changes example for npm and Helm, and nothing else.

I also ran the specs around these models on this branch, together with the three above: both CreateMetadataCacheService specs and the EE Helm one, ee/spec/models/ee/packages/helm/metadata_cache_spec.rb, ee/spec/replicators/geo/packages_helm_metadata_cache_replicator_spec.rb, the three metadata cache uploader specs, both CreateMetadataCacheWorker specs and bulk_sync_helm_metadata_cache_service_spec.rb. That is 394 examples, 0 failures. RuboCop and both commit message linters report nothing.

MR acceptance checklist

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

Edited by José M. Requena Plens

Merge request reports

Loading
Loading