Fix MongoDB query heuristic semantics - #1750
Conversation
… calculator Twenty-four defects in MongoHeuristicsCalculator, each as its own @disabled test, so they can be enabled one at a time and in any order as the behaviour is implemented. Every expected value was obtained by running the same query and the same document against a real MongoDB 7.0.40 server, so the assertions state what the database does rather than an interpretation of the documentation. Ten of them make the calculator throw. As MongoHandler does not catch anything, the exception escapes the heuristics computation for the action, so the ExtraHeuristicsDto is lost, including the SQL heuristics computed before it: $type with a string alias or with a list of aliases, a bitmask given as an Integer or as a list of bit positions, and $not holding more than one operator, are not parsed, and the calculator throws a NullPointerException on the resulting null operation; an operator that is not modelled at all is not parsed either, which covers $expr, $jsonSchema, $where, $text, $geoWithin, $geoIntersects and $comment. The last one attaches to an otherwise ordinary query, so {"a": 1, "$comment": "..."} is enough to lose the heuristics of the action; an empty list of values, an empty array in the document, and comparing two empty arrays aggregate over no element and throw IllegalArgumentException; an ordering comparison involving NaN builds a Truthness with neither of its values equal to 1, which its own constructor rejects. Any double field can hold NaN; a value the calculator cannot compare, such as a sub-document or binary data, reaches the "Unsupported type" branch and throws. The test for the operators that are not modelled asserts only that nothing is thrown, not any particular score: whether such an operator should be supported, and what it should answer, is a decision for the heuristic. What it should not do is cost the action its heuristics. The other fourteen are answered, but not the way MongoDB answers them. Those that report a match where MongoDB has none are the harmful direction, as a condition no data can satisfy is recorded as covered and the search stops working towards it. Several share one rule: MongoDB matches a field when its value satisfies the condition, or when it holds an array of which any element does, and that applies to every condition on a field rather than only to equality. $all is the same rule quantified the other way round, over the expected values. Four tests of behaviour that is already correct are added as well, as a guard while the heuristic is changed. One of them, testNotEqualsAgainstAnArrayField, answers correctly only because two of the reported defects cancel each other out; fixing either one alone turns it into a false positive, which is why it is worth keeping visible.
…ses; re-enable disabled tests
…dge cases; add related test cases.
… bitmask operations
… re-enable disabled tests and migrate geospatial logic to `MongoUtils`.
…; adjust comparison logic for `NOT_EQUALS_TO` operator.
…ated `@Disabled` annotation.
…nd simplify logic
…gic to handle `$comment` operators
… handling of query operations
… logic, and improve modularity for comparison and bitwise operations
… logic, and improve modularity for comparison and bitwise operations
…ity and consistency.
There was a problem hiding this comment.
Ran all of this against a local mongo 7.0.41 instead of going off the docs.
Two queries the server accepts make the calculator throw: $regex on an array with no strings in it, which is a regression since master returned C_FALSE, and a BinData bitmask. Nothing catches either on the way out of computeDistanceDocuments, so the action loses its SQL heuristics along with the mongo ones.
The rest are wrong answers rather than crashes. No validation on the bitmask itself, $comment stripped out of things that aren't operators, $all wrong on a repeated element.
Two I went after that turned out fine, so you can skip them: nested $not agrees with the server both ways, and the reversed actual/expected in evaluateListEquality does hit the taint handler backwards, but ExecutionTracer.handleTaintForStringEquals takes either direction so nothing comes of it.
Not your change, but the unparsed-query-then-NPE path that testOperatorsThatAreNotModelledDoNotThrow documents is bigger than that disabled test implies. {a:{$not:/x/}} lands in it, and that one the server answers fine.
| .map(element -> (String) element) | ||
| .map(element -> evaluateRegularExpression(element, pattern, taintHandler)) | ||
| .toArray(Truthness[]::new); | ||
| return buildOrAggregationTruthness(results); |
There was a problem hiding this comment.
Throws if the array holds no strings. The filter can empty the stream and checkValidTruthnesses won't take an empty array.
{a:{$regex:"x"}} vs {a:[1,2,3]} -> IllegalArgumentException: null or empty Truthness instance
Server returns 0 there, master returned C_FALSE. Nothing catches it on the way out (MongoHandler:155), so the action loses its SQL heuristics too.
This is also the only array path not wrapped in buildSafeScaledTruthness, cf 385 and 489.
| } | ||
| // value can be a byte array | ||
| if (value instanceof byte[]) { | ||
| byte[] bytes = (byte[]) value; |
There was a problem hiding this comment.
BinData masks NPE, only a raw byte[] gets through. Server matches both.
{a:{$bitsAllSet: BinData(0,"Ag==")}} vs {a:2}
Needs to take org.bson.types.Binary as well, reflectively like BsonHelper.isBsonTimestamp so the driver stays out of the compile deps.
The byte[] branch isn't wasted, to be fair. The query arrives however the app built it, with no decode in between (MongoOperationClassReplacement:20, MongoFindCommand.getQuery(), MongoHandler:155), so both shapes turn up. It's the decoded one that has no cover.
| for (Object p : (List<?>) value) { | ||
| if (p instanceof Number) { | ||
| long pos = ((Number) p).longValue(); | ||
| if (pos >= Long.SIZE) { |
There was a problem hiding this comment.
No check for pos < 0. Shift counts get masked to 6 bits, so 1L << -1 sets bit 63 and {a:{$bitsAllSet:[-1]}} comes back as a match on Long.MIN_VALUE.
Server won't run it at all: Failed to parse bit position. Expected a non-negative number in: 0: -1. Answering false would be fine. Answering match sends the search after something that can't happen.
| */ | ||
| public static OptionalLong toBitMaskValue(Object value) { | ||
| // value can be a number | ||
| if (value instanceof Number) { |
There was a problem hiding this comment.
Nothing validates the mask. 3.9 truncates to 3 and matches {a:3}, -1 becomes all-ones and matches {a:-1}. Server rejects both, Expected an integer and Expected a non-negative number.
The field side of the same comparison already validates, that's what getIntegralLongValue is for. Same call here, plus >= 0.
| return operation; | ||
| } | ||
| return parseWithSelectors(normalizedDocument); | ||
| } |
There was a problem hiding this comment.
Recurses through the whole query, so it strips the key out of values as well as operator objects.
doc {a: {$comment: "note", x: 1}}
query {a: {$eq: {$comment: "note", x: 1}}}
server 1, calculator no match
$-prefixed field names store fine since 5.0, I ran this on 7.0.41. Keeping the strip at the top level fixes it and stops the query being deep-copied on every parse.
| public class QueryParser { | ||
|
|
||
| private static final String SYNTHETIC_FIELD_NAME = "$"; | ||
| private static final Set<String> COMMENTS_OPERATORS = new HashSet<>(Arrays.asList("$comment", "$comments")); |
There was a problem hiding this comment.
$comments isn't a thing. Server says unknown top level operator: $comments. Dropping it here means we score a query that could never have run.
| Object actualValues = getValue(document, fieldName); | ||
| if (actualValues == null || !(actualValues instanceof List<?>)) { | ||
| if (!(actualValue instanceof List<?>)) { | ||
| if (expectedValues.size() != 1) { |
There was a problem hiding this comment.
$all is an $and of $eq, so repeats against a scalar are fine. Server returns 1 for both of these, we return no match.
{a:{$all:[1,1]}} vs {a:1}
{a:{$all:[null,null]}} vs {b:1}
Same assumption in the single-element branch just above. Dedupe expectedValues first and both go.
| boolean first = true; | ||
| for (Object doc : documents) { | ||
| double ofTrue = computeHeuristicOnDocument(operation, doc).getOfTrue(); | ||
| double ofTrue = computeHeuristicQueryOperation(operation, doc).getOfTrue(); |
There was a problem hiding this comment.
Three walks over documents here: 57 counts it, 74 counts it again, then this loop. MongoHandler:149 peeks before handing it over, so four.
It's a FindIterable, so it replays and nothing is broken. But every replay is another find on the wire. Four collection scans per action, on the path that runs for every action. One copy into a List at the top of computeDistanceDocuments covers it, and makes the Iterable in the signature honest.
Pre-existing, only raising it because it's the method you rewrote.
| } | ||
| } | ||
|
|
||
| Truthness compareNullableValues(Object leftValue, SqlExpressionEvaluator.ComparisonOperatorType comparisonOperatorType, Object rightValue) { |
There was a problem hiding this comment.
Names are the wrong way round. 243 calls compareNonNullValues(rightValue, op, leftValue), so the contract is (expected, op, actual), which is what makes $gt read as actual > expected.
127 and 283 pass them the other way. I went looking for reversed taint args at 153 and they are reversed, but ExecutionTracer.handleTaintForStringEquals takes either direction, so there's nothing to fix. Renaming these to expectedValue/actualValue would stop the next one of these happening.
| return C_FALSE; | ||
| } | ||
|
|
||
| long actualRemainder = ((Number) actualValue).longValue() % divisor; |
There was a problem hiding this comment.
{a:{$mod:[0,0]}} throws ArithmeticException. Server rejects the query (divisor cannot be 0) so it shouldn't reach us, and master behaves the same, but the method moved into this class so it's a cheap guard while you're here.
2 participants
Correct MongoDB query heuristics for arrays, null and missing fields, numeric comparisons, and bitwise operations. Extract shared evaluation logic into MongoHeuristicsCalculatorHelper.