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

Can we have a find_by, returning nil when not found #937

Closed
fidalgo opened this issue Mar 19, 2020 · 13 comments
Closed

Can we have a find_by, returning nil when not found #937

fidalgo opened this issue Mar 19, 2020 · 13 comments
Labels

Comments

@fidalgo
Copy link

fidalgo commented Mar 19, 2020

Instead of having a find that will throw an exception, it would help to have a find_by method, that would have the same behaviour as find_by in rails, where it returns the first match or nil?

@stale
Copy link

stale bot commented Apr 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 30, 2020
@parndt
Copy link
Collaborator

parndt commented May 7, 2020

@fidalgo does find_by(slug: params[:id]) work?

@stale stale bot removed the stale label May 7, 2020
@fidalgo
Copy link
Author

fidalgo commented May 8, 2020

@parndt
find_by(slug: id) always return nil, when the record exists or not.

I'm using Model.friendly.find_by

@parndt
Copy link
Collaborator

parndt commented May 10, 2020

Does the below work, without the friendly filter applied?

Model.find_by(slug: params[:id])

My reasoning is that slug should just be a column on your database table so bypassing this library should give the behaviour that you're after. I tried this with an app that I have running friendly_id and it works as expected.

@stale
Copy link

stale bot commented Jun 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 21, 2020
@fidalgo
Copy link
Author

fidalgo commented Jun 22, 2020

I'm sorry, @parndt but I guess we're having a communication issue here.

With friendly_id gem, I can use the method find with either slug or id, but then the record does not exist, it will raise a record not found an exception.

So my request is to add another method that will mimic the find_by in Rails https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html#method-i-find_by

so instead of raising an exception is will return nil.

When I run Model.find_by(slug: params[:id]) it works as expected, otherwise, it would be a bug on Rails, what I would like would be to be able to use Model.friendly.find_by(slug: params[:id]) that would return the record or nil.

Please let me know if I'm missing something!

@stale stale bot removed the stale label Jun 22, 2020
@parndt
Copy link
Collaborator

parndt commented Jun 22, 2020

@fidalgo understood, I think. I'm mostly wondering why we need to add that when Rails does it already? 😄 it seems like FriendlyId doesn't need a method for doing what Rails does already. That is why we put our functionality behind .friendly so that you still have Rails' methods available.

@stale
Copy link

stale bot commented Aug 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 4, 2020
@stale stale bot closed this as completed Aug 11, 2020
@joemsak
Copy link
Contributor

joemsak commented Jun 17, 2022

I found this issue (sup, @parndt how you been) looking for something similar:

For me the idea would be that you don't know if you have primary key ID or slug friendly ID and still want a way to use them interchangeably and just get a nil instead of a raised error:

MyModel.friendly.find_safe(id_or_slug) 
# not saying the method has to be called `find_safe()` but hopefully you get the idea

Does that make sense?

@parndt
Copy link
Collaborator

parndt commented Jun 28, 2022

@joemsak I think the idea does make sense in principle, and we could probably support it using something like MyModel.friendly.find(id_or_slug, raise: false)

Then this implementation

def find(*args)
id = args.first
return super if args.count != 1 || id.unfriendly_id?
first_by_friendly_id(id).tap { |result| return result unless result.nil? }
return super if potential_primary_key?(id)
raise_not_found_exception(id)
end
could change to raise_not_found_exception(id) if raise.

We would need to formalise the args though away from def find(*args) to def find(id, raise_on_missing: true) or something. I'm not immediately sure what the implication of this would be. I'm not sure what other args find might be expecting.

Super nice to hear from you too, Joe! ❤️

@joemsak
Copy link
Contributor

joemsak commented Jun 29, 2022

Yeah, to be sure, my present needs for it almost certainly result from deeper code design issues where the id_or_slug should not be nil in the first place, but it does also seem nice to have a friendly "find by" behavior where you expect a nil when the ID is nil or the record isn't found

@joemsak
Copy link
Contributor

joemsak commented Jul 2, 2022

BTW @parndt if you are interested in this idea, would you like me to try a go at a PR?

@parndt
Copy link
Collaborator

parndt commented Jul 3, 2022

yes please, I think it'd be good to see if it's feasible, and the idea makes sense. 😄

parndt pushed a commit that referenced this issue Jul 14, 2022
This PR adds an optional `allow_nil: true` option to the finder

From discussion in #937 the idea is to add functionality that behaves *like* Rails' `find_by` which returns `nil` when a record is not found. 

This is useful in conditions where the developer is allowing the primary key ID and the friendly slug ID but is not sure if the record will actually be found, and wants a nil instead of a raised exception.

### Usage

```ruby
MyModel.friendly.find("friendly-slug")
# => still works as expected, raises an ActiveRecord::RecordNotFound exception

MyModel.friendly.find("friendly-slug", allow_nil: true)
# => same as above, but some devs may find using this option to be more self-documenting of their intentions
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants