diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c4a94a..69d0314 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/example/bones_api_example.dart b/example/bones_api_example.dart index 1af9702..876b1fa 100644 --- a/example/bones_api_example.dart +++ b/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 diff --git a/lib/src/bones_api_base.dart b/lib/src/bones_api_base.dart index 3567eff..87f9c3e 100644 --- a/lib/src/bones_api_base.dart +++ b/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,10 +511,14 @@ abstract class APIRoot with Initializable, Closable { if (response is APIResponse) { return response; } else if (response is Future?>) { - 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?>( (resp) { if (resp != null) return resp; - return _callHandlersAsync( + return _callHandlersAsync( handlersIterator, request, handlersType, @@ -522,7 +526,7 @@ abstract class APIRoot with Initializable, Closable { }, onError: (e, s) { _logCallHandlersError(handlersType, handler, e, s); - return _callHandlersAsync( + return _callHandlersAsync( 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?>) { 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>) { - 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>( + (response) => _callZonedReturn(callZone, request, response), + onError: (e, s) => _callZonedReturn( callZone, request, - _resolveErrorAPIResponse(e, s), + _resolveErrorAPIResponse(e, s), ), ); } else { diff --git a/lib/src/bones_api_condition_encoder.dart b/lib/src/bones_api_condition_encoder.dart index 81f9984..c040a0c 100644 --- a/lib/src/bones_api_condition_encoder.dart +++ b/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 encodeCondition( Condition c, EncodingContext context, diff --git a/lib/src/bones_api_condition_sql.dart b/lib/src/bones_api_condition_sql.dart index bc1e7d9..d33b024 100644 --- a/lib/src/bones_api_condition_sql.dart +++ b/lib/src/bones_api_condition_sql.dart @@ -253,7 +253,10 @@ 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, @@ -261,8 +264,8 @@ class ConditionSQLEncoder extends ConditionEncoder { 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 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> keyToSQL( KeyCondition c, EncodingContext context, diff --git a/lib/src/bones_api_test_utils_mysql.dart b/lib/src/bones_api_test_utils_mysql.dart index 9bcc55f..f6abff9 100644 --- a/lib/src/bones_api_test_utils_mysql.dart +++ b/lib/src/bones_api_test_utils_mysql.dart @@ -19,16 +19,11 @@ class APITestConfigDockerMySQL APITestConfigDockerMySQL( Map 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(); } diff --git a/lib/src/bones_api_test_utils_postgres.dart b/lib/src/bones_api_test_utils_postgres.dart index da9fb8b..2e92535 100644 --- a/lib/src/bones_api_test_utils_postgres.dart +++ b/lib/src/bones_api_test_utils_postgres.dart @@ -26,18 +26,13 @@ class APITestConfigDockerPostgreSQL APITestConfigDockerPostgreSQL( Map 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(); } diff --git a/pubspec.yaml b/pubspec.yaml index 795943f..5f73741 100644 --- a/pubspec.yaml +++ b/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 diff --git a/test/bones_api_entity_db_sqlite_test.dart b/test/bones_api_entity_db_sqlite_test.dart index 596d4fc..7333b84 100644 --- a/test/bones_api_entity_db_sqlite_test.dart +++ b/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 main() async { diff --git a/test/bones_api_entity_db_tests_base.dart b/test/bones_api_entity_db_tests_base.dart index ca2e86f..13ff905 100644 --- a/test/bones_api_entity_db_tests_base.dart +++ b/test/bones_api_entity_db_tests_base.dart @@ -1384,6 +1384,52 @@ Future 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 =~ ?", diff --git a/test/bones_api_test.reflection.g.dart b/test/bones_api_test.reflection.g.dart index 59e951a..5004849 100644 --- a/test/bones_api_test.reflection.g.dart +++ b/test/bones_api_test.reflection.g.dart @@ -1,6 +1,6 @@ // // GENERATED CODE - DO NOT MODIFY BY HAND! -// BUILDER: reflection_factory/2.8.1 +// BUILDER: reflection_factory/2.9.0 // BUILD COMMAND: dart run build_runner build // @@ -22,7 +22,7 @@ typedef __TI = TypeInfo; typedef __PR = ParameterReflection; mixin __ReflectionMixin { - static final Version _version = Version.parse('2.8.1'); + static final Version _version = Version.parse('2.9.0'); Version get reflectionFactoryVersion => _version; @@ -1093,22 +1093,17 @@ extension MyInfoModuleProxy$reflectionProxy on MyInfoModuleProxy { } Future> mapKeys(Map map) { - var ret = onCall( - this, - 'mapKeys', - {'map': map}, - const __TR>>(Future, <__TR>[__TR.tListString]), - ); + var ret = onCall(this, 'mapKeys', { + 'map': map, + }, const __TR>>(Future, <__TR>[__TR.tListString])); return __retFut$>(ret); } Future> listMultiplier(List list, int m) { - var ret = onCall( - this, - 'listMultiplier', - {'list': list, 'm': m}, - const __TR>>(Future, <__TR>[__TR.tListInt]), - ); + var ret = onCall(this, 'listMultiplier', { + 'list': list, + 'm': m, + }, const __TR>>(Future, <__TR>[__TR.tListInt])); return __retFut$>(ret); } } diff --git a/test/bones_api_test_entities.reflection.g.dart b/test/bones_api_test_entities.reflection.g.dart index 46f483a..2507057 100644 --- a/test/bones_api_test_entities.reflection.g.dart +++ b/test/bones_api_test_entities.reflection.g.dart @@ -1,6 +1,6 @@ // // GENERATED CODE - DO NOT MODIFY BY HAND! -// BUILDER: reflection_factory/2.8.1 +// BUILDER: reflection_factory/2.9.0 // BUILD COMMAND: dart run build_runner build // @@ -22,7 +22,7 @@ typedef __TI = TypeInfo; typedef __PR = ParameterReflection; mixin __ReflectionMixin { - static final Version _version = Version.parse('2.8.1'); + static final Version _version = Version.parse('2.9.0'); Version get reflectionFactoryVersion => _version; diff --git a/test/bones_api_test_entities_orders.reflection.g.dart b/test/bones_api_test_entities_orders.reflection.g.dart index 7f36076..3121a4c 100644 --- a/test/bones_api_test_entities_orders.reflection.g.dart +++ b/test/bones_api_test_entities_orders.reflection.g.dart @@ -1,6 +1,6 @@ // // GENERATED CODE - DO NOT MODIFY BY HAND! -// BUILDER: reflection_factory/2.8.1 +// BUILDER: reflection_factory/2.9.0 // BUILD COMMAND: dart run build_runner build // @@ -22,7 +22,7 @@ typedef __TI = TypeInfo; typedef __PR = ParameterReflection; mixin __ReflectionMixin { - static final Version _version = Version.parse('2.8.1'); + static final Version _version = Version.parse('2.9.0'); Version get reflectionFactoryVersion => _version; diff --git a/test/bones_api_test_modules.reflection.g.dart b/test/bones_api_test_modules.reflection.g.dart index f8c5dcd..6470a24 100644 --- a/test/bones_api_test_modules.reflection.g.dart +++ b/test/bones_api_test_modules.reflection.g.dart @@ -1,6 +1,6 @@ // // GENERATED CODE - DO NOT MODIFY BY HAND! -// BUILDER: reflection_factory/2.8.1 +// BUILDER: reflection_factory/2.9.0 // BUILD COMMAND: dart run build_runner build // @@ -22,7 +22,7 @@ typedef __TI = TypeInfo; typedef __PR = ParameterReflection; mixin __ReflectionMixin { - static final Version _version = Version.parse('2.8.1'); + static final Version _version = Version.parse('2.9.0'); Version get reflectionFactoryVersion => _version; diff --git a/test/bones_api_test_utils_test.reflection.g.dart b/test/bones_api_test_utils_test.reflection.g.dart index 0caa334..8ace306 100644 --- a/test/bones_api_test_utils_test.reflection.g.dart +++ b/test/bones_api_test_utils_test.reflection.g.dart @@ -1,6 +1,6 @@ // // GENERATED CODE - DO NOT MODIFY BY HAND! -// BUILDER: reflection_factory/2.8.1 +// BUILDER: reflection_factory/2.9.0 // BUILD COMMAND: dart run build_runner build // @@ -22,7 +22,7 @@ typedef __TI = TypeInfo; typedef __PR = ParameterReflection; mixin __ReflectionMixin { - static final Version _version = Version.parse('2.8.1'); + static final Version _version = Version.parse('2.9.0'); Version get reflectionFactoryVersion => _version;