Skip to content

fix: compare against a null parameter with IS NULL - #152

Merged
gmpassos merged 7 commits into
masterfrom
fix/null-parameter-conditions
Aug 12, 2026
Merged

fix: compare against a null parameter with IS NULL#152
gmpassos merged 7 commits into
masterfrom
fix/null-parameter-conditions

Conversation

@gmpassos

Copy link
Copy Markdown
Contributor

The bug

A condition comparing a field to null passed as a parameter was encoded as field = ? bound to null. = NULL is never true in SQL, so the query returned no rows instead of the rows whose column is null.

repository.selectByQuery(' state == ? && active == ? ',
    parameters: {'state': null, 'active': true});
SELECT "ge".* FROM "geo_locality" AS "ge"
 WHERE ( "ge"."state" = ? AND "ge"."active" = ? )   -- [null, true]

The rows were there — the same predicate written by hand returns them:

sqlite> select count(*) from geo_locality where state is null and active = 1;
6

Why it slipped through

ConditionSQLEncoder did convert =/IN against null into IS NULL, and !=/NOT IN into IS NOT NULL. But the check was on the encoded text:

if (valueSQL == 'null') { ... }

That is only ever true when the null is written straight into the statement. A null arriving as a parameter is encoded as an EncodingPlaceholder — its text is @key, and the value itself lives in the encoding context — so the conversion was skipped. Entity queries take the parameter form, which is why this surfaces there and not in hand-written SQL.

The encoder is shared, so this affected SQLite, PostgreSQL and MySQL alike, and any compound condition containing such a term: a matching half cannot rescue a half that never matches.

The fix

encodeConditionValuesWithOperator now inspects the encoding value rather than its text — EncodingValueNull as before, plus an EncodingPlaceholder whose value in the context is null. Neither writes the placeholder, so the statement carries no parameter for it and the positional binding stays aligned.

Testing

The regression test is in the shared adapter suite, so all three SQL adapters exercise it. It covers the plain form, the compound form that made this visible, and the negation that must become IS NOT NULL.

  • Fix in place: bones_api_entity_db_sqlite_test.dart91 passing
  • Fix reverted, test kept: 14 failing
  • SQL-memory + condition + SQLite suites together: 213 passing
  • dart analyze: clean

Found while pointing a real application at the new SQLite adapter: its country and category pickers came back empty, because both are parent == null / state == null queries, which blocked onboarding entirely.

gmpassos and others added 6 commits August 12, 2026 05:31
A condition comparing a field to null passed as a *parameter* was encoded
as `field = ?` bound to null. `= NULL` is never true in SQL, so the query
returned no rows rather than the rows whose column is null.

`ConditionSQLEncoder` already converted `=`/`IN` against null into
`IS NULL`, and `!=`/`NOT IN` into `IS NOT NULL` — but only when the null
was written straight into the statement. A null arriving as a parameter is
encoded as a placeholder, whose text never equals 'null', so the
conversion was skipped. Entity queries take the parameter form, which is
where this shows up:

    selectByQuery(' state == ? && active == ? ',
        parameters: {'state': null, 'active': true});

    SELECT ... WHERE ( "state" = ? AND "active" = ? )   -- [null, true]

The encoder is shared, so this affected SQLite, PostgreSQL and MySQL
alike, and any compound condition containing such a term — a matching
half cannot rescue a half that never matches.

`encodeConditionValuesWithOperator` now inspects the encoding value rather
than its text: `EncodingValueNull` as before, and an `EncodingPlaceholder`
whose value in the context is null. Neither writes the placeholder, so the
statement carries no parameter for it.

The regression test lives in the shared adapter suite, so all three
adapters exercise it. Reverting the fix with the test in place fails 14
of the SQLite suite's cases; with it, all 91 pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`bones_api_test.dart` asserts the two agree, and the 1.15.1 bump missed
the constant.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dart format` changed how it lays out argument lists, so
`test/bones_api_test.reflection.g.dart` no longer matched its own
formatting and `--set-exit-if-changed` failed CI. The generated content is
unchanged; only the layout moved.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dart analyze --fatal-infos --fatal-warnings` reports seven issues under
3.13 that 3.12 did not. None are new code; the analyser got stricter.

Three warnings in `APIRoot`:

- Two `invalid_return_type_for_then`. `Future.then`'s `onError` takes an
  untyped `(e, s)` closure, so there is no return context to infer `T`
  from and the helpers called inside resolved to `dynamic` — not
  assignable to what `then` expects. The type arguments are now explicit.
- One `unawaited_return_in_try_block`, on a `FutureOr` the analyser cannot
  prove is not a future. Awaiting it is also the more correct form: a
  future returned bare from inside that `try` would carry its error past
  the `catch` below rather than into it.

Four `use_super_parameters`, in the example and the Docker test configs.
`SQLiteTestConfig` needed the explicit `memory:` argument dropped as well,
since a super parameter forwards on its own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewriting `field == ?` to `field IS NULL` leaves the parameter resolved
but no longer mentioned by the statement. PostgreSQL rejects a statement
carrying variables it does not use ("Contains superfluous variables"), so
the entry has to be removed once the condition is encoded.

Prunes against the encoded condition output, which is the only place
placeholders are ever written, so a key still used by another operator
(`field > ?` with a null bound) keeps its binding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
reflection_factory 2.8.1 pins `dart_style` to the formatter bundled with
Dart 3.12. On Dart 3.13 the generated `*.reflection.g.dart` therefore no
longer matches this package's own `dart format`, and the two CI checks
become mutually exclusive: committing the generator output fails
`dart format --set-exit-if-changed`, and committing the formatted output
fails `test/ensure_build_test.dart`, because `build_runner` rewrites it
back. That is the `ensure_build` failure on this branch — pre-existing and
unrelated to the null-condition fix; it blocks any PR built on 3.13.

reflection_factory 2.9.0 (gmpassos/reflection_factory#52) tracks the
formatter the SDK ships. Verified against it via a path override: the only
change to the generated files is the builder version stamp, the formatting
committed here is already correct, and `dart format`, `dart analyze` and
`ensure_build` all pass together.

`environment.sdk` deliberately stays at `>=3.10.0`: Dart 3.13 rejects
`final` on parameters, which this package uses, and a package's language
version comes from its own pubspec — only the toolchain needs to be 3.13.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gmpassos

Copy link
Copy Markdown
Contributor Author

Root-caused the remaining CI failures — both were mine, both now fixed

With Docker available I could finally run the PostgreSQL/MySQL suites locally, which settled attribution:

master this branch (before) this branch (now)
PostgreSQL 65/65 55/65 65/65
MySQL 65/65

So the 10 PostgreSQL failures were caused by this PR, not by the Dart 3.13 bump.

What broke. The fix rewrites field == ? bound to null into field IS NULL — correct SQL, but it leaves the parameter resolved and no longer mentioned by the statement:

SQL<< SELECT "us".* FROM "user" as "us" WHERE ( "us"."level" IS NULL ) >>( {"level":null} )
  -- [ArgumentError] Invalid argument (parameters): Contains superfluous variables: level

PostgreSQL rejects that; SQLite and the memory adapter tolerate it, which is why only postgres went red. And it was the new regression test itself that tripped it, inside the large TestEntityRepositoryProvider test — aborting it before it stored address 11001, which is why populate then failed further down with Error setting User field: address = null. One root cause, ten reported failures.

Fix (955134a): prune placeholders the encoded condition never references. It prunes against the condition output — the only place placeholders are ever written — so a key still used by another operator (field > ? bound to null) keeps its binding.

The ensure_build failure is not from this PR

reflection_factory 2.8.1 pins dart_style to the formatter bundled with Dart 3.12. Dart 3.13 bundles dart_style 3.1.10, so the generated *.reflection.g.dart no longer matches this package's own dart format, and two CI checks become mutually exclusive: committing the generator output fails dart format --set-exit-if-changed, committing the formatted output fails ensure_build, because build_runner rewrites it back. This blocks any PR built on Dart 3.13.

Fixed upstream in gmpassos/reflection_factory#52, and c84c3d7 here bumps to ^2.9.0. Verified against that branch via a path override: the only change to the generated files is the builder version stamp — the formatting already committed here is correct — and dart format, dart analyze --fatal-infos --fatal-warnings and ensure_build then pass together.

Important

CI stays red until reflection_factory 2.9.0 is published, since pub get cannot resolve ^2.9.0 yet. Order: merge + publish reflection_factory#52, then re-run CI here.

environment.sdk deliberately stays at >=3.10.0: Dart 3.13 rejects final on parameters, which this package uses in several places, and a package's language version comes from its own pubspec — only the toolchain needs to be 3.13.

Local verification on this branch

  • PostgreSQL 65/65, MySQL 65/65 (Docker)
  • Full VM suite: 914 passing, ensure_build the only failure, and it clears under the path override

gmpassos added a commit to gmpassos/reflection_factory that referenced this pull request Aug 12, 2026
Tracks the formatter the Dart SDK ships, so generated code and `dart format` agree again on Dart 3.13. Unblocks Colossus-Services/bones_api#152.
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.25%. Comparing base (82751af) to head (c84c3d7).

Files with missing lines Patch % Lines
lib/src/bones_api_base.dart 71.42% 2 Missing ⚠️
lib/src/bones_api_condition_encoder.dart 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   68.17%   68.25%   +0.08%     
==========================================
  Files          66       66              
  Lines       22137    22147      +10     
==========================================
+ Hits        15091    15116      +25     
+ Misses       7046     7031      -15     
Flag Coverage Δ
unittests 68.25% <91.17%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmpassos
gmpassos merged commit c05f56e into master Aug 12, 2026
5 of 8 checks passed
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.

1 participant