Skip to content

fix: five small ORM fixes (limit, having, restore, find, select doc) - #35

Merged
necessarylion merged 6 commits into
mainfrom
fix/orm-small-fixes
Nov 2, 2026
Merged

necessarylion merged 6 commits into
mainfrom
fix/orm-small-fixes

Conversation

@mydoxie

@mydoxie mydoxie commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Five small ORM fixes. One commit per item.

  1. Negative limit. limit(-1) sent LIMIT -1. Postgres gives an error. Now a negative limit() or take() is ignored, like Laravel.
  2. having() with null. having('x', '!=', null) built x = '!='. Now it builds x IS NOT NULL, and = null builds x IS NULL. It uses the same missing-arg marker and null check as where().
  3. Query restore(). Blog().withTrash().where(...).restore() ran WHERE id IS NULL. It changed nothing and returned true. Now it sets deleted_at to null on the rows of the query, and returns true only when a row changed. With no where() it throws a StateError, like delete(). restore() on a loaded model is not changed.
  4. find('email', null). It searched id = 'email'. Now a private "not given" marker tells no arg from null, so it finds email IS NULL. find(5) is not changed. findOrFail() gets the same fix.
  5. select() doc. The comment said a second call adds columns. It replaces them, like Laravel. Only the comment changed.

Changelog lines are under ## Unreleased. No version bump.

Tests

  • New tests for items 1 to 4 in test/bug_fixes_test.dart. They fail without the fix.
  • DRIVER=sqlite dart test in packages/dox-orm: all pass.
  • test/bug_fixes_test.dart on Postgres: pass.
  • dart format and dart analyze --fatal-infos packages: clean.

limit(-1) sent LIMIT -1, which is an error on Postgres. Now a negative value is ignored, like Laravel.
having('x', '!=', null) built x = '!=', because a null third arg was read as no arg. It now uses the same missing-arg marker and null check as where().
Blog().withTrash().where(...).restore() ran WHERE id IS NULL, changed nothing and returned true. Now it sets deleted_at to null on the rows of the query. With no where() it throws a StateError, like delete(). restore() on a loaded model is not changed.
A null second arg meant id only, so find('email', null) searched id = 'email'. Now a private missing-arg marker tells no arg from null. find(5) is not changed. findOrFail() gets the same fix.
The comment said a second select() adds columns. It replaces them, like Laravel. Only the comment changed.
@mydoxie

mydoxie commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Bot review

Verdict: changes requested. The code is good. One thing must be fixed: the changelog.

Blocking

  1. The changelog has the same lines many times. packages/dox-orm/CHANGELOG.md lines 3-17 add 15 lines, but only 5 are different. The limit line is there 4 times, the having line 4 times, the restore line 3 times, and the find line 2 times. This text goes into the release notes. Fix: keep one line per item (5 lines in all, with the select() docs line).

What I checked (all fine)

  • limit / take: a negative value is now ignored, and take() calls limit(). paginate() and chunk() still check their size first, so they do not lose the limit.
  • having(): the third arg now uses the same "missing" marker as where(). having('x', null) and having('x', '=', null) build IS NULL. '!=' and '<>' build IS NOT NULL. Other operators with null throw, like where(). A null value adds no binding.
  • find() / findOrFail(): find(5) and find('slug', 'x') work as before. find('slug', null) now finds slug IS NULL. ModelNotFoundException gets the right column and value.
  • Query restore(): _needKeyOrWhere runs before withTrash(), so Blog().restore() with no where throws a StateError, like delete(). onlyTrashed() still counts as a where and keeps deleted_at IS NOT NULL. Restore on a loaded model works as before.
  • missingArg and nullCheck are not exported. No version bump. The changelog lines are under ## Unreleased.
  • Tests: DRIVER=sqlite dart test passes (528). The full suite passes on Postgres (524). bug_fixes_test.dart passes on MySQL 8.4 too. dart analyze --fatal-infos and dart format are clean. CI is green.

Nits (no change needed)

  • Query restore() returns true when the query matches any row, also a row that is not deleted. Postgres and SQLite count matched rows. MySQL counts only changed rows. So Blog().where('status', 'new').restore() on rows that are not deleted gives true on Postgres and SQLite, but false on MySQL. Laravel works the same way. Adding whereNotNull('deleted_at') would make it the same on all three, if you want that.
  • find(5, null) with a null second arg written out now means "column 5 is null" and throws a type error. Before, it looked up id 5. I found no callers like this in the Dox repos. having('x', 5, null) changes the same way, like where() already does.
  • A loaded model with no key value (read without the id column) now throws on restore(). Before, it ran WHERE id IS NULL and returned true. The new way is better, and delete() works the same.

Each Unreleased line of this PR was added more than once. Keep one line per item.
@mydoxie

mydoxie commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Fixer reply

  • Fixed the duplicated changelog lines in eb229f7. My commit split script added the lines again in each commit.
  • ## Unreleased now has exactly 5 new lines, one per item, including the select() docs line.
  • Nothing else changed.

@mydoxie

mydoxie commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Bot review

Last commit eb229f7 only changes packages/dox-orm/CHANGELOG.md. It removes 10 duplicated lines.

  • The PR diff now adds exactly 5 lines under ## Unreleased: negative limit, having() with null, query restore(), find(column, null), select() doc.
  • No duplicates. No existing changelog line is removed.
  • No blocking issue.
  • CI: lint passed. test was still running when I checked. The last commit is a docs-only change, so it should not change the result. Do not merge until test is green.

Verdict: OK.

@necessarylion
necessarylion merged commit b9a17fb into main Nov 2, 2026
2 checks passed
@necessarylion
necessarylion deleted the fix/orm-small-fixes branch November 2, 2026 05:25
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.

2 participants