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.

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

Make find_signed and find_signed! consistent #39542

Closed
wants to merge 1 commit into from

Conversation

santib
Copy link
Contributor

@santib santib commented Jun 5, 2020

Summary

find_signed allows finding by a custom primary key, but find_signed! doesn't allow it. I think there should be consistency between them, so I added support for custom primary keys in find_signed!

if id = signed_id_verifier.verify(signed_id, purpose: combine_signed_id_purposes(purpose))
find(id)
find_by! primary_key => id
Copy link
Member

Choose a reason for hiding this comment

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

Originally find supports custom primary key.

def find_with_ids(*ids)
raise UnknownPrimaryKey.new(@klass) if primary_key.nil?

def find_one(id)
if ActiveRecord::Base === id
raise ArgumentError, <<-MSG.squish
You are passing an instance of ActiveRecord::Base to `find`.
Please pass the id of the object by calling `.id`.
MSG
end
relation = where(primary_key => id)
record = relation.take

Does find not work as expected?

Copy link
Contributor Author

@santib santib Jun 5, 2020

Choose a reason for hiding this comment

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

Sorry @kamipo my bad, I was around the signed id code and the difference in the methods implementation caught my attention. Now that you point me to the ActiveRecord's find code I think this PR can be closed. Thanks

@santib santib closed this Jun 5, 2020
@santib santib deleted the make-find-signed-consistent branch June 5, 2020 01:50
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants