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 callbacksshared example gains a case that changes an attribute the key is derived from on a saved record and expectsvalid?to leave the key as it was. Each caller names the attribute inkey_source_attributes. The new case fails for npm and Helm onmasterand passes for Rubygems, which already has the guard. The existingdoes not update object_storage_keyexample callstouch, which runs no validation, so it passes with or without the guard. - The
object_storage_key readonly attributesshared example reads the key back withmodel.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 withgot: "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
CreateMetadataCacheServicedoes withupdate!(file:, size:). A reloaded record looks its file up under its key, so reassigning the key alone madesave!fail withFile can't be blank; with the new file the presence validation reads that file, and the npm and Helm specs keeplet_it_be_with_reload.
Closes #597733 (closed)
References
- #597733 (closed)
- !229329 (merged), where the guard was proposed for
Packages::Rubygems::SpecFileand agreed, together with this follow-up for npm and Helm.
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
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.- With the two model changes reverted, the same command fails the new
when an attribute the key is derived from changesexample 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.