From dfef8e56221fad8a4a074f36004fd907dd3f87ce Mon Sep 17 00:00:00 2001 From: Antonio Perez Dieppa Date: Fri, 27 Feb 2026 16:49:12 +0000 Subject: [PATCH] fix: resolve ANALYSIS.md issues #6, #7, #8, #9, #10 - #10: Remove duplicate logger in CreateCollectionOperator that shadowed the parent's MongoOperator.logger - #6: Remove redundant session warning blocks from CreateCollectionOperator and CreateIndexOperator; all DDL operators now rely on base class logging - #7: Fix collation mapping for YAML input by adding Map-to-Collation conversion in MapperUtil.getCollation() with all 9 builder fields - #8: Add multi parameter support to DeleteOperator (default false = deleteOne, matching MongoDB native default and UpdateOperator pattern) - #9: Reject unrecognized parameter keys via checkUnrecognizedKeys() utility in OperationValidator, applied to all 7 non-NO_OP validators --- ANALYSIS.md | 60 +++--- .../template/mongodb/mapper/MapperUtil.java | 44 ++++- .../operator/CreateCollectionOperator.java | 7 - .../model/operator/CreateIndexOperator.java | 3 - .../model/operator/DeleteOperator.java | 17 ++ .../CreateIndexParametersValidator.java | 7 + .../CreateViewParametersValidator.java | 7 + .../validation/DeleteParametersValidator.java | 15 ++ .../DropIndexParametersValidator.java | 7 + .../validation/InsertParametersValidator.java | 7 + .../ModifyCollectionParametersValidator.java | 2 + .../validation/OperationValidator.java | 18 ++ .../RenameCollectionParametersValidator.java | 7 + .../validation/UpdateParametersValidator.java | 7 + .../mongodb/changes/_0002__seed_users.yaml | 1 + .../changes/_0003__multiple_operations.yaml | 1 + .../changes/_0004__apply_and_rollback.yaml | 1 + .../changes/_0005__step_based_change.yaml | 1 + .../mongodb/mapper/MapperUtilTest.java | 54 +++++- .../model/MongoOperationValidateTest.java | 179 ++++++++++++++++++ .../operations/DeleteOperatorTest.java | 33 +++- 21 files changed, 429 insertions(+), 49 deletions(-) diff --git a/ANALYSIS.md b/ANALYSIS.md index 40868b8..e4e3696 100644 --- a/ANALYSIS.md +++ b/ANALYSIS.md @@ -51,34 +51,32 @@ YAML -> MongoOperation (deserialized) -> MongoOperationValidator -> MongoOperati ### #5 - ~~HIGH: CreateIndexOperator claims `transactional=true` but ignores the session~~ RESOLVED **Status:** Fixed by correcting the transactional flag and warning message. **Original issue:** Constructor passed `super(mongoDatabase, operation, true)`, declaring itself transactional. But `applyInternal()` warned about "createCollection operation" (copy-paste error — should say "createIndex") and then ignored the `clientSession` entirely. MongoDB index operations are DDL and do not participate in transactions, so the flag was incorrect. -**Resolution:** Changed constructor to `super(mongoDatabase, operation, false)` to match all other DDL operators. Fixed the warning message from "createCollection operation" to "createIndex operation". - -### #6 - MEDIUM: DropIndexOperator completely ignores ClientSession -**File:** `DropIndexOperator.java:29-35` -**Impact:** Unlike other operators that check `if (clientSession != null)` and use it, `DropIndexOperator` never references `clientSession` at all (it receives it as a parameter but discards it). It's marked `transactional=false` in the constructor (line 25), which is correct, but the logging from `MongoOperator.logOperation()` will still produce a confusing info message if a session is present: `"DropIndexOperator is not transactional, but Change has been marked as transactional. Transaction ignored."` This is acceptable behavior but worth documenting. -**Fix:** Minor - add a comment explaining this is intentional. The behavior is correct. - -### #7 - MEDIUM: `getCollation()` in MapperUtil will always fail for YAML-sourced input -**File:** `MapperUtil.java:87-94` -**Impact:** The method checks `if (value instanceof Collation)` and otherwise throws `IllegalArgumentException`. When YAML is parsed, collation will be deserialized as a `Map`, never as a `Collation` object. This means the `collation` option in `IndexOptionsMapper` (line 92), `UpdateOptionsMapper` (line 41), and `CreateViewOptionsMapper` (line 31) can never work from YAML input. It will always throw `"field[collation] should be Collation"`. -**Risk:** Documented options that are impossible to use. Users who try `collation` in YAML will get a confusing error. -**Fix:** Implement Map-to-Collation conversion similar to how `getBson()` handles `Map` -> `BsonDocument` conversion. The Collation builder needs to be populated from the map keys (`locale`, `strength`, `caseLevel`, etc.). - -### #8 - MEDIUM: DeleteOperator always uses `deleteMany`, no `deleteOne` support -**File:** `DeleteOperator.java:36-38` -**Impact:** Unlike `UpdateOperator` which supports `multi: true/false` to switch between `updateOne`/`updateMany`, `DeleteOperator` always calls `collection.deleteMany()`. There is no `multi` parameter or any way to delete a single document. The `delete` operation documentation in `MongoOperationValidator.java:80-88` shows `filter` is the only parameter, confirming `deleteOne` is not supported. -**Risk:** Users cannot safely delete a single document matching a filter when multiple documents could match. All matching documents are always deleted. -**Fix:** Add `multi` parameter support (defaulting to `true` for backwards compatibility, or `false` to match MongoDB's default `deleteOne`). Consider the migration safety implications of the default. - -### #9 - MEDIUM: Unknown YAML fields are silently accepted -**Impact:** The YAML deserialization into `MongoOperation` only maps `type`, `collection`, and `parameters`. Any extra top-level field (e.g., a typo like `colection` or `paramters`) is silently ignored. Within `parameters`, the validator checks for specific required keys but never rejects unknown keys. A user could write `parameters: { documets: [...] }` (typo) for an insert, and the validator would correctly fail with "requires 'documents' parameter", but a field like `parameters: { documents: [...], unknown_field: true }` passes silently. -**Risk:** Typos in parameter names that don't affect required-field validation go undetected. -**Fix:** Add strict mode option that warns or rejects unknown parameter keys per operation type. - -### #10 - LOW: Duplicate logger field in CreateCollectionOperator -**File:** `CreateCollectionOperator.java:25` -**Impact:** `CreateCollectionOperator` declares `protected static final Logger logger = FlamingockLoggerFactory.getLogger("CreateCollection")` which shadows the parent's `MongoOperator.logger` field (also `protected static final Logger logger` at line 25). Both are static, so the parent's `logOperation()` method uses `MongoOperator.logger` ("MongoTemplate") while `CreateCollectionOperator.applyInternal()` uses its own `logger` ("CreateCollection"). This inconsistency means log messages from the same operation go to different logger names. -**Fix:** Remove the duplicate logger declaration from `CreateCollectionOperator`. Let it inherit the parent's. +**Resolution:** Changed constructor to `super(mongoDatabase, operation, false)` to match all other DDL operators. Fixed the warning message from "createCollection operation" to "createIndex operation". The per-operator warning blocks were later removed entirely (see issue #6) since the base class `MongoOperator.logOperation()` already handles session mismatch logging uniformly. + +### #6 - ~~MEDIUM: DDL operators have inconsistent session handling~~ RESOLVED +**Status:** Fixed by removing redundant session warning blocks. +**Original issue:** `CreateCollectionOperator` and `CreateIndexOperator` had redundant `if (clientSession != null) { logger.warn(...) }` blocks, while the other 6 DDL operators silently ignored the session. The base class `MongoOperator.logOperation()` already handles this case with an INFO log. +**Resolution:** Removed the redundant warning blocks from `CreateCollectionOperator` and `CreateIndexOperator`. All 8 DDL operators now consistently trust the base class logging. Also removed the duplicate logger declaration from `CreateCollectionOperator` (see issue #10). + +### #7 - ~~MEDIUM: `getCollation()` in MapperUtil will always fail for YAML-sourced input~~ RESOLVED +**Status:** Fixed by adding Map-to-Collation conversion. +**Original issue:** `getCollation()` only accepted `Collation` instances but YAML always deserializes to `Map`, making the `collation` option in `IndexOptionsMapper`, `UpdateOptionsMapper`, and `CreateViewOptionsMapper` impossible to use from YAML. +**Resolution:** Added `else if (value instanceof Map)` branch to `getCollation()` with a `buildCollationFromMap()` private method that handles all 9 Collation builder fields (`locale`, `caseLevel`, `caseFirst`, `strength`, `numericOrdering`, `alternate`, `maxVariable`, `normalization`, `backwards`). Tests added for Map with locale only, Map with all fields, and invalid type. + +### #8 - ~~MEDIUM: DeleteOperator always uses `deleteMany`, no `deleteOne` support~~ RESOLVED +**Status:** Fixed by adding `multi` parameter support. +**Original issue:** `DeleteOperator` always called `deleteMany()` with no way to delete a single document. +**Resolution:** Added `multi` parameter support to `DeleteOperator`, following the same pattern as `UpdateOperator`. Default is `false` (`deleteOne`), matching MongoDB's native default. Users must explicitly set `multi: true` for `deleteMany`. `DeleteParametersValidator` validates that `multi` is a boolean when present. Existing YAML test files updated to set `multi: true` where `deleteMany` behavior is intended. + +### #9 - ~~MEDIUM: Unknown YAML fields are silently accepted~~ RESOLVED +**Status:** Fixed by adding unrecognized parameter key rejection. +**Original issue:** Unknown parameter keys (typos like `documets` instead of `documents`) were silently accepted within `parameters`. +**Resolution:** Added a static `checkUnrecognizedKeys()` utility method to `OperationValidator` interface. Each of the 7 non-NO_OP validators now declares a `RECOGNIZED_KEYS` set and calls this utility at the end of validation. Unrecognized keys produce a validation error like `"Insert operation does not recognize parameter 'unknownKey'"`. Top-level keys only — nested fields inside `options`, `filter`, `documents`, etc. are not validated (they're opaque MongoDB driver domain). NO_OP validators (createCollection, dropCollection, dropView) are left as-is since those operations don't expect parameters. Tests added for all 8 operation types with validators. + +### #10 - ~~LOW: Duplicate logger field in CreateCollectionOperator~~ RESOLVED +**Status:** Fixed by removing duplicate logger. +**Original issue:** `CreateCollectionOperator` declared its own `logger` which shadowed the parent's `MongoOperator.logger`, causing inconsistent logger names for the same operation. +**Resolution:** Removed the duplicate logger declaration and unused imports from `CreateCollectionOperator`. It now inherits the parent's logger. --- @@ -86,12 +84,12 @@ YAML -> MongoOperation (deserialized) -> MongoOperationValidator -> MongoOperati | Operation | Enum Value | Operator Class | Transactional | Validation | Options Mapper | Session Handling | Unit Test | Integration Test | |-----------|-----------|---------------|:---:|:---:|:---:|:---:|:---:|:---:| -| createCollection | `CREATE_COLLECTION` | `CreateCollectionOperator` | No | collection only | None | Warns & ignores | `CreateCollectionOperatorTest` (1 test) | YAML `_0001` | +| createCollection | `CREATE_COLLECTION` | `CreateCollectionOperator` | No | collection only | None | Ignores (base class logs) | `CreateCollectionOperatorTest` (1 test) | YAML `_0001` | | dropCollection | `DROP_COLLECTION` | `DropCollectionOperator` | No | collection only | None | Ignores entirely | `DropCollectionOperatorTest` (1 test) | YAML `_0004` rollback | | insert | `INSERT` | `InsertOperator` | Yes | Full (documents) | `InsertOptionsMapper` | Full | `InsertOperatorTest` (3 tests) | YAML `_0002`, `_0003`, `_0005` | | update | `UPDATE` | `UpdateOperator` | Yes | Full (filter, update) | `UpdateOptionsMapper` | Full | `UpdateOperatorTest` (1 test) | None | -| delete | `DELETE` | `DeleteOperator` | Yes | filter required | None | Full | `DeleteOperatorTest` (1 test) | YAML `_0002` rollback | -| createIndex | `CREATE_INDEX` | `CreateIndexOperator` | No | Full (keys) | `IndexOptionsMapper` | Warns & ignores | `CreateIndexOperatorTest` (1 test) | YAML `_0003`, `_0005` | +| delete | `DELETE` | `DeleteOperator` | Yes | filter required, multi (opt) | None | Full | `DeleteOperatorTest` (5 tests) | YAML `_0002` rollback | +| createIndex | `CREATE_INDEX` | `CreateIndexOperator` | No | Full (keys) | `IndexOptionsMapper` | Ignores (base class logs) | `CreateIndexOperatorTest` (1 test) | YAML `_0003`, `_0005` | | dropIndex | `DROP_INDEX` | `DropIndexOperator` | No | indexName or keys | None | **Ignores entirely** | `DropIndexOperatorTest` (1 test) | YAML `_0005` rollback | | renameCollection | `RENAME_COLLECTION` | `RenameCollectionOperator` | No | target required | `RenameCollectionOptionsMapper` | Ignores entirely | `RenameCollectionOperatorTest` (1 test) | None | | modifyCollection | `MODIFY_COLLECTION` | `ModifyCollectionOperator` | No | Full (validator, validationLevel, validationAction) | None | Ignores entirely | `ModifyCollectionOperatorTest` (1 test) | None | diff --git a/src/main/java/io/flamingock/template/mongodb/mapper/MapperUtil.java b/src/main/java/io/flamingock/template/mongodb/mapper/MapperUtil.java index f9f07e6..dd261a9 100644 --- a/src/main/java/io/flamingock/template/mongodb/mapper/MapperUtil.java +++ b/src/main/java/io/flamingock/template/mongodb/mapper/MapperUtil.java @@ -16,6 +16,10 @@ package io.flamingock.template.mongodb.mapper; import com.mongodb.client.model.Collation; +import com.mongodb.client.model.CollationAlternate; +import com.mongodb.client.model.CollationCaseFirst; +import com.mongodb.client.model.CollationMaxVariable; +import com.mongodb.client.model.CollationStrength; import org.bson.BsonArray; import org.bson.BsonDocument; import org.bson.BsonValue; @@ -88,9 +92,47 @@ public static Collation getCollation(Map options, String key) { Object value = options.get(key); if (value instanceof Collation) { return (Collation) value; + } else if (value instanceof Map) { + @SuppressWarnings("unchecked") + Map map = (Map) value; + return buildCollationFromMap(map); } else { - throw new IllegalArgumentException(String.format("field[%s] should be Collation", key)); + throw new IllegalArgumentException(String.format("field[%s] should be Collation or Map", key)); + } + } + + private static Collation buildCollationFromMap(Map map) { + Object locale = map.get("locale"); + if (!(locale instanceof String)) { + throw new IllegalArgumentException("Collation requires 'locale' as a String"); + } + Collation.Builder builder = Collation.builder().locale((String) locale); + + if (map.containsKey("caseLevel")) { + builder.caseLevel((Boolean) map.get("caseLevel")); + } + if (map.containsKey("caseFirst")) { + builder.collationCaseFirst(CollationCaseFirst.fromString((String) map.get("caseFirst"))); + } + if (map.containsKey("strength")) { + builder.collationStrength(CollationStrength.fromInt(((Number) map.get("strength")).intValue())); + } + if (map.containsKey("numericOrdering")) { + builder.numericOrdering((Boolean) map.get("numericOrdering")); + } + if (map.containsKey("alternate")) { + builder.collationAlternate(CollationAlternate.fromString((String) map.get("alternate"))); + } + if (map.containsKey("maxVariable")) { + builder.collationMaxVariable(CollationMaxVariable.fromString((String) map.get("maxVariable"))); + } + if (map.containsKey("normalization")) { + builder.normalization((Boolean) map.get("normalization")); + } + if (map.containsKey("backwards")) { + builder.backwards((Boolean) map.get("backwards")); } + return builder.build(); } // Recursively converts a Map to BsonDocument diff --git a/src/main/java/io/flamingock/template/mongodb/model/operator/CreateCollectionOperator.java b/src/main/java/io/flamingock/template/mongodb/model/operator/CreateCollectionOperator.java index 9096f03..a5ff4ca 100644 --- a/src/main/java/io/flamingock/template/mongodb/model/operator/CreateCollectionOperator.java +++ b/src/main/java/io/flamingock/template/mongodb/model/operator/CreateCollectionOperator.java @@ -18,12 +18,8 @@ import com.mongodb.client.ClientSession; import com.mongodb.client.MongoDatabase; import io.flamingock.template.mongodb.model.MongoOperation; -import io.flamingock.internal.util.log.FlamingockLoggerFactory; -import org.slf4j.Logger; public class CreateCollectionOperator extends MongoOperator { - protected static final Logger logger = FlamingockLoggerFactory.getLogger("CreateCollection"); - public CreateCollectionOperator(MongoDatabase mongoDatabase, MongoOperation operation) { super(mongoDatabase, operation, false); @@ -31,9 +27,6 @@ public CreateCollectionOperator(MongoDatabase mongoDatabase, MongoOperation oper @Override public void applyInternal(ClientSession clientSession) { - if (clientSession != null) { - logger.warn("MongoDB does not support transactions for createCollection operation. Ignoring transactional flag."); - } mongoDatabase.createCollection(op.getCollection()); } diff --git a/src/main/java/io/flamingock/template/mongodb/model/operator/CreateIndexOperator.java b/src/main/java/io/flamingock/template/mongodb/model/operator/CreateIndexOperator.java index 2a5c595..8e3954e 100644 --- a/src/main/java/io/flamingock/template/mongodb/model/operator/CreateIndexOperator.java +++ b/src/main/java/io/flamingock/template/mongodb/model/operator/CreateIndexOperator.java @@ -30,9 +30,6 @@ public CreateIndexOperator(MongoDatabase mongoDatabase, MongoOperation operation @Override protected void applyInternal(ClientSession clientSession) { - if (clientSession != null) { - logger.warn("MongoDB does not support transactions for createIndex operation. Ignoring transactional flag."); - } IndexOptions indexOptions = IndexOptionsMapper.mapToIndexOptions(op.getOptions()); mongoDatabase.getCollection(op.getCollection()) .createIndex(op.getKeys(), indexOptions); diff --git a/src/main/java/io/flamingock/template/mongodb/model/operator/DeleteOperator.java b/src/main/java/io/flamingock/template/mongodb/model/operator/DeleteOperator.java index 0710038..21dd6e1 100644 --- a/src/main/java/io/flamingock/template/mongodb/model/operator/DeleteOperator.java +++ b/src/main/java/io/flamingock/template/mongodb/model/operator/DeleteOperator.java @@ -31,7 +31,24 @@ public DeleteOperator(MongoDatabase mongoDatabase, MongoOperation operation) { protected void applyInternal(ClientSession clientSession) { MongoCollection collection = mongoDatabase.getCollection(op.getCollection()); Document filter = op.getFilter(); + boolean multi = op.isMulti(); + if (multi) { + deleteMany(clientSession, collection, filter); + } else { + deleteOne(clientSession, collection, filter); + } + } + + private void deleteOne(ClientSession clientSession, MongoCollection collection, Document filter) { + if (clientSession != null) { + collection.deleteOne(clientSession, filter); + } else { + collection.deleteOne(filter); + } + } + + private void deleteMany(ClientSession clientSession, MongoCollection collection, Document filter) { if (clientSession != null) { collection.deleteMany(clientSession, filter); } else { diff --git a/src/main/java/io/flamingock/template/mongodb/validation/CreateIndexParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/CreateIndexParametersValidator.java index 8b46ed1..4d0c99a 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/CreateIndexParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/CreateIndexParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class CreateIndexParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("keys", "options")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -59,6 +64,8 @@ public List validate(MongoOperation operation) { "'options' must be a document")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "CreateIndex")); + return errors; } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/CreateViewParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/CreateViewParametersValidator.java index 17f06a7..6ab2002 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/CreateViewParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/CreateViewParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class CreateViewParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("viewOn", "pipeline", "options")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -56,6 +61,8 @@ public List validate(MongoOperation operation) { "'options' must be a document")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "CreateView")); + return errors; } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/DeleteParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/DeleteParametersValidator.java index e1f266b..24eeaa5 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/DeleteParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/DeleteParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class DeleteParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("filter", "multi")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -38,6 +43,16 @@ public List validate(MongoOperation operation) { "'filter' must be a document")); } + if (params != null) { + Object multi = params.get("multi"); + if (multi != null && !(multi instanceof Boolean)) { + errors.add(new TemplatePayloadValidationError("parameters.multi", + "'multi' must be a boolean")); + } + + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "Delete")); + } + return errors; } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/DropIndexParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/DropIndexParametersValidator.java index 1c32a60..b638efc 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/DropIndexParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/DropIndexParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class DropIndexParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("indexName", "keys")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -53,6 +58,8 @@ public List validate(MongoOperation operation) { "'keys' must be a map")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "DropIndex")); + return errors; } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/InsertParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/InsertParametersValidator.java index f591e21..af6b60d 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/InsertParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/InsertParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class InsertParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("documents", "options")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -67,6 +72,8 @@ public List validate(MongoOperation operation) { "'options' must be a document")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "Insert")); + return errors; } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/ModifyCollectionParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/ModifyCollectionParametersValidator.java index 57cb8ff..afe218d 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/ModifyCollectionParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/ModifyCollectionParametersValidator.java @@ -60,6 +60,8 @@ public List validate(MongoOperation operation) { "'validationAction' must be one of: 'error', 'warn'")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "ModifyCollection")); + return errors; } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/OperationValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/OperationValidator.java index a8acffe..bf13f01 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/OperationValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/OperationValidator.java @@ -18,8 +18,11 @@ import io.flamingock.api.template.TemplatePayloadValidationError; import io.flamingock.template.mongodb.model.MongoOperation; +import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Map; +import java.util.Set; @FunctionalInterface public interface OperationValidator { @@ -27,4 +30,19 @@ public interface OperationValidator { OperationValidator NO_OP = operation -> Collections.emptyList(); List validate(MongoOperation operation); + + static List checkUnrecognizedKeys( + Map params, Set recognizedKeys, String operationName) { + List errors = new ArrayList<>(); + if (params == null) { + return errors; + } + for (String key : params.keySet()) { + if (!recognizedKeys.contains(key)) { + errors.add(new TemplatePayloadValidationError("parameters." + key, + operationName + " operation does not recognize parameter '" + key + "'")); + } + } + return errors; + } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/RenameCollectionParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/RenameCollectionParametersValidator.java index 7c82fe6..a3f8828 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/RenameCollectionParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/RenameCollectionParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class RenameCollectionParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("target", "options")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -47,6 +52,8 @@ public List validate(MongoOperation operation) { "'options' must be a document")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "RenameCollection")); + return errors; } } diff --git a/src/main/java/io/flamingock/template/mongodb/validation/UpdateParametersValidator.java b/src/main/java/io/flamingock/template/mongodb/validation/UpdateParametersValidator.java index 8fea908..0775a4e 100644 --- a/src/main/java/io/flamingock/template/mongodb/validation/UpdateParametersValidator.java +++ b/src/main/java/io/flamingock/template/mongodb/validation/UpdateParametersValidator.java @@ -19,11 +19,16 @@ import io.flamingock.template.mongodb.model.MongoOperation; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class UpdateParametersValidator implements OperationValidator { + private static final Set RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("filter", "update", "multi", "options")); + @Override public List validate(MongoOperation operation) { List errors = new ArrayList<>(); @@ -59,6 +64,8 @@ public List validate(MongoOperation operation) { "'options' must be a document")); } + errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "Update")); + return errors; } } diff --git a/src/test/java/io/flamingock/template/mongodb/changes/_0002__seed_users.yaml b/src/test/java/io/flamingock/template/mongodb/changes/_0002__seed_users.yaml index 7413d3f..da59b7e 100644 --- a/src/test/java/io/flamingock/template/mongodb/changes/_0002__seed_users.yaml +++ b/src/test/java/io/flamingock/template/mongodb/changes/_0002__seed_users.yaml @@ -20,3 +20,4 @@ steps: collection: users parameters: filter: {} + multi: true diff --git a/src/test/java/io/flamingock/template/mongodb/changes/_0003__multiple_operations.yaml b/src/test/java/io/flamingock/template/mongodb/changes/_0003__multiple_operations.yaml index ee07965..16b5d91 100644 --- a/src/test/java/io/flamingock/template/mongodb/changes/_0003__multiple_operations.yaml +++ b/src/test/java/io/flamingock/template/mongodb/changes/_0003__multiple_operations.yaml @@ -31,6 +31,7 @@ steps: collection: products parameters: filter: {} + multi: true - apply: type: createIndex diff --git a/src/test/java/io/flamingock/template/mongodb/changes/_0004__apply_and_rollback.yaml b/src/test/java/io/flamingock/template/mongodb/changes/_0004__apply_and_rollback.yaml index 48a15a3..c714995 100644 --- a/src/test/java/io/flamingock/template/mongodb/changes/_0004__apply_and_rollback.yaml +++ b/src/test/java/io/flamingock/template/mongodb/changes/_0004__apply_and_rollback.yaml @@ -26,6 +26,7 @@ steps: collection: rollbackTestCollection parameters: filter: {} + multi: true - apply: type: createIndex diff --git a/src/test/java/io/flamingock/template/mongodb/changes/_0005__step_based_change.yaml b/src/test/java/io/flamingock/template/mongodb/changes/_0005__step_based_change.yaml index 1b04342..068617e 100644 --- a/src/test/java/io/flamingock/template/mongodb/changes/_0005__step_based_change.yaml +++ b/src/test/java/io/flamingock/template/mongodb/changes/_0005__step_based_change.yaml @@ -26,6 +26,7 @@ steps: collection: stepTestCollection parameters: filter: {} + multi: true - apply: type: createIndex diff --git a/src/test/java/io/flamingock/template/mongodb/mapper/MapperUtilTest.java b/src/test/java/io/flamingock/template/mongodb/mapper/MapperUtilTest.java index 0d31b75..9a03d90 100644 --- a/src/test/java/io/flamingock/template/mongodb/mapper/MapperUtilTest.java +++ b/src/test/java/io/flamingock/template/mongodb/mapper/MapperUtilTest.java @@ -16,6 +16,9 @@ package io.flamingock.template.mongodb.mapper; import com.mongodb.client.model.Collation; +import com.mongodb.client.model.CollationAlternate; +import com.mongodb.client.model.CollationCaseFirst; +import com.mongodb.client.model.CollationMaxVariable; import com.mongodb.client.model.CollationStrength; import org.bson.BsonArray; import org.bson.BsonBoolean; @@ -250,13 +253,60 @@ void getCollationValidTest() { } @Test - @DisplayName("WHEN value is not Collation THEN throws IllegalArgumentException") + @DisplayName("WHEN value is Map with locale only THEN returns Collation") + void getCollationFromMapLocaleOnlyTest() { + Map collationMap = new HashMap<>(); + collationMap.put("locale", "en"); + + Map options = new HashMap<>(); + options.put("collation", collationMap); + + Collation result = MapperUtil.getCollation(options, "collation"); + + assertNotNull(result); + assertEquals("en", result.getLocale()); + } + + @Test + @DisplayName("WHEN value is Map with all fields THEN returns fully configured Collation") + void getCollationFromMapAllFieldsTest() { + Map collationMap = new HashMap<>(); + collationMap.put("locale", "en"); + collationMap.put("caseLevel", true); + collationMap.put("caseFirst", "upper"); + collationMap.put("strength", 2); + collationMap.put("numericOrdering", true); + collationMap.put("alternate", "shifted"); + collationMap.put("maxVariable", "space"); + collationMap.put("normalization", true); + collationMap.put("backwards", true); + + Map options = new HashMap<>(); + options.put("collation", collationMap); + + Collation result = MapperUtil.getCollation(options, "collation"); + + assertNotNull(result); + assertEquals("en", result.getLocale()); + assertTrue(result.getCaseLevel()); + assertEquals(CollationCaseFirst.UPPER, result.getCaseFirst()); + assertEquals(CollationStrength.SECONDARY, result.getStrength()); + assertTrue(result.getNumericOrdering()); + assertEquals(CollationAlternate.SHIFTED, result.getAlternate()); + assertEquals(CollationMaxVariable.SPACE, result.getMaxVariable()); + assertTrue(result.getNormalization()); + assertTrue(result.getBackwards()); + } + + @Test + @DisplayName("WHEN value is not Collation or Map THEN throws IllegalArgumentException") void getCollationInvalidTest() { Map options = new HashMap<>(); options.put("collation", "not a collation"); - assertThrows(IllegalArgumentException.class, () -> + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> MapperUtil.getCollation(options, "collation")); + assertTrue(ex.getMessage().contains("Collation or Map")); } } diff --git a/src/test/java/io/flamingock/template/mongodb/model/MongoOperationValidateTest.java b/src/test/java/io/flamingock/template/mongodb/model/MongoOperationValidateTest.java index 3359531..68d3adf 100644 --- a/src/test/java/io/flamingock/template/mongodb/model/MongoOperationValidateTest.java +++ b/src/test/java/io/flamingock/template/mongodb/model/MongoOperationValidateTest.java @@ -458,6 +458,26 @@ void deleteEmptyFilterTest() { assertTrue(errors.isEmpty()); } + @Test + @DisplayName("WHEN delete multi is wrong type THEN validation fails") + void deleteMultiWrongTypeTest() { + MongoOperation op = new MongoOperation(); + op.setType("delete"); + op.setCollection("test"); + Map params = new HashMap<>(); + Map filter = new HashMap<>(); + filter.put("name", "Test"); + params.put("filter", filter); + params.put("multi", "true"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.multi", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("must be a boolean")); + } + @Test @DisplayName("WHEN delete is valid THEN validation passes") void deleteValidTest() { @@ -1077,4 +1097,163 @@ void multipleErrorsTest() { assertEquals(2, errors.size()); } } + + @Nested + @DisplayName("Unrecognized Keys Tests") + class UnrecognizedKeysTests { + + @Test + @DisplayName("WHEN insert has unrecognized key THEN validation fails") + void insertUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("insert"); + op.setCollection("test"); + Map params = new HashMap<>(); + List> docs = new ArrayList<>(); + Map doc = new HashMap<>(); + doc.put("name", "Test"); + docs.add(doc); + params.put("documents", docs); + params.put("unknownKey", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.unknownKey", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'unknownKey'")); + } + + @Test + @DisplayName("WHEN update has unrecognized key THEN validation fails") + void updateUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("update"); + op.setCollection("test"); + Map params = new HashMap<>(); + params.put("filter", new HashMap<>()); + Map update = new HashMap<>(); + update.put("$set", new HashMap<>()); + params.put("update", update); + params.put("typoKey", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.typoKey", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'typoKey'")); + } + + @Test + @DisplayName("WHEN delete has unrecognized key THEN validation fails") + void deleteUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("delete"); + op.setCollection("test"); + Map params = new HashMap<>(); + params.put("filter", new HashMap<>()); + params.put("unknownKey", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.unknownKey", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'unknownKey'")); + } + + @Test + @DisplayName("WHEN createIndex has unrecognized key THEN validation fails") + void createIndexUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("createIndex"); + op.setCollection("test"); + Map params = new HashMap<>(); + Map keys = new HashMap<>(); + keys.put("email", 1); + params.put("keys", keys); + params.put("badParam", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.badParam", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'badParam'")); + } + + @Test + @DisplayName("WHEN dropIndex has unrecognized key THEN validation fails") + void dropIndexUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("dropIndex"); + op.setCollection("test"); + Map params = new HashMap<>(); + params.put("indexName", "test_index"); + params.put("extra", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.extra", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'extra'")); + } + + @Test + @DisplayName("WHEN renameCollection has unrecognized key THEN validation fails") + void renameCollectionUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("renameCollection"); + op.setCollection("test"); + Map params = new HashMap<>(); + params.put("target", "newName"); + params.put("badParam", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.badParam", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'badParam'")); + } + + @Test + @DisplayName("WHEN createView has unrecognized key THEN validation fails") + void createViewUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("createView"); + op.setCollection("testView"); + Map params = new HashMap<>(); + params.put("viewOn", "sourceCollection"); + params.put("pipeline", Collections.singletonList(new HashMap<>())); + params.put("badParam", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.badParam", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'badParam'")); + } + + @Test + @DisplayName("WHEN modifyCollection has unrecognized key THEN validation fails") + void modifyCollectionUnrecognizedKeyTest() { + MongoOperation op = new MongoOperation(); + op.setType("modifyCollection"); + op.setCollection("test"); + Map params = new HashMap<>(); + params.put("validationLevel", "strict"); + params.put("unknownKey", "value"); + op.setParameters(params); + + List errors = op.validate(); + + assertEquals(1, errors.size()); + assertEquals("parameters.unknownKey", errors.get(0).getField()); + assertTrue(errors.get(0).getMessage().contains("does not recognize parameter 'unknownKey'")); + } + } } diff --git a/src/test/java/io/flamingock/template/mongodb/operations/DeleteOperatorTest.java b/src/test/java/io/flamingock/template/mongodb/operations/DeleteOperatorTest.java index c72cda2..318d7b6 100644 --- a/src/test/java/io/flamingock/template/mongodb/operations/DeleteOperatorTest.java +++ b/src/test/java/io/flamingock/template/mongodb/operations/DeleteOperatorTest.java @@ -74,8 +74,8 @@ void setupEach() { } @Test - @DisplayName("WHEN delete operator is applied with specific filter THEN matching documents are deleted") - void deleteWithFilterTest() { + @DisplayName("WHEN delete operator is applied with multi=true THEN all matching documents are deleted") + void deleteWithFilterMultiTrueTest() { assertEquals(4, getDocumentCount(), "Collection should have 4 documents before delete"); MongoOperation operation = new MongoOperation(); @@ -86,12 +86,13 @@ void deleteWithFilterTest() { Map filter = new HashMap<>(); filter.put("role", "user"); params.put("filter", filter); + params.put("multi", true); operation.setParameters(params); DeleteOperator operator = new DeleteOperator(mongoDatabase, operation); operator.apply(null); - assertEquals(2, getDocumentCount(), "Collection should have 2 documents after delete"); + assertEquals(2, getDocumentCount(), "Collection should have 2 documents after deleteMany"); List remainingDocs = mongoDatabase.getCollection(COLLECTION_NAME) .find() @@ -103,7 +104,28 @@ void deleteWithFilterTest() { } @Test - @DisplayName("WHEN delete operator is applied with empty filter THEN all documents are deleted") + @DisplayName("WHEN delete operator is applied without multi (default) THEN only one matching document is deleted") + void deleteWithFilterDefaultSingleTest() { + assertEquals(4, getDocumentCount(), "Collection should have 4 documents before delete"); + + MongoOperation operation = new MongoOperation(); + operation.setType("delete"); + operation.setCollection(COLLECTION_NAME); + + Map params = new HashMap<>(); + Map filter = new HashMap<>(); + filter.put("role", "user"); + params.put("filter", filter); + operation.setParameters(params); + + DeleteOperator operator = new DeleteOperator(mongoDatabase, operation); + operator.apply(null); + + assertEquals(3, getDocumentCount(), "Collection should have 3 documents after deleteOne"); + } + + @Test + @DisplayName("WHEN delete operator is applied with empty filter and multi=true THEN all documents are deleted") void deleteAllWithEmptyFilterTest() { assertEquals(4, getDocumentCount(), "Collection should have 4 documents before delete"); @@ -113,12 +135,13 @@ void deleteAllWithEmptyFilterTest() { Map params = new HashMap<>(); params.put("filter", new HashMap<>()); + params.put("multi", true); operation.setParameters(params); DeleteOperator operator = new DeleteOperator(mongoDatabase, operation); operator.apply(null); - assertEquals(0, getDocumentCount(), "Collection should be empty after delete with empty filter"); + assertEquals(0, getDocumentCount(), "Collection should be empty after deleteMany with empty filter"); } @Test