Skip to content

Commit 0fe108f

Browse files
authored
fix: resolve ANALYSIS.md issues #6, #7, #8, #9, #10 (#11)
- issue-10: Remove duplicate logger in CreateCollectionOperator that shadowed the parent's MongoOperator.logger - issue-6: Remove redundant session warning blocks from CreateCollectionOperator and CreateIndexOperator; all DDL operators now rely on base class logging - issue-7: Fix collation mapping for YAML input by adding Map-to-Collation conversion in MapperUtil.getCollation() with all 9 builder fields - issue-8: Add multi parameter support to DeleteOperator (default false = deleteOne, matching MongoDB native default and UpdateOperator pattern) - issue-9: Reject unrecognized parameter keys via checkUnrecognizedKeys() utility in OperationValidator, applied to all 7 non-NO_OP validators
1 parent a94281e commit 0fe108f

21 files changed

Lines changed: 429 additions & 49 deletions

ANALYSIS.md

Lines changed: 29 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -51,47 +51,45 @@ YAML -> MongoOperation (deserialized) -> MongoOperationValidator -> MongoOperati
5151
### #5 - ~~HIGH: CreateIndexOperator claims `transactional=true` but ignores the session~~ RESOLVED
5252
**Status:** Fixed by correcting the transactional flag and warning message.
5353
**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.
54-
**Resolution:** Changed constructor to `super(mongoDatabase, operation, false)` to match all other DDL operators. Fixed the warning message from "createCollection operation" to "createIndex operation".
55-
56-
### #6 - MEDIUM: DropIndexOperator completely ignores ClientSession
57-
**File:** `DropIndexOperator.java:29-35`
58-
**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.
59-
**Fix:** Minor - add a comment explaining this is intentional. The behavior is correct.
60-
61-
### #7 - MEDIUM: `getCollation()` in MapperUtil will always fail for YAML-sourced input
62-
**File:** `MapperUtil.java:87-94`
63-
**Impact:** The method checks `if (value instanceof Collation)` and otherwise throws `IllegalArgumentException`. When YAML is parsed, collation will be deserialized as a `Map<String, Object>`, 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"`.
64-
**Risk:** Documented options that are impossible to use. Users who try `collation` in YAML will get a confusing error.
65-
**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.).
66-
67-
### #8 - MEDIUM: DeleteOperator always uses `deleteMany`, no `deleteOne` support
68-
**File:** `DeleteOperator.java:36-38`
69-
**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.
70-
**Risk:** Users cannot safely delete a single document matching a filter when multiple documents could match. All matching documents are always deleted.
71-
**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.
72-
73-
### #9 - MEDIUM: Unknown YAML fields are silently accepted
74-
**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.
75-
**Risk:** Typos in parameter names that don't affect required-field validation go undetected.
76-
**Fix:** Add strict mode option that warns or rejects unknown parameter keys per operation type.
77-
78-
### #10 - LOW: Duplicate logger field in CreateCollectionOperator
79-
**File:** `CreateCollectionOperator.java:25`
80-
**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.
81-
**Fix:** Remove the duplicate logger declaration from `CreateCollectionOperator`. Let it inherit the parent's.
54+
**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.
55+
56+
### #6 - ~~MEDIUM: DDL operators have inconsistent session handling~~ RESOLVED
57+
**Status:** Fixed by removing redundant session warning blocks.
58+
**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.
59+
**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).
60+
61+
### #7 - ~~MEDIUM: `getCollation()` in MapperUtil will always fail for YAML-sourced input~~ RESOLVED
62+
**Status:** Fixed by adding Map-to-Collation conversion.
63+
**Original issue:** `getCollation()` only accepted `Collation` instances but YAML always deserializes to `Map<String, Object>`, making the `collation` option in `IndexOptionsMapper`, `UpdateOptionsMapper`, and `CreateViewOptionsMapper` impossible to use from YAML.
64+
**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.
65+
66+
### #8 - ~~MEDIUM: DeleteOperator always uses `deleteMany`, no `deleteOne` support~~ RESOLVED
67+
**Status:** Fixed by adding `multi` parameter support.
68+
**Original issue:** `DeleteOperator` always called `deleteMany()` with no way to delete a single document.
69+
**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.
70+
71+
### #9 - ~~MEDIUM: Unknown YAML fields are silently accepted~~ RESOLVED
72+
**Status:** Fixed by adding unrecognized parameter key rejection.
73+
**Original issue:** Unknown parameter keys (typos like `documets` instead of `documents`) were silently accepted within `parameters`.
74+
**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.
75+
76+
### #10 - ~~LOW: Duplicate logger field in CreateCollectionOperator~~ RESOLVED
77+
**Status:** Fixed by removing duplicate logger.
78+
**Original issue:** `CreateCollectionOperator` declared its own `logger` which shadowed the parent's `MongoOperator.logger`, causing inconsistent logger names for the same operation.
79+
**Resolution:** Removed the duplicate logger declaration and unused imports from `CreateCollectionOperator`. It now inherits the parent's logger.
8280

8381
---
8482

8583
## 3. Operation Coverage Matrix
8684

8785
| Operation | Enum Value | Operator Class | Transactional | Validation | Options Mapper | Session Handling | Unit Test | Integration Test |
8886
|-----------|-----------|---------------|:---:|:---:|:---:|:---:|:---:|:---:|
89-
| createCollection | `CREATE_COLLECTION` | `CreateCollectionOperator` | No | collection only | None | Warns & ignores | `CreateCollectionOperatorTest` (1 test) | YAML `_0001` |
87+
| createCollection | `CREATE_COLLECTION` | `CreateCollectionOperator` | No | collection only | None | Ignores (base class logs) | `CreateCollectionOperatorTest` (1 test) | YAML `_0001` |
9088
| dropCollection | `DROP_COLLECTION` | `DropCollectionOperator` | No | collection only | None | Ignores entirely | `DropCollectionOperatorTest` (1 test) | YAML `_0004` rollback |
9189
| insert | `INSERT` | `InsertOperator` | Yes | Full (documents) | `InsertOptionsMapper` | Full | `InsertOperatorTest` (3 tests) | YAML `_0002`, `_0003`, `_0005` |
9290
| update | `UPDATE` | `UpdateOperator` | Yes | Full (filter, update) | `UpdateOptionsMapper` | Full | `UpdateOperatorTest` (1 test) | None |
93-
| delete | `DELETE` | `DeleteOperator` | Yes | filter required | None | Full | `DeleteOperatorTest` (1 test) | YAML `_0002` rollback |
94-
| createIndex | `CREATE_INDEX` | `CreateIndexOperator` | No | Full (keys) | `IndexOptionsMapper` | Warns & ignores | `CreateIndexOperatorTest` (1 test) | YAML `_0003`, `_0005` |
91+
| delete | `DELETE` | `DeleteOperator` | Yes | filter required, multi (opt) | None | Full | `DeleteOperatorTest` (5 tests) | YAML `_0002` rollback |
92+
| createIndex | `CREATE_INDEX` | `CreateIndexOperator` | No | Full (keys) | `IndexOptionsMapper` | Ignores (base class logs) | `CreateIndexOperatorTest` (1 test) | YAML `_0003`, `_0005` |
9593
| dropIndex | `DROP_INDEX` | `DropIndexOperator` | No | indexName or keys | None | **Ignores entirely** | `DropIndexOperatorTest` (1 test) | YAML `_0005` rollback |
9694
| renameCollection | `RENAME_COLLECTION` | `RenameCollectionOperator` | No | target required | `RenameCollectionOptionsMapper` | Ignores entirely | `RenameCollectionOperatorTest` (1 test) | None |
9795
| modifyCollection | `MODIFY_COLLECTION` | `ModifyCollectionOperator` | No | Full (validator, validationLevel, validationAction) | None | Ignores entirely | `ModifyCollectionOperatorTest` (1 test) | None |

src/main/java/io/flamingock/template/mongodb/mapper/MapperUtil.java

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@
1616
package io.flamingock.template.mongodb.mapper;
1717

1818
import com.mongodb.client.model.Collation;
19+
import com.mongodb.client.model.CollationAlternate;
20+
import com.mongodb.client.model.CollationCaseFirst;
21+
import com.mongodb.client.model.CollationMaxVariable;
22+
import com.mongodb.client.model.CollationStrength;
1923
import org.bson.BsonArray;
2024
import org.bson.BsonDocument;
2125
import org.bson.BsonValue;
@@ -88,9 +92,47 @@ public static Collation getCollation(Map<String, Object> options, String key) {
8892
Object value = options.get(key);
8993
if (value instanceof Collation) {
9094
return (Collation) value;
95+
} else if (value instanceof Map) {
96+
@SuppressWarnings("unchecked")
97+
Map<String, Object> map = (Map<String, Object>) value;
98+
return buildCollationFromMap(map);
9199
} else {
92-
throw new IllegalArgumentException(String.format("field[%s] should be Collation", key));
100+
throw new IllegalArgumentException(String.format("field[%s] should be Collation or Map", key));
101+
}
102+
}
103+
104+
private static Collation buildCollationFromMap(Map<String, Object> map) {
105+
Object locale = map.get("locale");
106+
if (!(locale instanceof String)) {
107+
throw new IllegalArgumentException("Collation requires 'locale' as a String");
108+
}
109+
Collation.Builder builder = Collation.builder().locale((String) locale);
110+
111+
if (map.containsKey("caseLevel")) {
112+
builder.caseLevel((Boolean) map.get("caseLevel"));
113+
}
114+
if (map.containsKey("caseFirst")) {
115+
builder.collationCaseFirst(CollationCaseFirst.fromString((String) map.get("caseFirst")));
116+
}
117+
if (map.containsKey("strength")) {
118+
builder.collationStrength(CollationStrength.fromInt(((Number) map.get("strength")).intValue()));
119+
}
120+
if (map.containsKey("numericOrdering")) {
121+
builder.numericOrdering((Boolean) map.get("numericOrdering"));
122+
}
123+
if (map.containsKey("alternate")) {
124+
builder.collationAlternate(CollationAlternate.fromString((String) map.get("alternate")));
125+
}
126+
if (map.containsKey("maxVariable")) {
127+
builder.collationMaxVariable(CollationMaxVariable.fromString((String) map.get("maxVariable")));
128+
}
129+
if (map.containsKey("normalization")) {
130+
builder.normalization((Boolean) map.get("normalization"));
131+
}
132+
if (map.containsKey("backwards")) {
133+
builder.backwards((Boolean) map.get("backwards"));
93134
}
135+
return builder.build();
94136
}
95137

96138
// Recursively converts a Map<String, Object> to BsonDocument

src/main/java/io/flamingock/template/mongodb/model/operator/CreateCollectionOperator.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,22 +18,15 @@
1818
import com.mongodb.client.ClientSession;
1919
import com.mongodb.client.MongoDatabase;
2020
import io.flamingock.template.mongodb.model.MongoOperation;
21-
import io.flamingock.internal.util.log.FlamingockLoggerFactory;
22-
import org.slf4j.Logger;
2321

2422
public class CreateCollectionOperator extends MongoOperator {
25-
protected static final Logger logger = FlamingockLoggerFactory.getLogger("CreateCollection");
26-
2723

2824
public CreateCollectionOperator(MongoDatabase mongoDatabase, MongoOperation operation) {
2925
super(mongoDatabase, operation, false);
3026
}
3127

3228
@Override
3329
public void applyInternal(ClientSession clientSession) {
34-
if (clientSession != null) {
35-
logger.warn("MongoDB does not support transactions for createCollection operation. Ignoring transactional flag.");
36-
}
3730
mongoDatabase.createCollection(op.getCollection());
3831
}
3932

src/main/java/io/flamingock/template/mongodb/model/operator/CreateIndexOperator.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,6 @@ public CreateIndexOperator(MongoDatabase mongoDatabase, MongoOperation operation
3030

3131
@Override
3232
protected void applyInternal(ClientSession clientSession) {
33-
if (clientSession != null) {
34-
logger.warn("MongoDB does not support transactions for createIndex operation. Ignoring transactional flag.");
35-
}
3633
IndexOptions indexOptions = IndexOptionsMapper.mapToIndexOptions(op.getOptions());
3734
mongoDatabase.getCollection(op.getCollection())
3835
.createIndex(op.getKeys(), indexOptions);

src/main/java/io/flamingock/template/mongodb/model/operator/DeleteOperator.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,24 @@ public DeleteOperator(MongoDatabase mongoDatabase, MongoOperation operation) {
3131
protected void applyInternal(ClientSession clientSession) {
3232
MongoCollection<Document> collection = mongoDatabase.getCollection(op.getCollection());
3333
Document filter = op.getFilter();
34+
boolean multi = op.isMulti();
3435

36+
if (multi) {
37+
deleteMany(clientSession, collection, filter);
38+
} else {
39+
deleteOne(clientSession, collection, filter);
40+
}
41+
}
42+
43+
private void deleteOne(ClientSession clientSession, MongoCollection<Document> collection, Document filter) {
44+
if (clientSession != null) {
45+
collection.deleteOne(clientSession, filter);
46+
} else {
47+
collection.deleteOne(filter);
48+
}
49+
}
50+
51+
private void deleteMany(ClientSession clientSession, MongoCollection<Document> collection, Document filter) {
3552
if (clientSession != null) {
3653
collection.deleteMany(clientSession, filter);
3754
} else {

src/main/java/io/flamingock/template/mongodb/validation/CreateIndexParametersValidator.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,16 @@
1919
import io.flamingock.template.mongodb.model.MongoOperation;
2020

2121
import java.util.ArrayList;
22+
import java.util.Arrays;
23+
import java.util.HashSet;
2224
import java.util.List;
2325
import java.util.Map;
26+
import java.util.Set;
2427

2528
public class CreateIndexParametersValidator implements OperationValidator {
2629

30+
private static final Set<String> RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("keys", "options"));
31+
2732
@Override
2833
public List<TemplatePayloadValidationError> validate(MongoOperation operation) {
2934
List<TemplatePayloadValidationError> errors = new ArrayList<>();
@@ -59,6 +64,8 @@ public List<TemplatePayloadValidationError> validate(MongoOperation operation) {
5964
"'options' must be a document"));
6065
}
6166

67+
errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "CreateIndex"));
68+
6269
return errors;
6370
}
6471
}

src/main/java/io/flamingock/template/mongodb/validation/CreateViewParametersValidator.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,16 @@
1919
import io.flamingock.template.mongodb.model.MongoOperation;
2020

2121
import java.util.ArrayList;
22+
import java.util.Arrays;
23+
import java.util.HashSet;
2224
import java.util.List;
2325
import java.util.Map;
26+
import java.util.Set;
2427

2528
public class CreateViewParametersValidator implements OperationValidator {
2629

30+
private static final Set<String> RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("viewOn", "pipeline", "options"));
31+
2732
@Override
2833
public List<TemplatePayloadValidationError> validate(MongoOperation operation) {
2934
List<TemplatePayloadValidationError> errors = new ArrayList<>();
@@ -56,6 +61,8 @@ public List<TemplatePayloadValidationError> validate(MongoOperation operation) {
5661
"'options' must be a document"));
5762
}
5863

64+
errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "CreateView"));
65+
5966
return errors;
6067
}
6168
}

src/main/java/io/flamingock/template/mongodb/validation/DeleteParametersValidator.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,16 @@
1919
import io.flamingock.template.mongodb.model.MongoOperation;
2020

2121
import java.util.ArrayList;
22+
import java.util.Arrays;
23+
import java.util.HashSet;
2224
import java.util.List;
2325
import java.util.Map;
26+
import java.util.Set;
2427

2528
public class DeleteParametersValidator implements OperationValidator {
2629

30+
private static final Set<String> RECOGNIZED_KEYS = new HashSet<>(Arrays.asList("filter", "multi"));
31+
2732
@Override
2833
public List<TemplatePayloadValidationError> validate(MongoOperation operation) {
2934
List<TemplatePayloadValidationError> errors = new ArrayList<>();
@@ -38,6 +43,16 @@ public List<TemplatePayloadValidationError> validate(MongoOperation operation) {
3843
"'filter' must be a document"));
3944
}
4045

46+
if (params != null) {
47+
Object multi = params.get("multi");
48+
if (multi != null && !(multi instanceof Boolean)) {
49+
errors.add(new TemplatePayloadValidationError("parameters.multi",
50+
"'multi' must be a boolean"));
51+
}
52+
53+
errors.addAll(OperationValidator.checkUnrecognizedKeys(params, RECOGNIZED_KEYS, "Delete"));
54+
}
55+
4156
return errors;
4257
}
4358
}

0 commit comments

Comments
 (0)