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.
The answers quoted in the comments below are what a mongo 7.0.41 returns for those queries, not what the documentation implies.
Two queries mongo accepts make the calculator throw: $regex on an array holding no strings, and a BinData bitmask. The first one is a regression, the code this replaces returned C_FALSE for it. 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 that look wrong and aren't: nested $not agrees with mongo 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 mongo 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
mongo 7.0.41 returns 0 documents for that, and the pre-PR code returned C_FALSE, so it's a regression rather than a gap. 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. mongo 7.0.41 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.
mongo 7.0.41 won't run that query 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}. mongo 7.0.41 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}}}
mongo 1 document, calculator no match
$-prefixed field names store fine since 5.0, so that pair is not hypothetical. 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. mongo 7.0.41 says unknown top level operator: $comments and refuses the query. Dropping it here means we score something 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. mongo 7.0.41 returns 1 document 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. That reaches the taint handler at 153 with the arguments swapped, which costs nothing today because ExecutionTracer.handleTaintForStringEquals takes either direction. Renaming these to expectedValue/actualValue would keep it that way.
The same split shows up in tainting. $ne over a field holding an array reaches computeHeuristicContainsElement, which compares with EQUALS_TO and so records a specialization, while $ne over a scalar field goes through this method with NOT_EQUALS_TO and records nothing. One operator, two behaviours, decided by what the field happens to hold rather than by the query.
| return C_FALSE; | ||
| } | ||
|
|
||
| long actualRemainder = ((Number) actualValue).longValue() % divisor; |
There was a problem hiding this comment.
{a:{$mod:[0,0]}} throws ArithmeticException. mongo 7.0.41 rejects that query (divisor cannot be 0) so it shouldn't reach us, and the same line before this PR does it too, but the method moved into this class so it's a cheap guard while you're here.
|
hi @jgaleotti @LautaroPetaccio I m confused here... @jgaleotti you asked my review, but @LautaroPetaccio did it first? in that case, if you asked @LautaroPetaccio to do it, you should then wait to fix his comments before asking my review. @LautaroPetaccio did you do such review? or was AI generated? there is plenty of sentences like |
|
Hi @arcuri82, I proactively reviewed the PR to help finding issues on the changes, @jgaleotti did not ask me to do it. I'm sorry if I caused any problems while doing so. The idea was to avoid introducing bugs to the MongoDB heuristic which would imply another re-review. The review was done with AI and fully tested against tests examples and MongoDB (which is the server here). I read the comments and they seemed ok, but your reply made me realize that the wording "the server" is quite ambiguous, which, although it states that the PR is working with MongoDB, in the context of EvoMaster, the wording is not enough. When talking about |
|
hi @LautaroPetaccio. |
| // If both types are supported, but no actual comparison logic is defined, | ||
| // we considered them to be incompatible, therefore the comparison returns true | ||
| // only if the comparison operator is NOT_EQUALS_TO. Otherwise returns false. | ||
| truthnessOfComparison = comparisonOperatorType == SqlExpressionEvaluator.ComparisonOperatorType.NOT_EQUALS_TO ? TRUE_C : C_FALSE; |
There was a problem hiding this comment.
Two sub-documents never reach a comparison. The branches above cover numbers, strings, booleans, lists, dates, timestamps and object ids, and two documents fall through to here, which answers true for $ne and false for everything else.
The half that hurts is $ne and $nin, reporting a match mongo 7.0.41 does not have:
{a:{$ne:{x:1}}} vs {a:{x:1}} mongo 0 documents, we match
{a:{$nin:[{x:1}]}} vs {a:{x:1}} mongo 0 documents, we match
The other half is misses, and it takes in the plainest nested-object query there is:
{a:{x:1}} vs {a:{x:1}} mongo 1, we do not match
{a:{$eq:{x:1}}} vs {a:{x:1}} mongo 1, we do not match
{a:{x:1}} vs {a:[{x:1},{x:2}]} mongo 1, we do not match
{a:{$gt:{x:0}}} vs {a:{x:1}} mongo 1, we do not match
To be fair this is already better than what it replaces, which threw on a sub-document rather than answering at all. And testFieldsHoldingASubDocument does cover sub-documents, but against a scalar, where false is the right answer, so the document-to-document case falls outside it.
Binary data lands in the same place, with the same split between the two directions:
{a:{$eq:BinData(0,"AQI=")}} vs the same value mongo 1, we do not match
{a:{$ne:BinData(0,"AQI=")}} vs the same value mongo 0, we match
The rest of this area agrees with 7.0.41: $ne and $nin over arrays and missing fields, five $elemMatch shapes, $size, $exists, int against long against double against Decimal128, negative and fractional $mod, $regex with options, all four bit operators, $nor, nested $and and $or.
One more cost, beyond the answer. Neither branch records a string specialization, so a tainted value inside a sub-document is never offered to the search either. {a:{x:"..."}} against a document whose x holds a tainted string records nothing, where the same comparison one level up records it.
| default: | ||
| throw new IllegalArgumentException("Unsupported comparison operator type: " + comparisonOperatorType); | ||
| } | ||
| if ((actualValue instanceof List<?>) && !(expectedValue instanceof List<?>)) { |
There was a problem hiding this comment.
This condition sends a list expected value straight to compareNullableValues, which only ever compares the two arrays as wholes. So $eq misses an array holding the given array as one of its elements:
{a:{$eq:[1,2]}} vs {a:[1,2]} mongo 1, we match
{a:{$eq:[1,2]}} vs {a:[[1,2],3]} mongo 1, we do not
$in and $all both find a nested array in that position, so $eq is the one of the three that does not.
This one also pays to stay wrong. The document scores ofTrue 0.48970, against 0.10891 for {a:{$eq:100}} over {a:0}, which is reachable and merely far away, and 0.52857 for {a:{$gt:100}} over {a:99}, which one mutation satisfies. So a document that can never be reported as matching outranks a reachable one by more than four times, and the search climbs towards it.
…oduce Twenty queries where the calculator disagrees with the database, and three neighbouring cases that it already answers correctly, kept as guards so that a fix can be told apart from a regression in the path next to it. They extend the section at the end of MongoHeuristicsCalculatorTest, which was written for exactly this, and follow its convention: every expected value was taken from a MongoDB 7.0.41 server answering the same query against the same document, one test per defect, and @disabled with the reason on the ones that fail today. Where the database refuses the query outright there is no answer to copy, so those tests assert that the calculator answers false without throwing, that being the only answer available to it. Four of them concern values compareNonNullValues has no branch for, being two sub-documents or two binary values, which reach the case for types that cannot be compared. That answers false for $eq, missing the plainest nested-object query there is, and true for $ne and $nin, which is a match the database does not have. On master these threw instead, so this is a step forward that stops short. Six of the queries make the calculator throw, and MongoHandler catches nothing around computeDistanceDocuments, so the exception costs the action its whole ExtraHeuristicsDto, the SQL heuristics included. Two of those six are queries the database answers normally: $regex against an array holding no strings, and $not holding a bare regex.
LautaroPetaccio
left a comment
LautaroPetaccio
left a comment
There was a problem hiding this comment.
One more in code this pull request rewrote, plus two that are not yours but sit next to the ones already noted.
{a:{$gt:null}} and its three siblings are not parsed, so the calculator throws. mongo 7.0.41 runs all four and matches nothing, and $eq and $ne given null answer normally, so the ordering operators are the odd ones out in their own family. $exists given anything but a boolean throws the same way, where mongo takes any value and reads it as a truth value, so {$exists:1} and {$exists:0} are both queries it accepts and answers. Both selectors are untouched here, so this is the same route as testOperatorsThatAreNotModelledDoNotThrow rather than anything new.
Gradients are in good shape otherwise. Over fifteen families of documents ordered from nearest to furthest from matching, fourteen fall away monotonically, including $size, $mod, the bit operators, $all, $elemMatch and $gt over an array. The exception is string equality, where any difference in length flattens the score to the same value whatever the content, so "help" and "zzzz" score identically against "hello" and both score below "zzzzz". That one comes from calculateTruthnessForStringComparison, which master calls the same way, so it is not this pull request's doing.
| } | ||
|
|
||
| Truthness res = buildAndAggregationTruthness(expectedValues.stream() | ||
| .map(expectedValue -> helper.computeHeuristicContainsElement(expectedValue, actualValues)) |
There was a problem hiding this comment.
$all accepts $elemMatch among its elements, and each one then has to be satisfied by some element of the array. Here every element of $all is compared for equality against the elements of the field, so the document holding the operator never equals any of them.
{a:{$all:[{$elemMatch:{$gt:2}}]}} vs {a:[1,5]} mongo 1, we do not match
{a:{$all:[{$elemMatch:{$gt:4}},{$elemMatch:{$lt:2}}]}} vs {a:[1,5]} mongo 1, we do not match
The second one is the shape that makes the operator worth having: 5 satisfies the first condition and 1 the second, which no single $elemMatch could express.
The gradient points the same way: ofTrue 0.34390 for an array that cannot be reported as matching. Worth contrasting with the conditions that are equally unreachable but correctly flat, $size against a scalar and $regex against a number, which both sit at 0.10000 and offer the search nothing to climb.
…tics-gaps Tests for the Mongo semantics the heuristics calculator does not reproduce
3 participants
Correct MongoDB query heuristics for arrays, null and missing fields, numeric comparisons, and bitwise operations. Extract shared evaluation logic into MongoHeuristicsCalculatorHelper.