fix(mongodbw): wire semantics for null operators, nested $set, projection, regex, unique _id, replaceOne (#9061 #9060 #9063 #9064 #9068 #9069) - #9109

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Not up to standards ⛔

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Review

Solid PR: the six issues are addressed in a coherent way, the SQL translation is well documented, and the new MongoDBWireSemanticsTest goes end-to-end through the real Mongo driver. I read the full diff; I did not run the tests. Findings below, most important first.

Potential bugs

1. $set on a dotted path silently destroys non-document values (MongoDBDatabaseWrapper.setNested). If the container is a List and the next segment is not an index (e.g. {$set: {'tags.x': 1}} with tags: ['a','b']), the code falls through to copyOfEmbedded(container), which returns an empty map, so the array is replaced by {x: 1}. The same happens when the container is a scalar ({$set: {'name.first': 'x'}} on name: 'bob'). MongoDB rejects both with "Cannot create field 'x' in element {...}". Silent data loss on a write path is worse than an error here: please throw for a List with a non-index segment and for a non-null, non-embedded scalar. Same applies to $inc via setPath, and unsetNested has the mirror issue for scalars (it returns an empty map copy, wiping the scalar).

2. $not wrapping a null-matching operator (appendOperators). nullSensitive is true for everything except $exists, so {f: {$not: {$ne: 5}}} becomes (f IS NULL OR NOT (f IS NULL OR f <> 5)), which still matches a null field. MongoDB's $not: {$ne: 5} is equivalent to $eq: 5, so a null/missing field must NOT match. The same applies to $not: {$nin: [...]} and $not: {$in: [.., null]}. Rare in practice, but easy to get right by only adding the IS NULL OR guard when the operand has no null-matching operators (or by negating the inner expression's real three-valued result).

3. _id uniqueness vs. type coercion. The index is created with withDefaultKeyTypesForUndeclaredProperties(STRING), so _id: 1 and _id: "1" map to the same key and the second insert is rejected as a duplicate, while MongoDB treats them as distinct. Likewise an ObjectId is stored as its hex string, so a string _id equal to that hex collides. Probably acceptable as a known limitation, but worth a comment or a test documenting it.

4. Regex wrapping can be escaped by the user pattern (buildRegex). Only the wrapped string is compiled, so a pattern like a)|(b yields (?s:.*)(?u:a)|(b)(?s:.*), which compiles and silently changes the semantics (alternation outside the wrapper, and MATCHES then requires a full match). MongoDB would reject it as an invalid regex. Validating Pattern.compile(pattern, flagsAsInt) on the bare user pattern first (then wrapping) closes this and gives a better error message.

5. add() uses Math.addExact on longs: an overflow throws ArithmeticException, whereas MongoDB's $inc promotes to double. Minor; falling back to the double sum on overflow would match.

Performance / GC (per CLAUDE.md)

• executeUpdateOnRecords materializes every matching record (modify() copies) into a List before updating. For updateMany/replaceOne with a broad filter on a large collection this is unbounded heap. Consider processing in bounded chunks, or at least keeping the single SQL UPDATE for the common case. Note that any $set containing a dotted key now takes this slow path for the entire update, even when the other operators could stay SQL.
• The checkByHand path in insertDocuments runs one SELECT ... WHERE _id = :id per document. If the type has no index on _id that is a full scan per inserted document (O(n^2) for a bulk load). The fallback is reasonable for correctness, but a once-per-batch pre-collection of ids, or at least a log line at the point the fallback is chosen, would help operators understand a slowdown. Also check that the in-batch case (two equal _id in the same batch with no index) is detected: the query may not see uncommitted records from the same transaction when no index backs it.
• On a duplicate, the batch is rolled back and then re-inserted one document per transaction. For a 10k-document batch with a single duplicate near the end that is ~10k commits. Acceptable as a slow path for an error case, but worth noting; an unordered batch could instead be retried skipping only the offending index when the exception identifies it.
• ensureIdIndex on first insert into a pre-existing, populated type builds the index synchronously inside the insert request. Fine functionally, but a large collection will stall that first insert. Collections created through upsert (getOrCreateDocumentType in executeUpsert) bypass ensureIdIndex entirely and stay on the by-hand path (the wrapper is cached, so getOrCreateCollection never revisits it).
• Pattern.compile(wrapped) is executed on every translation just for validation, in addition to the engine's own compile. Cheap, but it disappears if you validate the bare pattern once as suggested above.

Behaviour changes worth calling out in the description

• insert now defaults to ordered=true when the option is absent (correct per MongoDB, but it is a behaviour change for clients relying on the old default).
• numIndexesBefore/After accounting changed (countIndexes); good that it handles both cases.
• Top-level calls of buildEquality/$in/$nin with field == null would emit the literal text null IS NULL (they append field directly instead of appendField). Not reachable today as far as I can tell, but inconsistent with the other operators.

Style / conventions

• com.arcadedb.database.Document is used fully qualified in MongoDBDatabaseWrapper (getPath, copyOfEmbedded); CLAUDE.md asks for imports. It clashes with the Mongo Document, so that is understandable, but a comment or a static helper in one place would be tidier.
• Several new locals/params are not final where they could be (e.g. n is fine, but nullSensitive-style flags and loop vars are mostly OK; just a skim for consistency).
• Dead code removal (the commented-out ProjectingIterable) is welcome.

Tests

Good coverage of the happy paths for all six issues, and the existing translator unit tests were updated consistently. Missing cases I would add:

• $set/$inc/$unset through an array index ('a.0.b'), and the error behaviour for $set into a scalar/array with a non-index key (item 1).
• $in: [null] alone, $nin: [null] alone, and $not of $ne/$in (item 2).
• replaceOne with the same _id in the replacement (allowed) and with a different one asserting the actual error code 66 / ImmutableField rather than just RuntimeException.
• A type created through SQL (no _id index) to exercise the checkByHand path, including a duplicate inside one batch.
• Invalid regex / unknown option ($options: 'z') returning an error, and $regex + $options ordering variants.
• Projection: dotted paths ('addr.city': 1) and the invalid mix rejected on an empty result set.
• assertThatThrownBy(...).isInstanceOf(RuntimeException.class) is very weak in the projection and replaceOne tests; assert on MongoCommandException/error code instead.

None of these look merge-blocking except item 1 (silent data loss), which I would fix before merging. Nice work overall.

Review of #9109 (mongodbw wire semantics)

Nice, focused set of fixes, and the test class is well organised (each expected answer is tied to MongoDB behaviour). The null-aware $ne/$nin/$in/$not translation, the copy-on-write nested $set, and the "validate the user's pattern alone before wrapping" regex check are all solid. I read the diff only (did not build or run the tests). Findings below, most important first.

Potential bugs

1. Standalone $unset/$inc on a dotted path still goes through the SQL path. executeUpdate routes to executeUpdateOnRecords only when setsDottedPath(u) is true, and that helper checks $set alone (MongoDBDatabaseWrapper.java, setsDottedPath). {$inc: {"addr.n": 1}} or {$unset: {"addr.zip": ""}} without a dotted $set is therefore handled by appendUpdateOperations (REMOVE/SET ... +=), not by the new setPath/unsetPath/add code. unsetAndIncOnDottedPaths only passes because it combines all three operators with a dotted $set. This matters beyond dotted paths: the two paths now behave differently for $inc (the record path keeps int/long, the SQL += path does not and cannot create missing nested paths). Suggest checking $set, $unset and $inc for dotted keys, or routing every non-replacement update with a dotted key through the record path, and adding tests for $inc-only and $unset-only on a dotted path (including a missing parent, e.g. {$inc: {"a.b": 1}} on {}).

2. Unbounded array padding in setNested (memory DoS). isArrayIndex accepts up to 9 digits, and setNested does while (copy.size() <= index) copy.add(null). {$set: {"tags.999999999": 1}} on an existing array tries to allocate ~1e9 list entries in the server JVM, and the result would also exceed the max record size. Any user with write access can trigger this. Please cap the index (MongoDB itself rejects very large padding) and return a MongoServerError instead.

3. Misleading E11000 for any DuplicatedKeyException. insertDocument reports every ErrorCategory.DUPLICATED_KEY as index: _id_ dup key: { _id: ... }. If the collection also has a user-defined unique index on another field, a violation on that index is reported as an _id duplicate (with the wrong key). DuplicatedKeyException carries the index name and key, so the message could be built from those. Also, doc.get("_id") is printed raw, so strings appear unquoted ({ _id: abc }, MongoDB prints { _id: \"abc\" }).

4. ensureIdIndex on a pre-existing, possibly large, collection. On the first insert into a cached-miss collection that already exists (made through SQL/Studio, or after a restart), getOrCreateCollection calls ensureIdIndex, which synchronously builds a unique LSM index over all existing records inside the insert request. For a big collection that is a long, surprising stall on a write (and a schema change taking locks). Worth either documenting, doing it off the hot path, or at least logging at INFO when it starts. The WARNING fallback on duplicates is good.

5. Collapsed key types in the new unique index (already noted in the code comment): _id: 1 and _id: \"1\" (and an ObjectId vs its hex string) are the same key, so inserting the second is now rejected with E11000, which real MongoDB would accept. The comment is honest about it; I would still pin it with a test so a later change is deliberate.

6. checkByHand path is a check-then-act. checkIdIsFree is racy under concurrent inserts, and it queries before the pending records of the same batch are saved, so two identical _ids in one batch may not be caught there (the unique-index path does catch them, the by-hand path might not). In practice ensureIdIndex makes the by-hand path rare (only types that already hold duplicates), but a test covering a duplicated batch on such a type would confirm the behaviour.

Smaller points

• buildRegex: validating the user's pattern alone is good, but a pattern that enables comments mode inline (e.g. (?x) a # note) is valid on its own, then swallows the closing )(?s:.*) once wrapped, producing a compile error at query time rather than a clean 'invalid regex' error. Compiling the wrapped string as well (or appending the newline whenever the pattern contains (?x/(? flag groups with x) would close this.
• The wrapper (?s:.*)pattern(?s:.*) makes MATCHES effectively unanchored-but-backtracking, so non-matching long strings cost O(n^2) for simple patterns. The Javadoc says evaluation time is bounded by the engine; it would be good to confirm that is really enforced for MATCHES (the comment is a security claim).
• appendOperators with field == null (a top-level {$regex: ...} or {$ne: ...} document) emits an operator with no left-hand side, i.e. invalid SQL surfacing as a parse error. Previously similar, but now that field is explicit a clear IllegalArgumentException would be cheap.
• $inc casts with (Number): a non-numeric stored value or operand gives a ClassCastException (surfaced as a generic error) where MongoDB returns a type error (code 14).
• replaceContent compares ids with Objects.equals, so Integer 1 vs Long 1 or 1 vs 1.0 is reported as an altered _id; MongoDB compares numerically.
• insertDocument's fallback commits one document per transaction after the batch fails. Fine for correctness; just note a 10k batch with one duplicate at the end becomes 10k commits.
• getOrCreateCollection now calls existsType on every insert, and hasUniqueIdIndex iterates all indexes per insert. Cheap, but it is on the hot path; consider caching the 'has id index' flag in the wrapper.
• Per CLAUDE.md, prefer imports over fully qualified names: com.arcadedb.database.Document is used fully qualified several times in MongoDBDatabaseWrapper (it clashes with the Mongo Document, so an alias is not possible, but a short comment or a helper isEmbedded(Object) would remove the repetition). Also final on the remaining non-final locals in buildRegex's flags loop is fine, no change needed.

Tests

Good coverage of the happy paths. Missing, in my view:

• standalone dotted $inc / $unset (see build support #1), and $inc int vs double result types
• $set on a huge array index (see Bump gremlin.version from 3.4.10 to 3.5.1 #2)
• default (omitted) ordered flag, which this PR changes from unordered to ordered; the duplicatedIdIsRejected test passes ordered only implicitly through the driver defaults, so an explicit raw-command test would pin the flag handling
• a user-defined unique index violation on a non-_id field (see Bump slf4j-simple.version from 1.7.31 to 1.7.32 #3)
• $options without $regex and $not nested in $not error cases, plus a regex with the inline (?x) flag
• a projection with $slice/$elemMatch (currently an IllegalArgumentException; assert the client-visible error is clean)
• the MongoDBToSqlTranslatorParamsTest updates are mechanical and fine; consider one extra translator unit test for $in: [null] / $nin: [null, x] SQL shapes

Overall the direction is right and the semantic fixes look correct. I would address #1 and #2 before merging and consider #3/#4; the rest are polish.

Review of #9109 (mongodbw wire semantics)

Good, well-scoped fixes. The null-aware $ne/$nin/$in/$not translation, the copy-on-write nested $set, the MAX_ARRAY_PADDING guard, validating the user's regex on its own before wrapping, and the production-mode concealment of duplicate keys are all careful. Earlier review rounds look addressed (setsDottedPath now covers every operator, requireEmbedded refuses writing below a scalar). I read the diff and checked a couple of engine details, but did not build or run the tests. Remaining points, most important first.

Potential bugs / behavior gaps

1. replaceContent compares _id with Objects.equals (MongoDBDatabaseWrapper.replaceContent). A document stored with _id: 2 (Integer) replaced by one carrying _id: 2L (or 2.0) is refused with ImmutableField, while MongoDB compares numerically and accepts it. Drivers produce Long/Integer differences easily (e.g. a Java long id). Consider a numeric-aware comparison for Number ids.
2. $inc on a non-numeric value does (Number) getPath(...) / (Number) f.getValue(), so a string field or operand gives a ClassCastException instead of MongoDB's "Cannot apply $inc to a value of non-numeric type" (code 14). Worth a MongoServerError like the other path errors.
3. buildEquality appends field unconditionally (MongoDBToSqlTranslator.buildEquality), while the rest of the code goes through appendField, which tolerates null. A top-level {$eq: ..}/{$ne: ..} (field is null) would emit the literal text null into the SQL. It is rejected later by the parser, but a clear IllegalArgumentException (as appendOperators does) would be better than a SQL parse error.
4. Hand-checked path is O(n) per inserted document and the "no index" fallback is untested. checkIdIsFree runs select ... where _id = :id per document; without an index that is a full scan, so a batch of N into a type that could not get the index (the only case where checkByHand is true) is O(N * size). That is acceptable as a rare fallback, but nothing in MongoDBWireSemanticsTest exercises it: duplicatedIdOnATypeCreatedBySql ends up with a real index because ensureIdIndex succeeds. A test that pre-seeds duplicate _id values through SQL (so the index build fails), then inserts a duplicate, would cover the checkByHand branch, including a duplicate inside one batch.
5. Batch retry cost. On a duplicate in the fast path, the whole batch is rolled back and then re-inserted one document per transaction. A 10k-document insertMany with one duplicate at the end becomes ~10k commits. Reasonable for correctness, but worth a comment noting the trade-off.
6. ensureIdIndex on first access of a large pre-existing type builds a unique index synchronously inside the first insert request. Fine functionally, but it can stall that request for a long time on a big SQL-created type. Maybe worth a log line when the type is non-empty.
7. ordered default change (query.get("ordered") == null || ...) is correct per MongoDB, but it is a behavior change for any client that relied on the old default. A one-line mention in the PR description would help.

Memory / performance

• executeUpdateOnRecords materializes every matching MutableDocument in a list before modifying (updateMany with a dotted path on a big collection holds them all on heap). The old single SQL UPDATE did not have this cost. Processing in bounded chunks would keep the heap flat.
• The regex wrapper (?s:.*)(?flags:pattern)(?s:.*) is quadratic on non-matching long strings by construction. The MATCHES regex deadline (confirmed in MatchesCondition) bounds it, so this is OK; just worth knowing for large text fields.

Style / CLAUDE.md conventions

• new com.mongodb.client.model.IndexOptions() in duplicateOnAUserDefinedUniqueIndexIsNotReportedAsId uses a fully qualified name; import it. In main code com.arcadedb.database.Document is qualified several times in MongoDBDatabaseWrapper - understandable because of the clash with the Mongo Document, but a small private helper (isEmbedded(Object)) would remove the repetition.
• Import order: com.arcadedb.server.ArcadeDBServer sits between com.arcadedb.schema.* imports in MongoDBDatabaseWrapper; MongoQueryException is out of order in the new test; MongoDBCollectionWrapper imports schema.* before query.*.
• Codacy flags one minor code-style issue; worth a look.
• Removing the commented-out ProjectingIterable code is a welcome cleanup.

Tests

MongoDBWireSemanticsTest is end-to-end with the real driver and covers all six issues well. Gaps worth adding:

• the checkByHand branch (point 4), including a duplicate inside a single batch on a type with no index;
• replaceOne with a numerically-equal but differently-typed _id (point 1);
• $inc on a string field (point 2);
• production-mode concealment of the duplicate key in the E11000 message (isProductionMode);
• $ne / $nin inside $or and nested $and combinations.

The *ParamsTest expectations were updated consistently with the new SQL shape.

No security problems spotted: regexes are validated and time-bounded, the array padding is capped, and the duplicate-key message respects production mode.

Review

Solid piece of work: the six fixes are well scoped, the translator changes are carefully reasoned (null-aware $ne/$nin/$in/$not, $options consumed with $regex, validating the user pattern on its own before wrapping), copy-on-write for embedded maps, array padding cap, and the test class is broad. Comments below, most important first.

Worth verifying before merge

1. _id index uses Type.STRING keys (MongoDBCollectionWrapper ensureIdIndex). The comment already admits _id: 1 and _id: "1" (and an ObjectId vs. its hex string) collapse into one key, so Mongo-valid inserts now get a spurious E11000. The bigger question is whether the planner now picks this index for range/sort predicates on _id ({_id: {$gt: 5}}, sort({_id: 1})) and then compares lexicographically. All tests use single-digit ids, so "10" < "5" style bugs would not show. Please add a test with numeric ids such as 2, 9, 10, 100 covering $gt/$lt and sort on _id. If it misbehaves, a $gt on _id needs to avoid the index, or the index key type must follow the data.
2. Creating the index on first insert into an existing type. For a SQL/Studio-created type, the first Mongo insert synchronously builds a unique index over all existing records (potentially long, blocking that request), and silently changes a user-owned schema. If the build fails (duplicates), the fallback checkIdIsFree runs one select ... where _id = :id per inserted document with no index, so a bulk insert becomes O(n*m). Consider logging at a higher level once, and/or de-duplicating against an in-memory set for the batch plus a single IN lookup, rather than a query per document.
3. Fast-path-then-retry insert (insertDocument). A batch of N documents with one duplicate is rolled back and then re-inserted as N single-document transactions. For large insertMany calls that is a sharp performance cliff, and it also isn't atomic against concurrent writers between the rollback and the retry. For the ordered case you could commit the prefix before the first failing document instead of restarting from zero. Also, ordered now defaults to true when absent (correct per Mongo, but a behavior change worth a line in the PR description).

Performance

4. executeUpdateOnRecords materializes every matching record in a List<MutableDocument> before modifying. updateMany with a dotted $set on a large collection will hold the whole result in memory. Processing in bounded batches (or collecting only RIDs and loading per record) would keep the GC footprint low, per the CLAUDE.md guidance.
5. Regex wrapper (?s:.*)(?flags:pattern)(?s:.*) with MATCHES: the leading .* forces a retry at every start offset, so matching is O(n^2) for non-matching long strings, and patterns can never use an index. The Javadoc says MATCHES "bounds the evaluation time"; please point to where that is enforced (I could not confirm it from the diff), or add a test with a pathological pattern. The pattern is also compiled twice per translation (the second compile of wrapped is mostly redundant given the first one validates the user pattern; keep it only for the (?x) case).
6. $ne/$nin/$not now emit IS NULL OR ..., which defeats index use for those predicates. That follows from the Mongo semantics, but worth noting in the PR.

Correctness / consistency

7. $inc on a non-dotted path still goes through the SQL UPDATE path while dotted paths use the new add() (integral stays integral). Make sure both produce the same type ($inc: {n: 1} on an int vs $inc: {'a.n': 1}); the setsDottedPath Javadoc claims consistency only "however they are combined", not across the two paths. A test comparing both would settle it.
8. In the regex branch, when the value is a BsonRegularExpression and $options is also present, the sibling $options is silently ignored.
9. $set on _id through applyOperatorsToDocument is not checked for immutability, unlike replaceContent. Mongo rejects changing _id with $set too.
10. ensureIdIndex is called on every upsert update batch (updateDocuments) even when the type already has the index; it is cheap, but it iterates getAllIndexes(false) each time. Fine, just noting.

Style / tests

11. Import order: com.arcadedb.server.ArcadeDBServer is inserted between schema imports in MongoDBDatabaseWrapper, and the new schema imports precede query.sql in MongoDBCollectionWrapper.
12. appendOperators: field == null throws at the top, so the later nullSensitive && field != null checks are redundant.
13. Fully qualified com.arcadedb.database.Document appears several times in MongoDBDatabaseWrapper (needed because of the Mongo Document import clash, so acceptable, but an alias-free helper like isEmbedded(Object) would remove the repetition).
14. Tests: please add coverage for (a) the _id int/string collision and range/sort behavior from point 1, (b) a multi-document updateMany with a dotted $set, (c) $not: {$regex} with $options, (d) a duplicate inside an unordered batch reporting the right index in the write error. MongoDBWireSemanticsTest could also tag nothing as slow, which looks right.

No security problems spotted: the regex is validated, the array padding is capped, the duplicate-key message honors production-mode concealing, and values are bound as parameters.

I could not run the build here; the PR reports the module's 151 tests passing.

Review of #9109 (mongodbw wire semantics)

Overall this is solid work. The null-aware $ne/$nin/$in/$not translation is careful (isTwoValued avoids redundant IS NULL OR), the regex wrapper validates the user's pattern on its own before wrapping it (so a)|(b cannot escape), the $set array padding is bounded, key values are concealed in production mode, and the new MongoDBWireSemanticsTest covers a lot of ground. I did not run the build; this is a read of the diff. Points below are roughly in priority order.

Potential bugs / risks

1. Index drop and rebuild is not safe under concurrency (MongoDBCollectionWrapper.ensureIdIndex, ~L172-180). When an insert introduces a mixed _id kind, the unique index is dropped and recreated with STRING keys. Another connection inserting at the same moment can pass ensureIdIndex, observe hasUniqueIdIndex == true, and then have its transaction run while the index is gone (no uniqueness check, since checkByHand is already false). Schema changes while other threads have open transactions on the same type are also something I would want confirmed as safe. Suggest serializing ensureIdIndex per collection (a lock on the wrapper or a synchronized block on the type name), and ideally re-evaluating checkByHand after the lock is released. A comment or test documenting the intended behaviour would help.

2. Numeric equality of _id is not honoured once the index is STRING (ensureIdIndex). After the downgrade to STRING keys, 2 and 2.0 / 2L become different keys ("2" vs "2.0"), while sameId in replaceContent treats them as equal, and MongoDB compares them by value. This is a narrow edge case and probably acceptable, but the Javadoc says only that ordering is lost; uniqueness semantics are also affected and could be mentioned.

3. Insert fallback cost on a duplicate (MongoDBDatabaseWrapper.insertDocument). One duplicate in a large insertMany rolls back the whole batch and then replays it one document per transaction (N commits, N ensureIdIndex calls and, without an index, N lookups). Correct, but a bulk load with a few duplicates becomes much slower. A cheaper variant is to identify the failing index from the exception and re-run the batch in chunks, or to commit the prefix before the failing document for the ordered case. At least worth a comment on the cost.

4. DuplicatedKeyException may surface at commit rather than at save(). The fast-path catch handles both because it wraps insertDocuments, which is good, but ErrorCategory.of(e) is only checked on the top-level exception, while duplicateKeyMessage walks the cause chain. If a commit wraps the duplicate (e.g. in a TransactionException), ErrorCategory.of may not classify it as DUPLICATED_KEY and it would be rethrown as a generic error instead of a writeError. Please confirm ErrorCategory.of also looks at causes, or reuse the same cause-walking helper.

5. Upsert without _id in the filter (updateDocuments). ensureIdIndex(..., q.get("_id") or null) does nothing for a null sample, so a collection populated only through upserts that omit _id never gets the unique index (and executeUpsert does not seem to check by hand). A later insertOne creates it, but until then the guarantee from MongoDB wire: insertOne and insertMany with an _id that already exists succeed and store a second document with the same _id (no E11000) #9060 is not in place. Probably fine if generated _ids are unique by construction; worth a note.

6. $set with a dotted path now routes all operators through applyOperatorsToDocument. Today that supports only $set/$unset/$inc, which matches appendUpdateOperations, so no regression. Just be aware that the non-dotted SQL path (SET x += ) and the record path (add()) can now disagree on result types (the SQL path may produce a double or keep the type depending on the engine). A test asserting $inc on an int field stays an int through both paths would pin this.

7. isInclusionProjection: MongoDB accepts {_id: 1, a: 0}? Not exactly, but it does accept {_id: 0, a: 1} and {_id: 1, a: 1}, which are handled. {a: 1, "b.c": 0} correctly throws. No bug found, but projectField does not descend into arrays of embedded documents ({ "items.n": 1 } on items: [{n:1}]), so that projection silently returns nothing for items. Probably worth a TODO or a test documenting the limitation.

Regex translation

• (?s:.*)(?flags:pattern)(?s:.*) is correct for find-anywhere semantics, but it makes every match at least O(n) in the prefix before the user's pattern even starts, and it defeats any index use. Fine for a compatibility layer; the Javadoc claims that MATCHES bounds evaluation time, which I could not verify from the diff. Please confirm that claim (a pointer to where the timeout is enforced would be enough) since it is the ReDoS mitigation.
• A pattern containing an unterminated \Q compiles on its own but fails once wrapped; it is rejected with a clean error, which is acceptable.
• The second Pattern.compile(wrapped) comment ("an inline (?x) in the pattern itself would swallow the closing parenthesis") sits above a try that compiles the wrapper, and the comment about comments mode sits above the first block; the two comments look swapped. Minor readability point.

Style / conventions (CLAUDE.md)

• Fully qualified com.arcadedb.database.Document is used several times in MongoDBDatabaseWrapper. I understand it collides with the Mongo Document import, so it is acceptable, but a single private static boolean isEmbedded(Object) helper would remove four repeats of instanceof Map || instanceof com.arcadedb.database.Document.
• import com.arcadedb.server.ArcadeDBServer; was inserted between the schema imports; please sort it.
• createCollection now extracts collectionName but does not use it for anything new. Either create the _id index there (the PR description says "created on first insert" only, but MongoDB's createCollection also yields an _id index, and createIndexes already special-cases the count) or revert that cosmetic change.
• MAX_ARRAY_PADDING is declared in the middle of the helper methods; move it to the top with the other constants.
• ensureIdIndex(Database, String, Object) overload looks used only by upsert; fine, but the ids.isEmpty() check could be moved before the loop.
• The large block of commented-out ProjectingIterable code was removed, which is good.

Tests

Good coverage of the six issues. Gaps I would add:

• Mixed _id kinds in one batch / across batches (the STRING rebuild path), including that uniqueness still holds afterwards.
• Production mode concealment of dup key in the E11000 message (the isProductionMode hook is new).
• Upsert path with and without _id in the filter on a fresh collection.
• Default ordered behaviour when ordered is omitted (the default flipped to ordered, which is a behaviour change worth an explicit test, since the Java driver sends it but other drivers may not).
• updateManyWithDottedSetAndDuplicateInUnorderedBatch: the try/catch has no fail() if the exception is not thrown. The final countDocuments == 4 would catch that indirectly, but an explicit assertThatThrownBy is clearer.
• A concurrency test for the first insert into a fresh collection from several clients (relates to point 1).

None of the above blocks the merge in my view; items 1 and 4 are the ones I would resolve or explicitly confirm before merging.

Review of #9109 (mongodbw wire semantics)

Overall this is solid work. The null-aware $ne/$nin/$in/$not translation is carefully reasoned, the regex wrapping validates the user pattern separately from the wrapped one (good catch on a)|(b), the heap-exhaustion guard on $set array padding is welcome, and MongoDBWireSemanticsTest covers a lot of ground with real driver round-trips. I could not run the build here, so the comments below come from reading the diff. I found no blockers, but a few things deserve attention before merge.

Potential bugs / correctness

1. Upsert can needlessly rebuild the _id index as STRING (MongoDBDatabaseWrapper.updateDocuments, ~L628-636).
   The sample passed to ensureIdIndex is q.get("_id") or new ObjectId(). Two problems:

   • If the filter's _id is an operator document ({_id: {$in: [1,2]}}, {_id: {$gt: 5}}), idKeyType returns null, which is treated as "mixed", so a numeric collection has its LONG index dropped and rebuilt as STRING. That silently turns {_id: {$gt: 5}} and sort({_id: 1}) into lexicographic ordering (the exact problem the Javadoc says the typed index avoids).
   • A filter without _id ({name: 'x'} with upsert: true) uses an ObjectId sample, so the rebuild to STRING happens eagerly even when the update matches an existing document and no ObjectId is ever inserted. It is also an O(n) drop and recreate triggered by an update request.
     Suggest: skip the sample when _id is a Document with $-keys (or use $eq's value), and do the rebuild lazily only when executeUpsert really inserts. A regression test for "numeric collection + upsert without _id keeps numeric $gt/sort on _id" would pin this.

2. Mixed-kind collections produce false duplicates. After the fallback to STRING keys, _id: 1 and _id: "1" map to the same key and the second insert fails with E11000, while MongoDB treats them as distinct. The Javadoc mentions 2 vs 2.0 but not this case. Worth documenting at least, or falling back to the by-hand check for mixed kinds instead of a lossy string index.

3. Javadoc and code disagree for unsupported _id kinds. The class comment says a collection whose _id is of another kind gets no index and is checked by hand. In ensureIdIndex, type == null sets mixed = true, so keyType becomes Type.STRING and an index is created (for a Boolean/Date/embedded _id, the string conversion is whatever the engine does). The if (keyType != null) guard at the end is therefore dead (ids is non-empty there, so keyType is never null). Please align the code and the comment, and add a test with a non-scalar _id.

4. Concurrency around the index rebuild. ensureIdIndex is static synchronized, which only serialises callers of that method, not the inserts that follow it. Thread A can pass ensureIdIndex and begin() while thread B drops and recreates the index (mixed kinds) before A commits, so A's transaction runs against a dropped or rebuilt index. Also, the lock is class-wide, so it serialises every database and collection on the server, and it is held during the O(n) rebuild. Consider a per-database/per-type lock that also covers the insert, or at least document the race.

Performance

5. Insert hot path overhead. Every insertDocuments call now allocates an ids list, takes a global monitor, iterates getAllIndexes(false) (twice, once in ensureIdIndex and once for hasUniqueIdIndex), and then does a getType. Per CLAUDE.md's performance mantra, a cheap fast path would help: cache "index verified for this type with key type X" (e.g. in MongoDBCollectionWrapper) and only re-run the full check if a document's _id kind differs from the cached one. This also removes the global lock from the common case.

6. checkIdIsFree is a full scan per document for a type without the index (select @rid ... where _id = :id). A batch of N inserts into a large unindexed collection costs N full scans. This is the fallback path so maybe acceptable, but a one-line comment on the cost, or a single _id IN [...] query per batch, would be better.

7. executeUpdateOnRecords materialises every matching record (multi: true with a dotted $set, e.g. updateMany({}, {$set: {'a.m': 5}})). On a large collection this loads all MutableDocuments into memory before saving. Consider processing in chunks, or collect only RIDs and load/modify/save one at a time. It is also a behaviour change from the previous single SQL UPDATE, which was streamed.

8. Insert error path: on a duplicate in a batch, the whole batch is rolled back and then replayed one document per transaction. For a large insertMany with a late duplicate this doubles the work and issues N commits. That is probably fine for correctness, but worth a note.

Style / minor

• MAX_ARRAY_PADDING check is index > MAX_ARRAY_PADDING, so an index of 100,001 elements is allowed. Use >= if the limit is meant as a size cap.
• Fully qualified com.arcadedb.database.Document appears in getPath, copyOfEmbedded and isEmbedded; CLAUDE.md asks to import and use the short name. The name clash with de.bwaldvogel...Document makes this understandable, so a short comment or a static helper would do.
• buildRegex has two overlapping comments about the x flag ("in comments mode ... hence the line break" and "in comments mode (x), also set inline..."). The first one appears stale or redundant, please merge them.
• setsDottedPath routes any operator document with a dotted key to the record path, including operators the SQL path would reject; both throw UnsupportedOperationException today, so fine, but keep this in mind if $push/$rename support is added.
• ensureIdIndex(Database, String, Object) overload looks used only from the upsert path; consider removing it if item 1 is reworked.
• Good removal of the dead commented-out ProjectingIterable code.

Test coverage

Very good for the six issues. Gaps I would add:

• upsert (with and without _id, and with an operator _id) on a numeric-_id collection keeps numeric ordering (item 1);
• _id: 1 then _id: "1" behaviour (item 2);
• a concurrent insertMany test with mixed _id kinds on a fresh collection (item 4);
• $in with null plus other values inside $not and $nor-like nesting, and $regex on a missing field under $not (covered partly by regularExpressions, but isTwoValued has several branches that are only exercised indirectly);
• $size/array fields with $ne ({tags: {$ne: 'a'}} on an array property) since <> on a list is the pre-existing semantic and the new IS NULL OR wrapper does not change it, but a test would document it.

Also, MongoDBToSqlTranslatorParamsTest only updates the $nin/$not expectations; adding unit-level assertions for the new $ne, $in-with-null and $regex SQL shapes would make regressions cheaper to localise than the wire tests.

Nice improvement overall; items 1, 3 and 4 are the ones I would fix before merging.

🤖 Generated with Claude Code

Review of #9109 (mongodbw wire semantics)

I reviewed the diff by reading it. I did not build the branch or run the tests. The scope is large but each of the six fixes is well isolated. The SQL is built from quoted field paths and bound parameters, so there is no injection risk. The regex handling validates the user pattern on its own and then the wrapped one, which is careful. The production-mode concealment of duplicate keys in the E11000 message is a good touch.

Issues worth addressing

1. A failed _id index build is retried on every insert (performance).
createIdIndex catches the failure when the collection already holds duplicate _id values, logs a warning and carries on. Afterwards idIndexSatisfies keeps returning false, because no unique _id index exists. So every later insertDocuments on that collection:

• takes the exclusive write lock,
• re-runs ensureIdIndex, which scans the collection and fails again,
• logs another WARNING.

The dups scenario in duplicatedIdOnATypeThatCannotGetTheIndex hits this path, but the test only checks correctness. Suggested fix: remember the failure per collection, for example a "gave up" marker next to ID_INDEX_LOCKS, and let idIndexSatisfies return true once the by-hand check is the chosen strategy. A partially built index may also be left behind after a failure, so please check that createIdIndex cleans up.

2. The _id index lock only covers inserts and upserts.
ensureIdIndex can dropIndex and recreate the index when the key type widens, for example LONG to STRING or DOUBLE. Concurrent find, update and delete calls do not take the lock, so they can run against an index that is being dropped. concurrentFirstInsertsWithMixedIdKinds only races inserts. Either document that this window exists or have readers take the shared lock. At minimum, add a test with a concurrent read during widening.

3. Lock map is never cleaned.
ID_INDEX_LOCKS is static and keyed by dbPath/collection. It grows with every collection ever touched and is not removed on drop collection or drop database. This is small but it is a leak in a long-lived server. A per-wrapper lock, or removal on drop, would avoid it.

4. False duplicates from the key-type widening (behavior to be aware of).

• Mixed numeric ids use a DOUBLE key, so two distinct longs above 2^53 collide.
• Mixed kinds use a STRING key, so {_id: 1} and {_id: "1"} are the same key and the second insert fails with E11000. MongoDB treats them as different.

Your Javadoc already admits both, so this is acceptable as a tradeoff, but it deserves a line in the PR description and a test pinning the intended behavior. For _id values that are neither string, ObjectId nor number (embedded document, UUID, Date), idKeyType falls to STRING. Please check that the index conversion of those values is stable, and add a test for one of them.

5. Unbounded memory in executeUpdateOnRecords.
A multi: true update on a dotted path, or a multi replacement, collects every matching record into a List<MutableDocument> before modifying anything. On a large collection that is a heap spike, and CLAUDE.md asks for care about GC pressure. Consider iterating and saving in batches, or streaming the result set and modifying records within it if the engine allows it.

6. Projection edge case.
isInclusionProjection treats {a: 0, _id: 1} as an invalid mix, because _id: 1 counts as inclusion while a: 0 counts as exclusion. As far as I know MongoDB accepts _id: 1 inside an exclusion projection. Worth checking against a real server. Array-of-subdocument paths such as {'items.n': 1} are also silently ignored by projectField and removeField, since they only descend into Document.

Smaller points

• insertDocument now defaults ordered to true when absent. This matches MongoDB but is a behavior change for existing clients. Please mention it in the PR description.
• The by-hand duplicate check (checkIdIsFree) queries the database per document, which is a full scan when no index exists. It also relies on the query seeing uncommitted records of the same transaction, to catch duplicates inside a single batch. duplicatedIdOnATypeCreatedBySql covers this today, but it is worth a comment.
• CLAUDE.md style:
  • MongoDBWireSemanticsTest.concurrentFirstInsertsWithMixedIdKinds uses fully qualified java.util.concurrent.* names. Import them instead.
  • The imports added to MongoDBCollectionWrapper are out of order (java.util.concurrent.* sits between ArrayList and HashMap).
  • Several tests only assert isInstanceOf(RuntimeException.class). For example replaceOneCannotChangeTheId could assert the ImmutableField code (66) via MongoCommandException.
• The Javadoc says the engine's MATCHES bounds evaluation time against hostile patterns. Please confirm that holds for the (?s:.*)pattern(?s:.*) shape, because the leading .* plus a backtracking user pattern is a classic ReDoS amplifier. A test with a catastrophic pattern would pin it. Per CLAUDE.md, use StallAwareStopwatch.assertGaveUpWithin, not raw wall-clock.

Test coverage

Coverage is broad and uses the project's AssertJ style, which is good. Gaps:

• production-mode concealment of the dup key text,
• $options without $regex,
• $nin: [],
• an unordered batch that mixes a duplicate with a non-duplicate failure,
• the retry-cost scenario from item 1,
• a concurrent reader during an index rebuild (item 2).

I would not merge before items 1 and 2 are addressed or explicitly accepted. The rest are follow-ups.

🤖 Generated with Claude Code

Review of #9109 (mongodbw wire semantics)

Overall this is a solid, well-reasoned change. The null-aware $ne/$nin/$in/$not translation, the copy-on-write nested $set/$unset/$inc, the immutable _id on replace and the projection rewrite are all clear, and MongoDBWireSemanticsTest is thorough (concurrency, failed index build, mixed id kinds, huge array index, user-defined unique index, comments-mode regex). Nothing below is a blocker; these are things worth considering.

Potential issues

1. Unbounded memory in executeUpdateOnRecords (MongoDBDatabaseWrapper). For updateMany/deleteMany-style traffic with a dotted $set/$inc or a replacement, every matching record is materialized as a MutableDocument in a List before any is saved. The previous path was a single streaming SQL UPDATE. On a large collection this is a heap hazard (and it is request-driven). Suggest collecting only the RIDs (cheap) and loading/modifying/saving each one in turn, or processing in bounded chunks.

2. Shared lock state is static and keyed by databasePath/collection (MongoDBCollectionWrapper.ID_INDEX_LOCKS, ID_INDEX_GAVE_UP). Entries are removed only on drop()/dropCollection. Dropping or closing a whole database (or a type dropped through SQL/Studio) leaves them behind, and ID_INDEX_GAVE_UP then survives for the JVM lifetime, so a recreated type of the same name will stay on the slow by-hand path forever. Consider clearing them in the database drop/close path, or keeping this state on the per-database wrapper instead of in statics.

3. Locking does not cover all writers. The read/write lock only protects insertDocuments; updates, upserts' own transactions, and deletes can still run while another thread drops and rebuilds the _id index (the comment says readers are fine, but an in-flight update transaction touching the old index is the same hazard the lock is introduced to avoid for inserts). Worth either documenting why that is safe or having executeUpdate* take the shared lock too.

4. Per-insert overhead. insertDocuments calls idIndexSatisfies (iterates getAllIndexes(false) and requiredKeyType over all ids) and then insertInTransaction calls hasUniqueIdIndex (iterates the indexes again) on every batch, including single-document inserts, which is the common wire pattern. Resolve the index once and pass it down, or cache the "satisfied key type" per collection and invalidate on schema change.

4b. getPath uses String.split("\\.") per $inc field per record, and setNested/unsetNested re-substring at each level. Fine for correctness; for hot updateMany paths a pre-split String[] would avoid garbage.

5. Regex translation (buildRegex):

   • The (?s:.*) prefix makes a non-matching scan O(n) backtracking per row even for anchored/literal patterns, and cannot use an index. Acceptable given the SQL MATCHES limitation, but the Javadoc claim that the engine "bounds the evaluation time" should be confirmed (I could not see a test that exercises a pathological pattern such as (a+)+$). A short test would back that claim.
   • A valid pattern with an inline (?x) and a trailing # comment, without the x option, is rejected as an "Invalid regular expression" because the comment swallows the wrapper's ). The test inlineCommentsModeRegexIsACleanError documents this, but it is a false rejection of a legal MongoDB (PCRE) regex; appending the \n whenever the pattern contains (? flags that include x would avoid it.
   • Java regex vs PCRE differences (e.g. \Z, possessive/atomic details, \h) are inherent, just worth a doc note.

6. Insert error index. With ordered now defaulting to true (correct per MongoDB), a non-duplicate failure thrown after the one-by-one fallback has inserted some documents is reported as a single writeError at index = n, where n counts successes. In the unordered case with earlier duplicate errors, n no longer equals the failing document's position, so the index can be wrong. Track the loop index i for the failing document instead of n.

7. Top-level operator without a field (buildExpression with field == null for $gt/$lt/$gte/$lte/$exists/$size/$in/$nin): appendField silently emits nothing, producing malformed SQL ( > :p0) and a confusing parser error, unlike buildEquality/appendOperators, which throw a clear IllegalArgumentException. A single guard at the top of buildExpression for non-top-level operators would make this consistent.

8. Id index key types: mixed string/numeric ids collapse into a STRING index where 1 and "1" become the same key, i.e. a false E11000. This is documented in the Javadoc and covered by mixedIdKindsKeepUniqueness, so just flagging it as a known semantic deviation that is worth a line in the user docs/release notes.

Style (CLAUDE.md)

• Imports in MongoDBCollectionWrapper are not in order (java.util.concurrent.* placed between java.util.Collection and java.util.HashMap), and com.arcadedb.schema.* imports sit above com.arcadedb.query.*.
• com.arcadedb.database.Document is used fully qualified in several places in MongoDBDatabaseWrapper (getPath, copyOfEmbedded, isEmbedded); the guideline asks to import the class. It collides with the Mongo Document import, so that one is a reasonable exception, but a short comment or an alias-free helper would make it less noisy.
• Deleting the commented-out ProjectingIterable code is a nice cleanup.

Tests

Coverage is good and follows the module's fixtures. Two additions I would suggest: a regression test for item 6 (error index after a partial unordered batch with a non-duplicate failure) and one for a large updateMany with a dotted $set (item 1) to pin the memory behavior once it is addressed. I did not run the suite locally; the PR states the module's 151 tests pass.

Nice work on a large and subtle semantics change.

Review of #9109 (mongodbw wire semantics)

I read the current source of all four changed main files and the test class. I did not build or run anything. Most of the points from the earlier review rounds are addressed in this head:

• $not over $ne/$nin is now two-valued.
• setsDottedPath covers every operator.
• requireEmbedded refuses to write below a scalar or an array.
• The _id index is guarded by a read/write lock and typed by the _id kind.
• ID_INDEX_GAVE_UP and ID_INDEX_LOCKS are cleaned up on drop.
• Numeric _id comparison, a typed $inc error, and RID-only collection in executeUpdateOnRecords.

The SQL stays fully parameterized and the regex pattern is validated on its own before it is wrapped, so I see no injection or security problem. What is left is small.

Behavior gaps

1. $set / $unset of _id on an existing document bypasses the immutability rule. replaceContent now refuses a changed _id (error 66, MongoDB wire: replaceOne with a replacement that has no _id removes the _id of the stored document, so it can no longer be found by _id #9064). But {$set: {_id: 99}} has no dotted key, so executeUpdate sends it down the SQL MERGE path (MongoDBDatabaseWrapper.executeUpdate, ~L718), which rewrites the _id. {$unset: {_id: ''}} goes down REMOVE the same way. MongoDB rejects both with ImmutableField. The unique index still prevents duplicates, but the key can silently change or disappear. Suggest routing an update that touches _id through the record path, with the same sameId check, and keeping the $set of _id allowed only for the document an upsert is inserting. There is no test for this today.

2. prepareUpsertIdIndex samples only the filter's _id and ignores the _id in u. For {q: {name: 'x'}, u: {_id: 7, ...}, upsert: true} that matches nothing, the sample is a generated ObjectId. A collection with a LONG index is therefore dropped and rebuilt as STRING (a full copy of the index) before a numeric _id is stored. After that, {_id: {$gt: 5}} and sort({_id: 1}) order lexicographically. That is the exact problem the typed index exists to avoid. Take the sample from a replacement's _id or from $set._id when present. Also, q.get("_id") == null cannot tell an explicit _id: null from an absent one, so use containsKey. An explicit null currently gets an ObjectId sample. A test with a numeric collection and an upsert of this shape would pin it.

3. prepareUpsertIdIndex costs an extra query per upsert. matchesAny runs a SELECT ... LIMIT 1 before the transaction, and executeUpdate then runs the same filter again. It is only a heuristic that decides whether to touch the index, so it is fine to keep. A one-line comment saying the result is advisory (the match can change before the transaction starts) would stop the next reader from treating it as a correctness check.

Nits

• getPath calls split("\\.") on every $inc field of every matching record (MongoDBDatabaseWrapper ~L1023). indexOf('.') in a loop avoids the regex and the array allocation, in line with the performance guidance in CLAUDE.md.
• The buildRegex Javadoc says the engine's MATCHES "bounds the evaluation time, so a hostile expression cannot pin a thread". I could not confirm that from this diff. If it is true, a test that depends on it would be good. If not, drop the claim so nobody relies on it.
• The com.arcadedb.database.Document fully qualified names are justified by the clash with the BSON Document, so I have no objection there.

Tests

MongoDBWireSemanticsTest is broad: 34 tests, real driver round-trips, concurrency, a failed index build, mixed _id kinds, and numeric range and sort on _id. The two gaps are the ones above: $set/$unset of _id on an existing document, and an upsert whose _id comes from u against a numeric collection. There is no System.out, no wall-clock assertion, and the tests start their servers through BaseMongoServerTest.

No blockers from my side. Point 1 is the one I would fix before merge.

🤖 Generated with Claude Code

Not up to standards ⛔

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of #9109 (mongodbw wire semantics)

Overall this is a solid, well-tested fix set. The null-aware $ne/$nin/$in/$not translation, the regex wrapping (user pattern validated alone, then the wrapped form, with the engine's MATCHES timeout still protecting against ReDoS), the copy-on-write nested $set/$unset, the array-padding cap, and concealing duplicate keys in production mode are all careful. MongoDBWireSemanticsTest covers the main paths, including a concurrent first-insert test. Nothing here looks blocking. Points worth considering:

Concurrency / correctness

1. The _id index lock only guards inserts. ensureIdIndex can dropIndex and recreate the index (numeric to string/double widening) under the write lock, but updateDocuments / deleteDocuments / executeUpdateOnRecords transactions take no lock. The Javadoc says "readers do not take the lock", but those paths are writers that maintain the same unique index. A concurrent update or delete on the collection during a rebuild may see the index disappear mid-transaction. Taking the read lock around those write transactions would make the invariant hold. If that was ruled out, please document why it is safe.
2. The first insert into an existing, large collection builds the index synchronously. A collection created before this change (or via SQL/Studio) gets a full index build on the request thread, under the exclusive lock, so every other insert into it blocks for the duration. This is probably acceptable, but it is worth a note in the docs or release notes, or an async or opt-in path later.
3. ID_INDEX_GAVE_UP is permanent and broad. createIdIndex catches any RuntimeException (a transient lock or timeout failure, not only duplicates) and the collection stays on the by-hand check until the type is dropped through the Mongo path. A type dropped via SQL and recreated also keeps the stale entry and lock. Consider narrowing the catch to DuplicatedKeyException, or at least re-validating the type's existence/identity.
4. Per-upsert extra query. prepareUpsertIdIndex runs matchesAny (SELECT ... LIMIT 1) for every upsert in the batch before the transaction. For a bulk upsert on a non-indexed filter that doubles the scans. The result is advisory only, so it could be skipped when the _id index already satisfies the filter's _id kind (check idIndexSatisfies first and avoid the query).

Style / CLAUDE.md

5. MongoDBDatabaseWrapper.java: there is an orphaned Javadoc ("Whether any operator targets a dotted path...") sitting directly above the touchesId Javadoc, and setsDottedPath has none. Move it down onto setsDottedPath.
6. com.arcadedb.database.Document is used fully qualified in several places in MongoDBDatabaseWrapper. That is understandable because of the clash with the Mongo Document import, but a small private helper (isEmbedded/toMap) would avoid repeating it.
7. A few new lines exceed the file's usual width (for example duplicateKeyMessage and the add return statement). Worth wrapping for readability.

Tests

8. The MongoDBToSqlTranslatorParamsTest updates only adjust expectations. Unit-level tests for buildRegex flag handling (i, m, s, x, unknown option, duplicated flag, $options without $regex) and isInclusionProjection would be cheap and catch SQL-shape regressions without a server. Also consider a test that an absent ordered stops at the first duplicate, since the default flipped to ordered. That is a user-visible behavior change and should be called out in the PR description.
9. Pre-existing and out of scope, but related: {field: {$in: [/a/, /b/]}} still binds BsonRegularExpression objects as plain parameters, so it silently matches nothing. A follow-up issue may be worthwhile now that regex is translated elsewhere.

Verification note: I reviewed the diff statically and did not run the test suite.

Review of #9109 (mongodbw wire semantics)

I read the diff only. I did not build the module or run the tests. Overall this is careful work, and the new MongoDBWireSemanticsTest covers each of the six issues well. The points below are in rough priority order; none is a clear blocker.

Correctness and behaviour

1. Race in the by-hand duplicate check. When _id has no unique index (a type made through SQL or Studio, or one in ID_INDEX_GAVE_UP), checkIdIsFree is a check-then-insert with no lock against other writers. Two concurrent inserts of the same _id can both pass and both commit. The class comment says the by-hand check is a fallback, but it would help to say explicitly that it is best effort and not a uniqueness guarantee.
2. By-hand check cost. In that mode every inserted document runs select @rid ... where _id = :id. With no index this is a full scan per document, so a bulk insert becomes O(n^2). Consider batching the check, or at least logging once per collection that the slow path is active.
3. ID_INDEX_GAVE_UP is sticky. After the first duplicate-caused build failure, the collection stays on the by-hand path until the JVM restarts or the collection is dropped, even if the user removes the duplicates. This is probably acceptable, but it is worth documenting. The static maps (ID_INDEX_LOCKS, ID_INDEX_GAVE_UP) are also keyed by database path. They are cleaned up only through the Mongo wrapper's drop paths, so a database dropped some other way (server API, SQL) leaves entries behind.
4. Index rebuild under the exclusive lock. A single insert whose _id kind the index cannot hold (for example a string _id into a collection of numeric ids) drops and rebuilds the whole _id index while all inserts, updates and deletes on that collection wait on the lock. For a large collection, one request from any authenticated client can stall writes for the duration of the rebuild. Consider whether to bound this or log it.
5. Mixed _id kinds collapse into one string key. When numeric and string ids are mixed, 1 and "1" get the same key, so inserting "1" next to 1 reports a false E11000. After the rebuild, range queries and sorts on _id also become lexicographic. The Javadoc says so, which is good. It is still a deviation from MongoDB worth listing as a known limitation in the PR description.
6. Locking only covers this plugin. The read/write lock protects only the Mongo plugin's own writers. Index maintenance by SQL, HTTP or Gremlin writers on the same type is not covered. Readers are also unlocked, and the comment relies on the planner resolving indexes at plan time. If that holds for the version in use, fine. A short test that queries concurrently with a rebuild would confirm it.
7. Update via records is not atomic with the filter. executeUpdateOnRecords collects RIDs, then loads and modifies each record without re-evaluating the filter. A record deleted between the select and the modify makes rid.asDocument() throw RecordNotFoundException, which surfaces as a generic error. Catching that and skipping the record would match the old single-statement behaviour more closely.
8. Array-index segments. isArrayIndex accepts leading zeros ("01") and any Unicode digit. MongoDB treats "01" as a field name, not as index 1. This is a minor edge case; c >= '0' && c <= '9' plus rejecting a leading zero would be stricter.
9. Regex wrapping. (?s:.*)(?flags:pattern)(?s:.*) run through MATCHES gives the find-anywhere semantics, and compiling the user's pattern alone first is a good defence against breaking out of the wrapper. The leading and trailing .* can backtrack heavily on long strings with patterns that fail late. The Javadoc says MATCHES bounds evaluation time. Please double check that claim holds for this code path, or add a test with a pathological pattern such as (a+)+$ to demonstrate it.
10. Projection scope. Projection operators such as $slice and $elemMatch are rejected with IllegalArgumentException. Make sure that maps to a proper MongoDB error code (for example 2 BadValue) rather than an internal error. Exclusion of a dotted path through arrays of embedded documents is not handled either.

Style and conventions (CLAUDE.md)

• There are a few fully qualified names, e.g. com.arcadedb.database.Document in getPath, copyOfEmbedded and isEmbedded. Please import it. The collision with the Mongo Document is the likely reason, so an alias-free approach would be a small helper that takes Object and does the check once.
• Deleting the large commented-out ProjectingIterable block is welcome.
• Some of the Javadoc and comments are long multi-paragraph blocks. CLAUDE.md prefers short comments that explain only the non-obvious why. The index-type reasoning in ensureIdIndex is a legitimate exception, but the others could be trimmed.

Performance

• idIndexSatisfies on every insert calls getType, getAllIndexes and a linear scan of the indexes. This is cheap, but it runs on the hot insert path, so caching the result per collection would remove it.
• withoutNull and containsNull allocate per query. That is fine for the parse path.
• The extra matchesAny query per upsert only runs when the index does not already satisfy the key. That is a good guard.

Security

• No injection concerns: values go through bound parameters, and field names go through quoteFieldPath and Identifier.quote.
• Concealing the duplicate key in production mode is a good touch.
• MAX_ARRAY_PADDING caps the heap amplification from $set on a large array index. Up to 100k nulls per request is still sizeable. It is acceptable, but note it multiplies across multi updates.

Tests

• Coverage is thorough, including a concurrency test, ordered and unordered duplicate handling, and _id immutability.
• Please also add: a test that a bulk insert of many documents in by-hand mode (no index) stays reasonable, and a test where a record disappears between the select and the modify in executeUpdateOnRecords (a RecordNotFoundException case).
• The concurrency test depends on retrying WriteConflict up to 20 times. If that is flaky on slow CI, a deterministic barrier on the first insert would be more stable.

Thanks for the detailed regression tests. The items most worth addressing before merge are 1, 2, 7 and the missing import of Document.

Review of #9109 (mongodbw wire semantics)

Overall this is a careful, well-scoped fix for six real compatibility gaps. The comments explain the non-obvious why (lock protocol, null three-valued logic, regex wrapper), and MongoDBWireSemanticsTest covers the main paths end to end through the real Mongo driver, including concurrency and the "cannot build index" fallback. I found no blocking bugs. Notes below, roughly by importance.

Correctness / behavior

1. String key fallback gives false E11000s for mixed id kinds. When ids are not all one kind, the index becomes a STRING index, so _id: 1 and _id: "1" collide (the javadoc says so). Likewise the DOUBLE key for mixed long/double ids collapses longs above 2^53. Both are documented trade-offs and rare, but they turn into a hard insert failure. Worth a line in the user docs or a follow-up issue.
2. ID_INDEX_GAVE_UP is never cleared except on drop through the Mongo plugin. If a user removes the duplicates (via SQL) the collection stays on the slower by-hand check until restart, and a type dropped/recreated through SQL/Studio keeps a stale entry (and a stale lock in ID_INDEX_LOCKS). The leak is tiny; consider clearing the entry when createIndexes creates or finds the _id index.
3. The by-hand duplicate check is not atomic (checkIdIsFree is a query followed by an insert), so two concurrent inserts into a type without the index can both succeed. Acceptable for the legacy no-index case, but the comment on insertInTransaction could say it is best-effort.
4. Index rebuild under the exclusive lock (_id key widening, e.g. numeric to string) is a full copy that blocks every insert/update/delete on the collection while it runs. Fine for the first insert (empty type), but on a large collection one stray string _id triggers it. Consider logging at INFO when a rebuild of a non-empty index starts, so it is diagnosable.
5. Projection is only applied in find(). Operators such as $slice/$elemMatch/$meta (a Document/List flag) now throw IllegalArgumentException, where before they were silently ignored. Arguably better, but it is a behavior change that existing clients using $slice will hit; mention it in the release notes. Please also confirm that the exception from find reaches the client as a command error and not as an internal error.
6. _id: null is stored but skipped by the unique index (default NULL_STRATEGY.SKIP), so duplicate explicit-null ids are accepted, unlike MongoDB. Edge case only.

Performance

• Dotted-path $set/$unset/$inc, _id-touching updates and replacements now run as SELECT @rid plus per-record load/modify/save instead of one SQL UPDATE. Needed for the semantics, and collecting only RIDs is a good touch, but updateMany over a big collection will be slower.
• The regex wrapper (?s:.*)...(?s:.*) with MATCHES cannot use an index. I checked MatchesCondition: it shares a deadline per command, so the javadoc claim about hostile patterns being bounded holds.
• insertDocuments computes ids and walks getAllIndexes on every call. Cheap, but it is the hot insert path; idIndexSatisfies could cache the resolved key type per collection if profiling shows it matters.

Style / CLAUDE.md

• com.arcadedb.database.Document is used fully qualified several times in MongoDBDatabaseWrapper. The name clash with the Mongo Document makes that unavoidable, but isEmbedded/copyOfEmbedded could centralize it further.
• Good: final used consistently, single-statement ifs without braces, no System.out, assertThat(...) style in tests, and the dead commented-out ProjectingIterable code was removed.
• concurrentFirstInsertsWithMixedIdKinds runs a thread pool against a real server: make sure it uses a bounded future.get(timeout) so a lock regression fails instead of hanging CI, and tag it @Tag("slow") if it is slow.

Test gaps (minor)

• No test for $not with a regex literal on a null/missing field ({name: {$not: /^a/}}), although the code path exists.
• No test for $set through an embedded typed document (rather than a Map), which copyOfEmbedded special-cases.
• No test that a failed batch insert leaves no open transaction on the thread (the finally { rollback } fix), e.g. a follow-up command on the same connection after an E11000.

The lock upgrade/downgrade protocol in insertDocuments is correct as written (read lock released before the write lock is requested, then downgraded before release). Nice work.

🤖 Generated with Claude Code

Review of #9109: mongodbw wire semantics (#9061 #9060 #9063 #9064 #9068 #9069)

I read the diff and the new tests, but did not run the module suite myself. Overall this is solid work. The null-aware $ne/$nin/$in/$not, the regex wrapping (the user pattern is validated on its own and again inside the wrapper, and the engine's MATCHES has a shared deadline), the copy-on-write nested $set/$unset/$inc, the MAX_ARRAY_PADDING guard and the PathNotViable refusal are all careful. The rollback in finally after a failed insert fixes a real leak. No blocking bugs found. The points below are design risks and test gaps, roughly in order of importance.

Behavior and design

1. Upgrade path on existing collections. A collection that already holds data gets its unique _id index built synchronously, on the request thread of whichever client inserts first, under the exclusive lock. On a large collection this stalls every insert, update and delete on it for the duration of the build, and the client may time out. The rebuild logs at INFO ("writes to it wait"), but the first build is silent. Consider at least an INFO log before the first build, and think about whether there should be an opt-out for existing collections.

2. Key type coercion causes false E11000s. holds(STRING, LONG) is true, so once a collection has any string or ObjectId _id (the most common case), a numeric _id is stored in the STRING index. 1 and "1" then collide, where MongoDB treats them as distinct. The DOUBLE key (mixed integral and floating ids) has the same issue above 2^53, where two different longs become one key. Both are documented in the Javadoc, but no test pins the behavior down. I'd add one for 1 then "1", so a future change to the key strategy is a conscious decision.

3. Static, path-keyed state. ID_INDEX_LOCKS, ID_INDEX_GAVE_UP and ID_INDEX_SLOW_PATH_LOGGED are JVM-wide maps keyed by databasePath/collection, with manual forget* calls on two drop paths. These leak or go stale in other cases:

   • a type dropped or recreated through SQL or Studio;
   • a database that is closed rather than dropped;
   • ID_INDEX_GAVE_UP, which keeps a collection on the slow check forever, even after the duplicates are cleaned up, until a restart.

   MongoDBDatabaseWrapper is already one instance per database and owns the collections map. Moving this state onto the wrapper (or the collection wrapper) would remove the static maps and the cleanup hooks. It would also let GAVE_UP be re-evaluated, for example by checking existsType/index presence when a collection is re-resolved.

4. By-hand _id check is racy. This is documented in the Javadoc, and it only applies to collections that cannot get the index. It does mean "E11000 guaranteed" holds only for plugin-created collections, so it is worth stating in the PR description or docs.

5. Duplicate fallback cost. When a batch hits a duplicate, the whole batch is rolled back and then re-inserted one document per transaction. A 10k-document insertMany with a duplicate at the end costs 10k commits. This is correct, and arguably rare, but it is worth knowing about. A cheaper variant would reuse the first pass to find the failing index, then commit the prefix in one transaction for ordered inserts.

6. Regex performance. Every regex query becomes MATCHES '(?s:.*)(?u..:pattern)(?s:.*)'. It cannot use an index, and the leading .* makes a non-matching scan more expensive than a find-anywhere match. The timeout bounds the damage. For ^-anchored patterns without the m flag, the leading (?s:.*) could be omitted, which is cheaper and keeps the semantics.

7. executeUpdateOnRecords. Any update with a dotted path, a replacement, or an _id operator now does select, then load, then modify, then save per record, instead of one SQL UPDATE. That is necessary for the semantics. Concurrent writers on the same record now surface as conflicts (the test tolerates a 112 retry), which is the correct MongoDB answer. It is still worth a note that large updateMany with dotted $set is slower than before.

8. Projection errors. isInclusionProjection throws IllegalArgumentException. Please confirm the command dispatcher maps that to a MongoDB error with a sensible code, such as BadValue/31254. The test only asserts RuntimeException. Also, dotted projection into an array of embedded documents ({'items.n': 1}) silently drops the field in projectField, since only Document is descended, where MongoDB projects through the array.

9. ordered default flip. Defaulting to ordered is correct per MongoDB. It is a visible change for clients that relied on the old behavior, so it is worth a release note.

Tests

The new MongoDBWireSemanticsTest is thorough and follows the project assertion style, with no wall-clock assertions. Gaps I would add:

• An existing collection that already holds data and gets the index on its first plugin insert (the upgrade path in point 1), including the large-ish case.
• 1 versus "1" collision behavior (point 2), and a Long above 2^53.
• A concurrent test where an index rebuild (a string id arriving after numeric ones) races in-flight updates or deletes. The current concurrency test covers inserts only.
• An insertMany where a non-duplicate error occurs after a duplicate. It checks that the throw duplicate path in the fallback loop leaves a consistent n and write-error list.
• A projection on an array of embedded documents (point 8).

Style nits

• com.arcadedb.database.Document is used fully qualified in a few places in MongoDBDatabaseWrapper. That is understandable given the clash with the BSON Document, but a short comment or a helper would make the intent clear.
• MongoDBDatabaseWrapper is growing quickly (update-path helpers, path utilities, upsert index preparation). Extracting the path helpers (setPath/unsetPath/getPath/add/...) into a small package-private class would make them easier to unit test directly.

Nothing here should block merging if the maintainers accept the documented trade-offs. I would at least address the state-lifecycle question (point 3) and add the 1 versus "1" test (point 2).

Review of #9109 (mongodbw wire semantics)

I read the full diff and the new test class. I did not build the module or run the tests. Earlier rounds already covered most of the design points (lock protocol, STRING/DOUBLE key trade-off, executeUpdateOnRecords memory, by-hand duplicate check). This pass looks for what is left. I found no blockers.

Worth a look

1. No test for range or sort on _id once the kinds are mixed. rangeAndSortOnNumericIdsWithTheIdIndex and mixedIntegralAndFloatingIdsStayNumeric cover the numeric index. numericAndStringIdsOfTheSameTextShareAKeyOnceKindsAreMixed pins only uniqueness. After a rebuild to a STRING-key index, {_id: {$gt: 5}} or sort({_id: 1}) may be planned against that index with a numeric parameter. A test that mixes 10, 9 and "abc" and then runs a range query would show what the planner does (lexicographic, coerced, or skipped). It would also keep the "conscious deviation" documented in the last test honest.

2. Projection covers find only. findAndModify has a fields option and the legacy query path has a field selector. Neither goes through projectDocument, so those clients still get full documents. If that is intentional for this PR, a short note in the description or a follow-up issue would help. The @rid/@type/@cat stripping in convertMapToMongoDB does apply to every path.

3. Unanchored regex wrapper and cost. (?s:.*)<pattern>(?s:.*) through MATCHES makes every regex a full scan with a leading greedy .*. For a failing row that is O(n^2) in the string length for many patterns, and ^prefix regexes can no longer use an index. The engine's MATCHES deadline bounds the damage, so this is a performance note and not a security one. Using Matcher.find() semantics (or a dedicated function) would avoid the wrapper, but the current approach is acceptable if documented as a known cost.

4. $ne/$nin/$not now emit field IS NULL OR .... This is correct, but the OR defeats index use on that field, which a plain <> could not use either. Nothing to change, just worth knowing for users running {x: {$ne: ...}} on large collections.

Tests (CLAUDE.md style)

• Several assertions are too loose to pin the behavior: replaceOneCannotChangeTheId, projectionAndNoRecordMetadata (invalid mix) and idIsImmutableUnderSetAndUnset check only RuntimeException/MongoCommandException. Asserting the error code (66 ImmutableField, the projection error code) would stop an unrelated failure from satisfying them.
• aFailedIdIndexBuildIsNotRetriedOnEveryInsert asserts only the end state (no index, duplicate rejected). It would still pass if the build were retried on every insert. Checking the warning is logged once, or timing nothing but counting build attempts through a package-private hook, would test the claim in the name.
• concurrentFirstInsertsWithMixedIdKinds retries on WriteConflict (112) up to 20 times. That is reasonable, but if the lock protocol regressed it would turn into a slow pass instead of a failure. Consider asserting no attempt other than a 112 is ever seen, which it already does, and logging the retry count so a drift is visible.

Minor / style

• ID_INDEX_LOCKS, ID_INDEX_GAVE_UP and ID_INDEX_SLOW_PATH_LOGGED are static and keyed by database path. They are cleaned on dropCollection and drop(Oplog), but a database dropped through SQL, HTTP or the server API leaves its entries behind, and a recreated database at the same path would inherit a stale GAVE_UP flag. A per-database holder owned by MongoDBDatabaseWrapper, or a schema-change listener, would avoid that.
• com.arcadedb.database.Document is written fully qualified in MongoDBDatabaseWrapper and in the new helpers. That is justified by the clash with the Mongo Document, but a one-line comment or a local alias-style helper would make it less noisy.
• The commented-out ProjectingIterable/ProjectingIterator removal is a good cleanup.

Security

The SQL stays parameterized with quoted field paths. The user's regex is validated on its own before it is wrapped, and the duplicate-key message hides key values in production mode. The MAX_ARRAY_PADDING cap addresses the obvious heap-exhaustion vector. I see no new security hazard.

Overall this is a careful, well-tested change. Items 1 and the weak error-code assertions are the ones I would address before merge.

Review

Solid, well-documented work. The null semantics for $ne/$nin/$in/$not, the _id immutability checks, the copy-on-write nested $set/$unset and the regex wrapping are carefully thought through, and the new MongoDBWireSemanticsTest has good coverage, including a concurrent mixed-_id insert case. I checked the MATCHES claim: MatchesCondition does cache the compiled pattern per command and applies a shared regex timeout, so the regex path is not a ReDoS hole. Items below, most important first.

Potential bugs

1. Index read lock can leak if database.begin() throws (MongoDBDatabaseWrapper.deleteDocuments and updateDocuments). Both do:

   indexLock.lock();
   database.begin();
   try { ... } catch (...) { database.rollback(); throw e; } finally { indexLock.unlock(); }

   begin() sits outside the try, so if it throws, the read lock is never released. A leaked read lock then blocks every future ensureIdIndex writer on that collection forever (the first insert, or a rebuild for a new _id kind). Move begin() inside the try, or acquire the lock inside it. Note the catch would also call rollback() when begin() failed, so a nested try/finally for the lock is cleaner.

2. ID_INDEX_GAVE_UP is permanent for the JVM lifetime. If a collection had duplicate _ids when first touched, it stays on the slow by-hand path (one query per inserted document) even after the duplicates are cleaned up. It is only cleared by the plugin's own drop/dropCollection, not by a drop through SQL/Studio or a truncate. Consider re-evaluating after N inserts or exposing a way to reset it. It is also static, so state survives a server restart inside the same JVM (relevant for tests that reuse a database path).

3. Mixed _id kinds collapse to a STRING key, so {_id: 1} followed by {_id: "1"} is reported as E11000, where real MongoDB treats them as distinct. The Javadoc is upfront about this, but a clear regression test pinning the behavior (and a note in the docs) would help so nobody mistakes it for a bug later.

4. Batch fallback cost. On a DUPLICATED_KEY in the fast path, the whole batch is rolled back and re-inserted one transaction per document. A 10k-document batch with a single duplicate near the end means ~10k commits. Acceptable for correctness, but worth a comment or a smarter retry (for example, retry the part before the failing index as one batch for ordered inserts, using the failing document's position if available).

Design and performance

• withTotalBuckets(1) in getOrCreateCollection (also already used in create/createIndexes) limits insert parallelism for collections made by an insert. If this is deliberate (for example, to keep the unique _id index single-bucket), say so in a one-line comment, since it is a non-obvious scalability tradeoff.
• Rebuilding the _id index under the exclusive lock blocks all writes to the collection for the duration of the copy. The log lines say so, which is good. Mixed-kind workloads on a big collection could stall noticeably; fine as an edge case, but worth documenting.
• The regex wrapper (?s:.*)(?flags:pattern)(?s:.*) makes every $regex a full-string MATCHES, so it never uses an index and costs more than Matcher.find(). That is the practical option with SQL MATCHES, but a short note that anchored/prefix regexes are not index-assisted would set expectations.
• getPath calls String.split("\\.") per $inc, which allocates a regex and an array on every call. A manual indexOf('.') loop (as the other path helpers already do) fits the project's GC-light guidance.
• setNested copies the entire embedded map/list at each level on every $set. Correct for dirty-tracking, but for large arrays updated in a multi-doc update it is O(size) per matching record.
• executeUpdateOnRecords loads each record by RID after a select: two lookups per record versus a single SQL UPDATE, and only for the dotted/replace/_id cases, so fine.

Style and conventions (CLAUDE.md)

• com.arcadedb.database.Document is used fully qualified in several places in MongoDBDatabaseWrapper. I understand it clashes with the Mongo Document import, so this is justified, but an import alias is not possible in Java, so consider a small private helper (isArcadeEmbedded(Object)) to confine the FQN to one spot.
• Tests: please confirm MongoDBWireSemanticsTest takes ports from BaseMongoServerTest rather than hard-coding any (it extends the base class, so it looks right). The concurrent test is good; make sure it has no raw wall-clock assertions.
• Good removal of the dead commented-out ProjectingIterable code.

Security

No concerns found. Values are bound as parameters (checkIdIsFree uses Identifier.quote and a named param), the E11000 message honors isProductionMode() to conceal key values, and MAX_ARRAY_PADDING bounds the heap use of a hostile $set array index.

Test coverage

Strong overall. Gaps worth adding: (a) a regression test for the begin() lock-leak path in item 1 if you fix it, (b) an ordered vs unordered batch with the duplicate in the middle of a large batch, and (c) a type created via SQL with existing duplicate _ids to cover the GAVE_UP path end to end.

Overall, I'd fix item 1 before merge; the rest are suggestions.

🤖 Generated with Claude Code

Codecov Report

❌ Patch coverage is 74.32675% with 143 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.57%. Comparing base (9b0c097) to head (1452b7a).
⚠️ Report is 40 commits behind head on main.

@@             Coverage Diff              @@
##               main    #9109      +/-   ##
============================================
+ Coverage     76.53%   76.57%   +0.04%     
+ Complexity     3904     3903       -1     
============================================
  Files          2115     2116       +1     
  Lines        209219   209810     +591     
  Branches      44191    44353     +162     
============================================
+ Hits         160130   160669     +539     
+ Misses        31898    31885      -13     
- Partials      17191    17256      +65     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.