fix: compare against a null parameter with IS NULL

CHANGELOG.md

@@ -1,3 +1,41 @@
+## 1.15.1
+
+- Fixed: 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.
+
+  `ConditionSQLEncoder` did turn `=`/`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 why it surfaced there:
+
+  ```dart
+  // Returned [] with matching rows present; now returns the rows whose
+  // `state` is null.
+  repository.selectByQuery(' state == ? && active == ? ',
+      parameters: {'state': null, 'active': true});
+  ```
+
+  The encoder is shared, so this affected every SQL adapter — SQLite,
+  PostgreSQL and MySQL alike — and any condition compared against a null
+  parameter, including compound ones whose other terms matched.
+
+  Rewriting the comparison also leaves the parameter unmentioned by the
+  statement, so it is now dropped once the condition is encoded: PostgreSQL
+  rejects a statement carrying variables it does not use. A placeholder still
+  referenced by another operator (`field > ?` bound to null) keeps its binding.
+
+  Covered now by the shared adapter test suite, so all three adapters exercise
+  it.
+
+- `reflection_factory`: `^2.8.1` → `^2.9.0`.
+  - 2.8.1 pinned `dart_style` to the formatter bundled with Dart 3.12, so on
+    Dart 3.13 the generated `*.reflection.g.dart` no longer matched this
+    package's own `dart format`, making `dart format --set-exit-if-changed` and
+    `test/ensure_build_test.dart` mutually exclusive. 2.9.0 tracks the
+    formatter the SDK ships.
+
 ## 1.15.0
 
 - Faster request dispatch. A logged route call is **~2.9x** faster

example/bones_api_example.dart

@@ -60,7 +60,7 @@ class MyBTCModule extends APIModule {
 
 /// The `APIRoot` defines the API version and modules to use:
 class MyAPI extends APIRoot {
-  MyAPI({dynamic apiConfig}) : super('example', '1.0', apiConfig: apiConfig);
+  MyAPI({super.apiConfig}) : super('example', '1.0');
 
   // Load the modules used by this API:
   @override

lib/src/bones_api_base.dart

@@ -48,7 +48,7 @@ typedef APILogger =
 /// Bones API Library class.
 class BonesAPI {
   // ignore: constant_identifier_names
-  static const String VERSION = '1.15.0';
+  static const String VERSION = '1.15.1';
 
   static bool _boot = false;
 
@@ -511,18 +511,22 @@ abstract class APIRoot with Initializable, Closable {
         if (response is APIResponse) {
           return response;
         } else if (response is Future<APIResponse<T>?>) {
-          return response.then(
+          // The type argument is explicit on both branches: `onError` takes an
+          // untyped `(e, s)` closure, so there is no return context to infer
+          // from and `_callHandlersAsync` resolved to `dynamic` — which is not
+          // assignable to what `then` expects here.
+          return response.then<APIResponse<T>?>(
             (resp) {
               if (resp != null) return resp;
-              return _callHandlersAsync(
+              return _callHandlersAsync<T>(
                 handlersIterator,
                 request,
                 handlersType,
               );
             },
             onError: (e, s) {
               _logCallHandlersError(handlersType, handler, e, s);
-              return _callHandlersAsync(
+              return _callHandlersAsync<T>(
                 handlersIterator,
                 request,
                 handlersType,
@@ -566,7 +570,11 @@ abstract class APIRoot with Initializable, Closable {
         if (response == null) continue;
 
         if (response is APIResponse) {
-          return response;
+          // Awaited rather than returned bare: the declared type is a
+          // `FutureOr`, so the analyzer cannot rule out a future here, and a
+          // future returned from inside this `try` would carry its error past
+          // the `catch` below instead of into it.
+          return await response;
         } else if (response is Future<APIResponse<T>?>) {
           var resp = await response;
           if (resp != null) {
@@ -647,12 +655,15 @@ abstract class APIRoot with Initializable, Closable {
         // to be rethrown by the previous `Zone`.
 
         if (response is Future<APIResponse<T>>) {
-          return response.then(
-            (response) => _callZonedReturn(callZone, request, response),
-            onError: (e, s) => _callZonedReturn(
+          // Explicit type arguments: `onError` takes an untyped `(e, s)`
+          // closure, so there is no return context to infer `T` from and both
+          // helpers resolved to `dynamic`, which `then` will not accept here.
+          return response.then<APIResponse<T>>(
+            (response) => _callZonedReturn<T>(callZone, request, response),
+            onError: (e, s) => _callZonedReturn<T>(
               callZone,
               request,
-              _resolveErrorAPIResponse(e, s),
+              _resolveErrorAPIResponse<T>(e, s),
             ),
           );
         } else {

lib/src/bones_api_condition_encoder.dart

@@ -1198,10 +1198,51 @@ abstract class ConditionEncoder {
         s.write(groupCloser);
       }
 
+      pruneUnusedParametersPlaceholders(context);
+
       return context;
     });
   }
 
+  /// Drops the placeholders that the encoded output never references.
+  ///
+  /// A null compared with `==`/`!=` is written as `IS NULL`/`IS NOT NULL`
+  /// rather than as the placeholder, which leaves its parameter resolved but
+  /// unmentioned by the statement. PostgreSQL rejects a statement carrying
+  /// variables it does not use, so the entry has to go.
+  void pruneUnusedParametersPlaceholders(EncodingContext context) {
+    var parametersPlaceholders = context.parametersPlaceholders;
+    if (parametersPlaceholders.isEmpty) return;
+
+    var output = context.outputString;
+
+    parametersPlaceholders.removeWhere(
+      (key, _) => !_isPlaceholderInOutput(output, parameterPlaceholder(key)),
+    );
+  }
+
+  static bool _isPlaceholderInOutput(String output, String placeholder) {
+    for (var i = output.indexOf(placeholder); i >= 0;) {
+      var end = i + placeholder.length;
+
+      // A longer placeholder that merely starts with this one is a different
+      // parameter: `@level` vs. `@level_0`, the indexed form used for lists.
+      if (end >= output.length || !_isPlaceholderChar(output.codeUnitAt(end))) {
+        return true;
+      }
+
+      i = output.indexOf(placeholder, i + 1);
+    }
+
+    return false;
+  }
+
+  static bool _isPlaceholderChar(int c) =>
+      (c >= 0x30 && c <= 0x39) || // 0-9
+      (c >= 0x41 && c <= 0x5a) || // A-Z
+      (c >= 0x61 && c <= 0x7a) || // a-z
+      c == 0x5f; // _
+
   FutureOr<EncodingContext> encodeCondition(
     Condition c,
     EncodingContext context,

lib/src/bones_api_condition_sql.dart

@@ -253,16 +253,19 @@ class ConditionSQLEncoder extends ConditionEncoder {
     context.write(tableKey);
     context.write(' ');
 
-    var valueSQLRet = valueToSQL(
+    // The encoding value rather than only its text: a null supplied as a
+    // parameter encodes to a placeholder, so the text alone cannot say whether
+    // the comparison is against null.
+    var valueRet = valueToParameterValue(
       context,
       values,
       fieldKey: fieldKey,
       fieldType: keyType,
       valueAsList: valueAsList,
     );
 
-    return valueSQLRet.resolveMapped((valueSQL) {
-      if (valueSQL == 'null') {
+    return valueRet.resolveMapped((value) {
+      if (isNullEncodingValue(value, context)) {
         switch (operator) {
           case '=':
           case 'IN':
@@ -281,12 +284,37 @@ class ConditionSQLEncoder extends ConditionEncoder {
 
       context.write(operator);
       context.write(' ');
-      context.write(valueSQL);
+      context.write(value.encode);
       context.write(' ');
       return context;
     });
   }
 
+  /// Whether [value] is a comparison against SQL `NULL`.
+  ///
+  /// A null written straight into the statement arrives as an
+  /// [EncodingValueNull], which is the case this encoder has always handled. A
+  /// null supplied as a *parameter* arrives as an [EncodingPlaceholder]: its
+  /// text is the placeholder and the value itself is held in the context, so
+  /// it has to be looked up there.
+  ///
+  /// Missing that produced `field = ?` bound to null. `= NULL` is never true
+  /// in SQL, so such a query returned nothing at all rather than the rows whose
+  /// column is null.
+  bool isNullEncodingValue(
+    EncodingValue<String, Object?> value,
+    EncodingContext context,
+  ) {
+    if (value is EncodingValueNull) return true;
+
+    if (value is EncodingPlaceholder) {
+      // Absent and present-but-null both mean the statement binds null.
+      return context.parametersPlaceholders[value.key] == null;
+    }
+
+    return false;
+  }
+
   FutureOr<MapEntry<Type, String>> keyToSQL(
     KeyCondition<dynamic, dynamic> c,
     EncodingContext context,

lib/src/bones_api_test_utils_mysql.dart

@@ -19,16 +19,11 @@ class APITestConfigDockerMySQL
   APITestConfigDockerMySQL(
     Map<String, dynamic> apiConfig, {
     DockerHost? dockerHost,
-    String? containerNamePrefix,
+    super.containerNamePrefix,
     this.forceNativePasswordAuthentication = true,
     this.version = 'latest',
     super.cleanContainer,
-  }) : super(
-         dockerHost ?? DockerHostLocal(),
-         'MySQL',
-         apiConfig,
-         containerNamePrefix: containerNamePrefix,
-       ) {
+  }) : super(dockerHost ?? DockerHostLocal(), 'MySQL', apiConfig) {
     DBMySQLAdapter.boot();
   }
 

lib/src/bones_api_test_utils_postgres.dart

@@ -26,18 +26,13 @@ class APITestConfigDockerPostgreSQL
   APITestConfigDockerPostgreSQL(
     Map<String, dynamic> apiConfig, {
     DockerHost? dockerHost,
-    String? containerNamePrefix,
+    super.containerNamePrefix,
     this.postgresPort,
     this.maxConnections,
     this.logStatement,
     this.version = 'latest',
     super.cleanContainer,
-  }) : super(
-         dockerHost ?? DockerHostLocal(),
-         'PostgreSQL',
-         apiConfig,
-         containerNamePrefix: containerNamePrefix,
-       ) {
+  }) : super(dockerHost ?? DockerHostLocal(), 'PostgreSQL', apiConfig) {
     DBPostgreSQLAdapter.boot();
   }
 

pubspec.yaml

@@ -1,6 +1,6 @@
 name: bones_api
 description: Bones_API - A powerful API backend framework for Dart. It comes with a built-in HTTP Server, route handler, entity handler, SQL translator, and DB adapters.
-version: 1.15.0
+version: 1.15.1
 homepage: https://github.com/Colossus-Services/bones_api
 
 environment:
@@ -12,7 +12,7 @@ executables:
 dependencies:
   async_extension: ^1.2.22
   async_events: ^1.3.0
-  reflection_factory: ^2.8.1
+  reflection_factory: ^2.9.0
   statistics: ^1.2.1
   swiss_knife: ^3.3.14
   data_serializer: ^1.2.1

test/bones_api_entity_db_sqlite_test.dart

@@ -16,7 +16,7 @@ class SQLiteTestConfig extends APITestConfigSQLite {
   SQLiteTestConfig({
     required bool generateTables,
     required bool checkTables,
-    required bool memory,
+    required super.memory,
   }) : super({
          'db': {
            'sqlite': {
@@ -26,7 +26,7 @@ class SQLiteTestConfig extends APITestConfigSQLite {
              'checkTables': checkTables,
            },
          },
-       }, memory: memory);
+       });
 }
 
 Future<void> main() async {

test/bones_api_entity_db_tests_base.dart

@@ -1384,6 +1384,52 @@ Future<bool> runAdapterTests(
             expect(user4?.id, equals(user3.id));
           }
 
+          {
+            // A null supplied as a parameter must become `IS NULL`, not `= ?`
+            // bound to null — which is never true in SQL, so the query
+            // silently returned nothing instead of the rows whose column is
+            // null. An inlined null was already handled; passing it as a
+            // parameter was not, and that is the form entity queries take.
+            var nullLevel = await userAPIRepository.selectByQuery(
+              " level == ? ",
+              parameters: {'level': null},
+            );
+
+            expect(
+              nullLevel,
+              isNotEmpty,
+              reason: 'there are users with no level',
+            );
+            expect(
+              nullLevel.map((e) => e.level),
+              everyElement(isNull),
+              reason: 'a null parameter must select the rows that are null',
+            );
+
+            // Compounded with another term — the shape that made this visible,
+            // since a true half cannot rescue a half that never matches.
+            var nullLevelCompound = await userAPIRepository.selectByQuery(
+              " level == ? && email != ? ",
+              parameters: {'level': null, 'email': 'nobody@$testDomain'},
+            );
+
+            expect(nullLevelCompound, isNotEmpty);
+            expect(nullLevelCompound.map((e) => e.level), everyElement(isNull));
+
+            // And the negation, which has to become `IS NOT NULL`.
+            var notNullLevel = await userAPIRepository.selectByQuery(
+              " level != ? ",
+              parameters: {'level': null},
+            );
+
+            expect(notNullLevel, isNotEmpty);
+            expect(
+              notNullLevel.map((e) => e.level),
+              everyElement(isNotNull),
+              reason: 'a negated null parameter must select the non-null rows',
+            );
+          }
+
           {
             var user4 = await userAPIRepository.selectFirstByQuery(
               "roles =~ ?",