find_by(nil) returns first record instead of nil

[robertoplancarte]

On any Model find_by(nil) will return the first record or nil if there are no records. find_by(nil) should always return nil. On the other hand find_by!(nil) does raise ActiveRecord::RecordNotFound as it should.

[estsauver]

Hey @robertoplancarte,

I'm not sure that's inherently true. find_by is a straight alias to where(args*).take, so really the question your asking is what should

where(nil)

do. I think if you treat where as an operation on a set of results, the noop for that operation is to return the same operation.

If nothing else, I think changing this would pretty substantially change the API. Is there a use case that this makes a lot more sense in?

[robertoplancarte]

It was just odd, I was expecting it to generate a query with "WHERE NULL" at the end. I dind't know find_by and find_by! were just an alias to where(args_).take and where(args_)take.!
I just went through the source code for where and see the blank condition comment you are talking about, sorry I didn't look this up before creating the issue.

Thanks.

[laurocaetano]

I guess we can close, right?

@estsauver thanks for explaining :)

[sealocal]

This topic confused me as well. I would prefer that find_by(nil) returned an error, rather than a nearly-random record.

I can see how a "noop" for a where method would return the original result set. In the case of a find_by, the "noop" isn't actually returning result set, it's returning a single record from the original result set. To me, that doesn't really make sense.

It would change the API, but what scenario would find_by(nil) be reliably used with its current implementation? I know, we can't say for sure how many apps are dependent on it, but I would think it's very few to none.

Of course, this is just my opinion. I'm curious what other think if anyone else visits and wants to join the conversation.

[mfpiccolo]

I just ran into this and agree that it is very confusing that find_by(nil) returns a record. This does not seem like intuitive behavior here.

[sealocal]

Is it too late to change find_by(nil) in Rails 5.0.0?

: D

[sealocal]

On the other hand, if you view find_by's hash of options parameter as the conditions for the where clause in your SQL query, then find_by is simply returning the first record of your unfiltered model.

So, changing your perspective makes this not so confusing and more intuitive.

[EdDeAlmeidaJr]

I agree. It makes no sense to find a certain record, whatever it is, based on no condition at all. I already pointed a semantically inconsistent construction in this issue and now it seems there is another.

[mfpiccolo]

The argument that find_by being a alias for where(args*).take should change my perspective seems a bit weak. That is essentially an implementation detail from the perspective of the API. I would rather the method do what makes semantic sense than follow the where implementation.

I am also not sure the where(nil) returning a collection makes sense to me either but that is a different topic.

[sealocal]

I'm not arguing that a the API should change someone's perspective, but I'm saying that I could see it interpreted both ways.

I think it's clear that the where method is intended to mirror the SQL where clause as much as possible - AR is an ORM after all.

I think there are only two options to interpret how the where method should be implemented with a nil argument:

1. where(nil) maps to a SQL query that has no where clause
2. where(nil) maps to a SQL query that has a where clause without conditions

As far as I know, SQL will always error if the query's where clause has no conditions.

Would it make more sense to return a SQL syntax error when the argument is nil? If so, then perhaps where({}) and find_by({}) could return an empty collection and nil, respectively?