Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resource identity comparable #1430

Merged
merged 1 commit into from
Jan 16, 2024
Merged

resource identity comparable #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jsonapi-resources.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Gem::Specification.new do |spec|
spec.add_development_dependency 'database_cleaner'
spec.add_development_dependency 'hashie'
spec.add_development_dependency 'sorted_set'
spec.add_development_dependency 'memory_profiler'
spec.add_dependency 'activerecord', '>= 5.1'
spec.add_dependency 'railties', '>= 5.1'
spec.add_dependency 'concurrent-ruby'
Expand Down
13 changes: 12 additions & 1 deletion lib/jsonapi/resource_identity.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ module JSONAPI
# rid = ResourceIdentity.new(PostResource, 12)
#
class ResourceIdentity
include Comparable

# Store the identity parts as an array to avoid allocating a new array for the hash method to work on
def initialize(resource_klass, id)
@identity_parts = [resource_klass, id]
Expand Down Expand Up @@ -41,7 +43,16 @@ def hash
end

def <=>(other_identity)
self.id <=> other_identity.id
return nil unless other_identity.is_a?(ResourceIdentity)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparsble includes some of the above methods. Is the override here intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the idea is we have a composite key and need to ensure we sort consistently even when given a polymorphic list of ResourceIdentities. This logic places grouping by type as a higher priority than by id. I chose that logic since it will seem more natural when ids are either UUIDs or integers.

Comparable uses <=> to implement the conventional comparison operators (<, <=, ==, >=, and >) and the method between?.

I could remove the override of == as well, which might be more performant than the hash call. I'm going to test this and see what makes the most sense. I wish I had focused on including Comparable earlier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut was wrong and it like I should keep the override of == that uses hash as it's about twice as fast in the most common case where the resource klasses are the same:

>> require 'benchmark'
=> false
>> # Using hash method
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(Image, 13); puts Benchmark.measure { 1_000_000.times { rid1 == rid2 } }
  0.194055   0.000052   0.194107 (  0.195783)

>> # Using new <=> logic prioritizing resource_klass - Case where they are equal
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(Image, 13); puts Benchmark.measure { 1_000_000.times { rid1 <=> rid2 } }
  0.398467   0.000156   0.398623 (  0.401471)

>> # Using new <=> logic prioritizing resource_klass - Case where resource klasses differ, which should only do one comparison (best case)
>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(User, 13); puts Benchmark.measure { 1_000_000.times { rid1 <=> rid2 } }
  0.330682   0.000103   0.330785 (  0.332534)

Copy link
Member Author

@lgebhardt lgebhardt Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tested removing the check on other_identity.is_a?(ResourceIdentity) and it only saved a small amount of time:

>> rid1 = JSONAPI::ResourceIdentity.new(Image, 13); rid2 = JSONAPI::ResourceIdentity.new(Image, 13); puts Benchmark.measure { 1_000_000.times { rid1 <=> rid2 } }
  0.379685   0.000151   0.379836 (  0.383279)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job testing!


case self.resource_klass.name <=> other_identity.resource_klass.name
when -1
-1
when 1
1
else
self.id <=> other_identity.id
end
end

# Creates a string representation of the identifier.
Expand Down
58 changes: 58 additions & 0 deletions test/unit/resource/resource_identity_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
require File.expand_path('../../../test_helper', __FILE__)
require 'memory_profiler'

class ResourceIdentity < ActiveSupport::TestCase

def test_can_generate_a_consistent_hash_for_comparison
rid = JSONAPI::ResourceIdentity.new(PostResource, 12)
assert_equal(rid.hash, [PostResource, 12].hash)
end

def test_equality
rid = JSONAPI::ResourceIdentity.new(PostResource, 12)
rid2 = JSONAPI::ResourceIdentity.new(PostResource, 12)
assert_equal(rid, rid2) # uses == internally
assert rid.eql?(rid2)
end

def test_inequality
rid = JSONAPI::ResourceIdentity.new(PostResource, 12)
rid2 = JSONAPI::ResourceIdentity.new(PostResource, 13)
refute_equal(rid, rid2)
end

def test_sorting_by_resource_class_name
rid = JSONAPI::ResourceIdentity.new(CommentResource, 13)
rid2 = JSONAPI::ResourceIdentity.new(PostResource, 13)
rid3 = JSONAPI::ResourceIdentity.new(SectionResource, 13)
assert_equal([rid2, rid3, rid].sort, [rid, rid2, rid3])
end

def test_sorting_by_id_secondarily
rid = JSONAPI::ResourceIdentity.new(PostResource, 12)
rid2 = JSONAPI::ResourceIdentity.new(PostResource, 13)
rid3 = JSONAPI::ResourceIdentity.new(PostResource, 14)

assert_equal([rid2, rid3, rid].sort, [rid, rid2, rid3])
end

def test_to_s
rid = JSONAPI::ResourceIdentity.new(PostResource, 12)
assert_equal(rid.to_s, 'PostResource:12')
end

def test_comparisons_return_nil_for_non_resource_identity
rid = JSONAPI::ResourceIdentity.new(PostResource, 13)
rid2 = "PostResource:13"
assert_nil(rid <=> rid2)
end

def test_comparisons_allocate_no_new_memory
rid = JSONAPI::ResourceIdentity.new(PostResource, 13)
rid2 = JSONAPI::ResourceIdentity.new(PostResource, 13)
allocation_report = MemoryProfiler.report do
rid == rid2
end
assert_equal 0, allocation_report.total_allocated
end
end