Include CA metadata in Gem::NameTuple#to_a - #181
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Gem::NameTuple hashing to account for content-addressable metadata while preserving the three-field serialized index format.
Changes:
- Expands
to_a, deconstruction, and hashing to five fields. - Preserves three-field wire serialization through
to_basic. - Adds conversion and hashing tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/rubygems/name_tuple.rb |
Updates tuple conversion, serialization, and hashing. |
test/rubygems/test_gem_name_tuple.rb |
Tests metadata conversion and hash behavior. |
Suppressed comments (1)
lib/rubygems/name_tuple.rb:32
- This branch accepts every array length and silently drops elements after index 4; non-array entries also become
nil.from_listprocesses downloaded spec indexes, andGem::Source#load_marshal_specsrelies on the priorArgumentErrorbehavior to reject/retry invalid caches. Restrict arrays to the documented 3- or 5-element forms and raiseArgumentErrorfor all other inputs so corrupt rows cannot be accepted or fail later outside that recovery path.
list.map do |tuple|
case tuple
when Gem::NameTuple
tuple
when Array
new(tuple[0], tuple[1], tuple[2], content_address: tuple[3], ruby_abi: tuple[4])
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9a76819 to
b8c592f
Compare
girachawda
left a comment
There was a problem hiding this comment.
Makes sense of supporting the legacy shape with to_basic while updating the NameTuple API shape with the 3 or 5 field option
OughtPuts
left a comment
There was a problem hiding this comment.
Great stuff Jen, thanks for your work ❤️ Had a few thoughts, happy to discuss!
|
|
||
| refute_equal base.hash, ca1.hash | ||
| refute_equal ca1.hash, ca2.hash | ||
| assert_equal ca1.hash, ca1_dup.hash |
There was a problem hiding this comment.
Nit: I'd say it's worth making absolutely sure this is asserting the new behaviour:
| assert_equal ca1.hash, ca1_dup.hash | |
| assert_equal [ca1.name, ca1.version, ca1.platform, ca1.content_address, ca1.ruby_abi].hash, ca1.hash |
2c8afeb to
9915896
Compare
9915896 to
761f341
Compare
a3547fe to
4dd1d3b
Compare
…iants Assisted-By: devx/25d3c4be-88ab-425e-a76b-08a00d8e9d71
761f341 to
a16ce85
Compare
f9999af
into
feature-branch-ca-changes-rubygems
Problem
Gem::NameTuple#==compared five fields —name,version,platform,content_address, andruby_abi— but#hashwasto_a.hash, andto_areturned only[name, version, platform]. That's a legal hash (equal objects still hash equal), but it meant every CA variant sharing the same name/version/platform landed in the same hash bucket.The one place this is exercised today is
commands/update_command.rb, wheregems_to_update.uniq.sortbuckets tuples byhashand disambiguates witheql?; in anySet,Array#uniq, orHashkeyed by tuples, lookups walk that bucket chain field-by-field, turning what should be O(1) into roughly O(n) as the number of variants grows.Change
to_anow returns the full 5-field tuple[name, version, platform, content_address, ruby_abi], consistent withdeconstruct_keysandinspect. Since#hashisto_a.hash, it now tracks==exactly and spreads CA variants across buckets.The slim 3-element shape is still needed for the marshalled spec index (
specs.4.8.gz), so it is now produced explicitly by the two class methods that own that contract, rather than inherited fromto_a:Gem::NameTuple.to_basic—list.map { |t| [t.name, t.version, t.platform] }(decoupled fromto_a).Gem::NameTuple.from_list— handlesNameTupleobjects (passed through, CA metadata preserved), 3-element arrays (the wire form), and 5-element arrays (the full form), routingcontent_address/ruby_abito the keyword args ofnew. This also fixes a latent data-loss where the oldnew(*t)stripped CA metadata when handed aNameTuplewhoseto_acarried it.The
==Array branch still usesto_a == other, so array equality now expects the full 5-field form.