Skip to content

Auto-inject LIMIT 1 in bob.One#673

Open
jacobmolby wants to merge 4 commits into
stephenafamo:mainfrom
jacobmolby:apply-limit-in-bob.one
Open

Auto-inject LIMIT 1 in bob.One#673
jacobmolby wants to merge 4 commits into
stephenafamo:mainfrom
jacobmolby:apply-limit-in-bob.one

Conversation

@jacobmolby

Copy link
Copy Markdown
Contributor

Closes #579.

bob.One (and the generated .One() methods that route through it) did not add a LIMIT clause to the underlying query.

Unless the caller explicitly applied an sm.Limit(1) mod, the database would return every matching row and scan.One would silently discard all but the first — a quiet performance footgun on tables of any meaningful size.

Changes

  • bob.One now calls SetLimitIfUnset(1) on the query before executing. If the query already has a row-limit configured, it is left alone.
  • bob.First is added as the escape hatch: same semantics as the previous bob.One (no auto-limit). Use it when you genuinely want an unlimited query and only the first row scanned.
  • bob.Limiter interface introduced; BaseQuery forwards to its embedded expression, and each dialect's SelectQuery implements SetLimitIfUnset — picking Limit/Fetch for plain selects and CombinedLimit/CombinedFetch for UNION/INTERSECT/EXCEPT queries so the limit lands on the outer result.

Scope

  • SELECT queries only. UPDATE/DELETE/INSERT (with or without RETURNING) are unaffected.
  • Prepared statements (bob.QueryStmt.One) are unchanged — the SQL is baked at Prepare time.

Migration

Most callers benefit automatically. Anyone relying on the previous unlimited-query behavior of One should switch those call sites to bob.First.

@stephenafamo

Copy link
Copy Markdown
Owner

This is an elegant solution... however, this means that running bob.One would modify the query which could lead to unexpected results.

@jacobmolby

Copy link
Copy Markdown
Contributor Author

That is true.

However even though I cannot think of a reason for not to include a LIMIT 1 on a single scan, we still have the bob.First() method with old semantics. It will allow users to opt out of "modifying" the query behind the scenes.

I think this is a better default.

@stephenafamo

Copy link
Copy Markdown
Owner

I'll have to give this some thought.

Generally, I prefer to avoid any unexpected behaviour. And while querying the full result set to retrieve just one is unexpected, it is a performance issue, not a "correctness" issue.
On the other hand, modifying the query could potentially lead to unexpected incorrect behaviour.

Rather than merge this as-is, I'll have to first consider making queries immutable. There are a couple PRs for this, but that is a big change since everyone would have to update their use of the queries.

@jacobmolby

Copy link
Copy Markdown
Contributor Author

I generally agree, but the thing is with this case that a lot of users, including myself at some point, thought that the old behaviour was unexpected. Since the majority of ORMs automatically apply a limit on single row retrieval it's what the regular user (and an AI coding assistant) will expect.

Given there is plenty of opt-out with the bob.First(), sm.Limit(10000000) or using a mysql.RawQuery, I don't see it as too big of an issue.

I think the increased expectedness of how the implementation works, outweighs the concern of not altering the query behind the scene, ultimately providing a better developer experience.

@stephenafamo

Copy link
Copy Markdown
Owner

While I agree, my concern is about the effect of "misuse"

  • In the current code, misuse is a performance issue, but everything works correctly
  • If LIMIT is altered, then misuse leads to correctness bugs

@jay-babu

jay-babu commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

i would prefer to avoid this. my expectation for using .One would be that there could only be one result returned and if there were two, my view of the world is not true. we have solved a few bugs because of this in the past

@stephenafamo

Copy link
Copy Markdown
Owner

I understand.
I'll find some time to review the PR to make queries immutable (#657).

As much as I like the idea of One adding LIMIT 1 I really do not want to unexpectedly modify queries so this will ONLY be merged after the immutable query PR

@PudottaPommin

Copy link
Copy Markdown
Contributor

EntityFramework Core has this done pretty nicely.

Single - returns 1 row or error when no row or more than 1 ( using LIMIT 2 )
First - returns 1st row ( using LIMIT 1 )

So adding First with LIMIT 1 looks as good solution for anyone who wants something like this.

@stephenafamo

Copy link
Copy Markdown
Owner

returns 1 row or error when no row or more than 1

Oh!!! This could be a good direction in the meantime, to return an error if more than one row is returned.
Hopefully this catches any misuse

What do you think @jacobmolby @jay-babu

@jacobmolby

Copy link
Copy Markdown
Contributor Author

I think the confusion here really comes from the original .One not applying a limit in the first place. So a Single method (or sole, as Laravel calls it) is fine, but it doesn't address that mismatch. It's more of new feature, which is fine, but it's different (albeit related) topic.

It's worth noting that EF Core, which is the reference point here, has no single-retrieval method that skips the limit like bob does. First uses LIMIT 1 and Single uses LIMIT 2 (so it can error on more than one row). Both cap the result set. The unlimited-single-row behavior we currently have in .One isn't really something the EF model offers as an option.

I think its semantically reversed to keep .One as-is and making the proposed .First the limited variant. .One is the name that implies you expect a single row, so it should be the one applying the limit. Having .First apply LIMIT 1 while .One returns the full result set and just scans the first row inverts what the two names communicate IMO.

@stephenafamo

Copy link
Copy Markdown
Owner

Okay. thanks for that. Here is my tentative plan:

  • When Go 1.27 releases, do a big refactor and make use of generic methods where possible.
  • Use that refactor to make immutable queries the only path
  • Add LIMIT 1 to One() queries.

Unfortunately, this means that it will be months before this can be properly addressed, but I am not at all comfortable rewriting queries underneath users.

The alternative would be to fully clone the query before adding LIMIT 1 which would be very inefficient, but perhaps usable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.One() does not add a limit - resulting in unexpectedly large queries

4 participants