runSelectReturningOne should not return Nothing for more than one row

[moll]

As asked on IRC, creating an issue for discussion.

runSelectReturningOne returns Nothing when more than one row matches the query. #268 states this is by design.

I claim that returning Nothing when more than one row is returned is a risky choice. It's correct to state the row ordering may not be defined and non-determinism may result, but returning Nothing signifies there are no rows that match the given constraints. That's likely to lead code to attempt to create a row and then fail either with a database constraint failure, or to further exacerbate the duplicate row problem. Secondly, a lot of other ORMs and database layers do not behave like this. Be it an OO .first or a functional getFirst, they often tack on a LIMIT 1 and get on with it. runSelectReturningOne, while not modifying the query to optimize with a LIMIT 1, shouldn't at least behave in an unexpecting manner. I, for one, leaned towards runSelectReturningOne to not have to do listToMaybe, and didn't think twice whether there are or aren't duplicate rows.

Cheers.

[mulderr]

Semantics of runSelectReturningOne is that you expect exactly one row. Multiple rows in that case is equivalent to no rows - something unexpected, something very wrong with your assumptions. That is useful in it's own right and it did save me a couple times. I caught the error early which would not have happened had runSelectReturningOne silently done a limit 1.

What you describe I would rather call runSelectReturningFirst and you can easily get it combining runSelectReturningList (probably together with limit_ 1) and listToMaybe or so - as you observe yourself.

The reason there isn't something like runSelectReturningFirst in beam already is I guess exactly what you indicated above. It's not a very good primitive to begin with and could be "unstable" if not properly handled.

[moll]

I alluded to above that the returned Nothing also has a meaning — a meaning of "nothing matched", not something out of the ordinary happened. If it's meant to say "something unexpected" happened, that should be encoded in the type system through an Either MoreRowsError (Maybe row). I personally don't care to protect myself from more rows and will choose the get-one-and-ignore-the-rest choice. As for naming, I believe you, @mulderr, intuitively differentiate between *First and *One, but I suspect very few others share that intuition. :P As with web pages, we spend most of our time on other APIs, and all of those tend to behave as I described above — .first-likes gets the first row and ignores the rest.

[3noch]

I can sympathize with both positions here. To be honest, I think runSelectReturningOne is a bit opinionated for such an all purpose querying DSL, especially since it's not at all hard to build yourself. I would be in favor of either a) making the type signature far more obvious about how it's opinionated (as @moll suggests) or, b) just deprecating it entirely and relying on people to write their own basic combinators.

[mulderr]

Oh I didn't intuitively know what the function does from the name and the type signature alone. It's certainly nice when you can but it's hardly a precedent when you can't.

As for people coming from other libraries with preconceived notions and finding it confusing, I cannot really argue with that. Likewise I agree it may be a bit opinionated but Either MoreRowsError (Maybe row) seems a bit much. Would Either RowCountError row also work? (RowCountError = NoRowsError | MoreRowsError).

[3noch]

@mulderr Yes I think that would be better.

[tathougies]

Perhaps we could move it to a separate 'utility' module, and then make it return a more obvious error. I like the Either MoreRowsError row idea. Thoughts?

[3noch]

I like it.

[mulderr]

Actually, I like the idea of having two versions that also guarantee not to make the database work overtime by using limit internally. I think currently this is left to the user?

runSelectReturingOne :: ... (Either SomeError row)

Runs the query with limit_ 2, returns Right only if exactly one row was returned.

runSelectReturningFirst :: ... (Maybe row)

Runs the query with limit_ 1, returns Just if anything was returned.

[alexfmpe]

Secondly, a lot of other ORMs and database layers do not behave like this. Be it an OO .first or a functional getFirst, they often tack on a LIMIT 1 and get on with it.

Yop - my brain always defaults to the runSelectReturningAtMostOne meaning, and it has lead to some head scratching more than once. Just spent half an hour with a colleague trying to figure out what was wrong with a filter_ only to remember it was runSelectReturningExactlyOne.

I do like the Either SomeError row return, but also think the current name is way too ambiguous to expose to users. Note even the tutorial claims "at most one", not "the only one"

We also use the runSelectReturningOne function to get at most one
record from the database.

[thomasjm]

I used Beam for a long time before noticing this, and was pretty surprised by the actual behavior of runSelectReturningOne. I think the current behavior is pretty undesirable because it forces me to think about whether multiple results are possible every time I use it (whether a query is on a primary key or an aggregation or something like that, often relying on reasoning that is in the database schema and not in the nearby code).

It turns out adding the runSelectReturningFirst function with the semantics I'd expect is pretty easy, so I opened a PR in #630. What do you think?

[kmicklas]

My current plan for this is to add the "first" version with #630 and in the next major release rename runSelectReturningOne to runSelectReturningUnique as suggested by @alexfmpe. We can keep the old name as a deprecated synonym for compatibility.