From 375f3c60cc69b40871fb0b03c4966c79cebe59c0 Mon Sep 17 00:00:00 2001 From: Javad Zobeidi Date: Fri, 12 Dec 2025 14:58:33 +0330 Subject: [PATCH] fix(ORM): Concurrent modification error when including multiple relations #211 --- .../migration/runners/migration_runner.dart | 4 +- lib/src/database/orm/model.dart | 185 ++++++++++-------- .../_where_clauses_builder_impl.dart | 44 ++++- 3 files changed, 141 insertions(+), 92 deletions(-) diff --git a/lib/src/database/migration/runners/migration_runner.dart b/lib/src/database/migration/runners/migration_runner.dart index 48dfe3c..585cce5 100644 --- a/lib/src/database/migration/runners/migration_runner.dart +++ b/lib/src/database/migration/runners/migration_runner.dart @@ -31,7 +31,9 @@ class MigrationRunner { } catch (e) { stopwatch.stop(); if (e is QueryException) { - stderr.write(e.cause); + stderr.writeln(e.cause); + } else { + stderr.writeln(e); } stderr.writeln( '❌ Migration $migrationName failed ......................................\x1B[31m ${stopwatch.elapsedMilliseconds}ms FAILED\x1B[0m', diff --git a/lib/src/database/orm/model.dart b/lib/src/database/orm/model.dart index 9541ec1..47feb84 100644 --- a/lib/src/database/orm/model.dart +++ b/lib/src/database/orm/model.dart @@ -712,131 +712,146 @@ abstract class Model extends QueryBuilderImpl { return this; } - void _clearWithRelation(_RelationQuery r) { - _withRelation = _withRelation.where((item) => item != r).toList(); - } - Future _eagerLoadRelation( List> models, _RelationQuery rq, Function(dynamic data) callBack, ) async { String relation = rq.relation; + List wr = relation.split('.'); - if (_withRelation.any((r) => r.relation == relation)) { - List wr = relation.split('.'); + String primaryRelation = wr.first; - String primaryRelation = wr.first; + List getColumns = ['*']; + final relationParts = primaryRelation.split(':'); - List getColumns = ['*']; - final relationParts = primaryRelation.split(':'); + if (relationParts.length > 1) { + primaryRelation = relationParts.first.trim(); - if (relationParts.length > 1) { - primaryRelation = relationParts.first.trim(); + final columnsString = relationParts.last.trim(); - final columnsString = relationParts.last.trim(); - - if (columnsString.isNotEmpty) { - getColumns = columnsString - .split(',') - .map((col) => col.trim()) - .where((col) => col.isNotEmpty) - .toList(); - } + if (columnsString.isNotEmpty) { + getColumns = columnsString + .split(',') + .map((col) => col.trim()) + .where((col) => col.isNotEmpty) + .toList(); } + } - if (!_relations.containsKey(primaryRelation)) { - throw InvalidArgumentException( - 'Relation $relation not found in $runtimeType', - ); - } + if (!_relations.containsKey(primaryRelation)) { + throw InvalidArgumentException( + 'Relation $relation not found in $runtimeType', + ); + } - Relation rela = _relations[primaryRelation] as Relation; - Model qb = rela.related; - rela.parent._clearWithRelation(rq); + Relation rela = _relations[primaryRelation] as Relation; + Model qb = rela.related; - if (rq.callback != null) { - qb = rq.callback!(qb) as Model; - } + if (rq.callback != null) { + qb = rq.callback!(qb) as Model; + } - if (rela is MorphRelation) { - if (rela is MorphTo) { - Set ids = models.map((m) => m[rela.morphKey]).toSet(); - qb = qb.whereIn(rela.localKey, ids.toList()) as Model; - } else { - Set ids = models.map((m) => m[rela.localKey]).toSet(); - - if (rela is MorphToMany || rela is MorphedByMany) { - qb = - qb - .whereIn(rela.morphKey, ids.toList()) - .whereEqualTo(rela.morphType, rela.type) - .join( - rela.related.tableName, - '${rela.pivotTable}.${rela.relatedMorphKey}', - '=', - '${rela.related.tableName}.${rela.localKey}', - ) - as Model; - qb.tableName = rela.pivotTable!; - } else { - qb = - qb - .whereIn(rela.morphKey, ids.toList()) - .whereEqualTo(rela.morphType, rela.type) - as Model; - } + if (rela is MorphRelation) { + if (rela is MorphTo) { + Set ids = models.map((m) => m[rela.morphKey]).toSet(); + + // Early return if no IDs to query + if (ids.isEmpty) { + callBack(rela.match(models, [], primaryRelation)); + return; } + + qb = qb.whereIn(rela.localKey, ids.toList()) as Model; } else { - late final String getLocalKey; - if (rela is BelongsTo) { - getLocalKey = - rela.foreignKey ?? - '${rela.related.runtimeType.toString()}_id'.toLowerCase(); - } else { - getLocalKey = rela.localKey; - } + Set ids = models.map((m) => m[rela.localKey]).toSet(); - Set ids = models.map((m) => m[getLocalKey]).toSet(); + // Early return if no IDs to query + if (ids.isEmpty) { + callBack(rela.match(models, [], primaryRelation)); + return; + } - if (rela is BelongsToMany) { + if (rela is MorphToMany || rela is MorphedByMany) { qb = qb - .whereIn( - '${rela.pivotTable}.${rela.parentPivotKey}', - ids.toList(), - ) + .whereIn(rela.morphKey, ids.toList()) + .whereEqualTo(rela.morphType, rela.type) .join( - rela.pivotTable, - '${rela.pivotTable}.${rela.relatedPivotKey}', + rela.related.tableName, + '${rela.pivotTable}.${rela.relatedMorphKey}', '=', - '${rela.related.tableName}.${rela.relatedLocalKey}', + '${rela.related.tableName}.${rela.localKey}', ) as Model; - } else if (rela is BelongsTo) { - qb = qb.whereIn(rela.localKey, ids.toList()) as Model; + qb.tableName = rela.pivotTable!; } else { - qb = qb.whereIn(rela.foreignKey!, ids.toList()) as Model; + qb = + qb + .whereIn(rela.morphKey, ids.toList()) + .whereEqualTo(rela.morphType, rela.type) + as Model; } } - late final List> results; + } else { + late final String getLocalKey; + if (rela is BelongsTo) { + getLocalKey = + rela.foreignKey ?? + '${rela.related.runtimeType.toString()}_id'.toLowerCase(); + } else { + getLocalKey = rela.localKey; + } + + Set ids = models.map((m) => m[getLocalKey]).toSet(); + + // Early return if no IDs to query + if (ids.isEmpty) { + callBack(rela.match(models, [], primaryRelation)); + return; + } - if (wr.length > 1) { - wr.removeAt(0); - results = await qb.include(wr.join('.')).get(getColumns); + if (rela is BelongsToMany) { + qb = + qb + .whereIn( + '${rela.pivotTable}.${rela.parentPivotKey}', + ids.toList(), + ) + .join( + rela.pivotTable, + '${rela.pivotTable}.${rela.relatedPivotKey}', + '=', + '${rela.related.tableName}.${rela.relatedLocalKey}', + ) + as Model; + } else if (rela is BelongsTo) { + qb = qb.whereIn(rela.localKey, ids.toList()) as Model; } else { - results = await qb.get(getColumns); + qb = qb.whereIn(rela.foreignKey!, ids.toList()) as Model; } + } + + late final List> results; - callBack(rela.match(models, results, primaryRelation)); + if (wr.length > 1) { + wr.removeAt(0); + results = await qb.include(wr.join('.')).get(getColumns); + } else { + results = await qb.get(getColumns); } + + callBack(rela.match(models, results, primaryRelation)); } Future>> _loadRelations( List> result, ) async { if (_withRelation.isNotEmpty) { - final relationsToLoad = List<_RelationQuery>.from(_withRelation); + // Create an immutable copy of relations to load and clear the original list + // This prevents concurrent modification when iterating + final relationsToLoad = List<_RelationQuery>.unmodifiable(_withRelation); + _withRelation = []; for (_RelationQuery relation in relationsToLoad) { await _eagerLoadRelation(result, relation, (callBackResult) { diff --git a/lib/src/database/query_builder/_where_clauses_builder_impl.dart b/lib/src/database/query_builder/_where_clauses_builder_impl.dart index 3076145..a83fc63 100644 --- a/lib/src/database/query_builder/_where_clauses_builder_impl.dart +++ b/lib/src/database/query_builder/_where_clauses_builder_impl.dart @@ -7,15 +7,51 @@ import '_query_builder_impl.dart'; int _paramCounter = 0; abstract mixin class WhereClausesBuilderImpl implements QueryBuilder { + /// Valid SQL comparison operators to prevent SQL injection. + /// Only these operators are allowed in where clauses. + static const Set _validOperators = { + '=', + '<>', + '!=', + '<', + '>', + '<=', + '>=', + 'LIKE', + 'NOT LIKE', + 'ILIKE', // PostgreSQL case-insensitive LIKE + 'NOT ILIKE', + 'REGEXP', + 'NOT REGEXP', + 'RLIKE', // MySQL alias for REGEXP + 'SIMILAR TO', // PostgreSQL + }; + set paramCounter(int paramN) { _paramCounter = paramN; } + /// Returns the current parameter counter value. + /// Useful for synchronizing nested queries. + int get currentParamCounter => _paramCounter; + String _nextParamName() { _paramCounter++; return 'p$_paramCounter'; } + /// Validates that the given operator is a valid SQL comparison operator. + /// Throws [InvalidArgumentException] if the operator is not valid. + /// This prevents SQL injection attacks through malicious operators. + void _validateOperator(String operator) { + if (!_validOperators.contains(operator.toUpperCase())) { + throw InvalidArgumentException( + 'Invalid SQL operator: "$operator". ' + 'Allowed operators: ${_validOperators.join(", ")}', + ); + } + } + @override String buildWhereClause() { return conditions.isNotEmpty ? " WHERE ${conditions.join(" ")}" : ""; @@ -41,6 +77,7 @@ abstract mixin class WhereClausesBuilderImpl implements QueryBuilder { String boolean = 'and', ]) { if (condition is String) { + _validateOperator(operator); final paramName = _nextParamName(); bindings[paramName] = value; _appendCondition("$condition $operator :$paramName", isOr: true); @@ -284,6 +321,7 @@ abstract mixin class WhereClausesBuilderImpl implements QueryBuilder { String boolean = 'and', ]) { if (condition is String) { + _validateOperator(operator); final paramName = _nextParamName(); bindings[paramName] = value; _appendCondition( @@ -974,12 +1012,6 @@ abstract mixin class WhereClausesBuilderImpl implements QueryBuilder { String clause = not ? "NOT IN" : "IN"; if (values is List) { - if (values.isEmpty) { - throw InvalidArgumentException( - "The list of values for IN must not be empty.", - ); - } - List paramNames = []; for (var i = 0; i < values.length; i++) { final paramName = _nextParamName();