Introduce `MongoDBTextFilterUsage` check #649

philleonard · 2023-05-26T11:48:45Z

What?

A simple check which discourages the usage of Filters#text API in Mongo queries.

Why?

We have observed at Picnic that using MongoDB to serve full text search queries can lead to major cluster outages (Mong server OOMs) when query strings are tokenised into even just a handful of tokens, invoking an index scan (IXSCAN) for each individual token in a query string. As Mongo does not gracefully handle such queries, and it is not possible to ban the usage of this filter on the (Mongo) server-side, our best effort is to introduce a Bug Check. MongoDB advises against implementing FTS queries on Mongo Atlas, and to use alternative solutions like Atlas Search, and other search engine databases like Elastic Search, or by simply implementing bespoke queries.

Implementation notes

Description "Usage of the Mongo full-text search ($text) queries is discouraged" doesn't actually hold true, perhaps there are other reasons why you would need to use the Filters API to obtain the Bson query and not use it in the Java driver - this Bug Check would still flag this. I can add the driver dependencies and match on the execution of a query using the drivers, or we can settle on this being good enough.
Named enum value MONGO, like others in ThirdPartyLibrary unsure why we don't refer to the specific name of the FQC but likely we can revisit later when we need to rename in order to encompass more than just the Filters API in this case.
Excluding the org.mongodb:bson-record-codec artifact , not required for testing, but contains classes violating compatibility with byte code enforcement limited by the max JDK version (11). The other option is to (see commit) ignore the class via Maven byteCodeEnforcement ( org.bson.codecs.record.* ) but I think this would be more disruptive.
Perhaps a more elaborate summary is desired to help others better understand the underlying problem, but typically we keep these pretty succinct?

Suggested commit message

Introduce `MongoFullTextSearchQueryUsage` check (#649) 

Discourage the inefficient usage of `mongodb-driver-core`'s `Filters#text` 
for constructing full-text search queries.

github-actions · 2023-05-26T11:51:18Z

Surviving mutants in this change: 1
Killed mutants in this change: 5

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MongoFullTextSearchQueryUsage`	1	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-05-26T12:01:45Z

Surviving mutants in this change: 1
Killed mutants in this change: 5

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MongoFullTextSearchQueryUsage`	1	5

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice work @philleonard! Just a quick comment :).

rickie · 2023-05-26T12:12:29Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+        || !TEXT_FILTER_INVOCATION.matches(tree, state)) {
+      return Description.NO_MATCH;
+    }
+    return describeMatch(tree);


As we do not have a SuggestedFix we don't need to have this ThirdPartyLibrary.MONGO part. That is to prevent us from suggesting a Mongo fix while someone might not want / need this. Here we are only flagging something, so we can omit that part :).

Ah, I misread. I thought the intention of ThirdPartyLibrary was to list all possibly not present dependencies for bug checks even without a suggested fix. Good catch, will update.

github-actions · 2023-05-26T12:50:07Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoFullTextSearchQueryUsage`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

philleonard · 2023-06-02T09:30:44Z

If I understood @Stephan202 correctly from an offline discussion we can downgrade this to a SUGGESTION and for our fork of error-prone utilise this upcoming feature to PicnicSupermarket/error-prone#33. However this would apply to 25 other SUGGESTIONs if we were to utilise this flag internally.

Question, would there be an option to add an additional tag from an enum to indicate the Picnic escalation on a per check basis using our fork's processing of the annotation yet still ensuring compatibility with error-prone? Something like:

enum PicnicSeverityEscalation {
    WARNING_TO_ERROR,
    SUGGESTION _TO_ERROR,
    SUGGESTION_TO_WARNING
}

or simply just

enum PicnicSeverity {
    ERROR,
    WARNING,
    SUGGESTION
}

such that for usage in @BugPatterns:

@BugPattern(
    summary =
        "Usage of Mongo for full-text search queries via the `$text` operator is discouraged.",
    link = BUG_PATTERNS_BASE_URL + "MongoFullTextSearchQueryUsage",
    linkType = CUSTOM,
    severity = WARNING,
    tags = {PERFORMANCE, PicnicSeverityEscalation.SUGGESTION_TO_WARNING})
...

then in our fork modify BugCheckerInfo to iterate over the tags and override the BugCheckerInfo#defaultSeverity where necessary?

Interested to hear your thoughts!

rickie · 2023-06-07T15:05:09Z

Hi @philleonard,

If I understood @Stephan202 correctly from an offline discussion we can downgrade this to a SUGGESTION and for our fork of error-prone utilise this upcoming feature to PicnicSupermarket/error-prone#33.

Yes we should make this a SUGGESTION. We already use the flag internally (in PSM) so this will already work! The PR you linked is open for reference but it's already included in the release of the fork. It would be nice however if the upstream PR google/error-prone#3301 would be merged. I'll rebase the PR and do another ping there.

W.r.t. your other ideas and suggestions; I think it is easier to just configure/override the severity we want a specific BugChecker to have, so we can use the Error Prone command line flags for that. It would allow us to say, just change the severity of this specific check to WARNING or ERROR, if we do not have the -Xep:AllSuggestionsAsWarnings flag.

Internally, we enable all the checks and only disable the ones that are not applicable on our codebase, or have a high number of false positives for whatever reason 😄.

So what I mentioned above allows us to configure it just like we want to.

github-actions · 2023-06-07T16:59:56Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoFullTextSearchQueryUsage`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-06-10T17:46:27Z

Looks good. All 3 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoFullTextSearchQueryUsage`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Rebased and added a commit. Looks nice!
Only doubt is about the name if it can be shorter, but didn't find a good alternative. Maybe we can drop the Full part?

Suggested commit message:

Introduce `MongoFullTextSearchQueryUsage` check (#649)

rickie · 2023-06-10T17:30:21Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

@@ -32,6 +32,7 @@ public enum ThirdPartyLibrary {
   * @see <a href="https://github.com/google/guava">Guava on GitHub</a>
   */
  GUAVA("com.google.common.collect.ImmutableList"),
+


Let's make sure to not sneak in any unrelated changes 😉.

rickie · 2023-06-10T17:44:49Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+public final class MongoFullTextSearchQueryUsage extends BugChecker
+    implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<ExpressionTree> TEXT_FILTER_INVOCATION =


We usually use _METHOD instead of INVOCATION. In this case I made a suggestion to make it a bit clearer what the matches wants to match. Maybe we can drop the MONGO_ prefix, but now its inline with the class and order of matching.

rickie · 2023-06-10T17:45:01Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+        "Usage of Mongo for full-text search queries via the `$text` operator is discouraged.",
+    link = BUG_PATTERNS_BASE_URL + "MongoFullTextSearchQueryUsage",
+    linkType = CUSTOM,
+    severity = WARNING,


Let's change to a SUGGESTION as discussed in the other comments.

rickie · 2023-06-10T17:46:00Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!TEXT_FILTER_INVOCATION.matches(tree, state)) {


We can do a ?: here, that'll be readable still. We have one occurrence of this in the MoreMatchersTest :).

rickie · 2023-06-10T17:46:11Z

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsageTest.java

+            "class A {",
+            "",
+            "  void m() {",
+            "    Bson allowed = Filters.eq(\"a\", \"b\");",


We don't need the Bson allowed = and other variables :).

rickie · 2023-06-10T17:48:05Z

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsageTest.java

+            "    // BUG: Diagnostic contains:",
+            "    Bson textSearch = Filters.text(\"Some text\");",
+            "    // BUG: Diagnostic contains:",
+            "    Bson textSearch2 = Filters.text(\"Some text\", new TextSearchOptions().caseSensitive(true));",


Added one extra case, just for completeness but might not be that necessary...

rickie · 2023-06-13T08:53:01Z

An extra thought: we could've also used a Checkstyle module here to flag this method. However, I imagine this check being generalized when we find another method we want to flag.

We can have a check that flags multiple specific methods and have a custom message for every thing that we want to flag. Additionally, we should see if we can provide an optional fix for the methods that do have a simple replacement. So the check will contain methods without possible replacement, or simpler replacements then the ones from the FluxFlatMapUsage check.

rickie · 2023-07-02T07:44:21Z

Rebased.

github-actions · 2023-07-02T07:46:42Z

Looks good. All 3 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoFullTextSearchQueryUsage`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Rebased and added a commit. Indeed we might want to eventually generalize this to a check which flags methods with a custom message.

Tweaked suggested commit message, given the proposed rename:

Introduce `MongoDBTextFilterUsage` check (#649)

Stephan202 · 2023-07-23T10:49:08Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+import com.sun.source.tree.MethodInvocationTree;
+
+/**
+ * A {@link BugChecker} that flags usages of Mongo {@code $text} filters used for full text


Here and elsewhere: suggest we spell out MongoDB :)

Stephan202 · 2023-07-23T10:52:43Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary = "Avoid the `$text` operator in Mongo's full-text search queries",


Suggested change

summary = "Avoid the `$text` operator in Mongo's full-text search queries",

summary = "Avoid MongoDB's `$text` filter operator, as it can trigger heavy queries and even cause the server to run out of memory",

Stephan202 · 2023-07-23T10:53:40Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MongoFullTextSearchQueryUsage.java

+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = PERFORMANCE)
+public final class MongoFullTextSearchQueryUsage extends BugChecker


W.r.t. the name: how about MongoDBTextFilterUsage?

Stephan202 · 2023-07-23T10:58:23Z

pom.xml

+                <!-- XXX: Drop this when we forgo enforcement of
+                JDK 11 bytecode version compatibility. See follow-ups from:
+                https://github.com/PicnicSupermarket/error-prone-support/pull/603
+                https://github.com/PicnicSupermarket/error-prone-support/pull/198. -->


For maintainability purposes I propose to omit the links; IIUC even after those changes this exclusion would apply.

Stephan202 · 2023-07-23T11:05:30Z

pom.xml

+            <dependency>
+                <groupId>org.mongodb</groupId>
+                <artifactId>mongodb-driver-core</artifactId>
+                <version>4.9.1</version>


Will upgrade.

github-actions · 2023-07-23T11:14:57Z

Looks good. All 3 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoDBTextFilterUsage`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-07-23T12:36:54Z

Rebased.

github-actions · 2023-07-23T12:39:00Z

Looks good. All 3 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoDBTextFilterUsage`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-07-24T08:14:29Z

Looks good. All 3 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoDBTextFilterUsage`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

…he Java driver

rickie · 2023-07-31T14:58:10Z

Rebased, will merge!

sonarqubecloud · 2023-07-31T15:00:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

github-actions · 2023-07-31T15:00:56Z

Looks good. All 3 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MongoDBTextFilterUsage`	0	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

philleonard requested review from Stephan202 and rickie and removed request for Stephan202 May 26, 2023 11:59

rickie reviewed May 26, 2023

View reviewed changes

rickie force-pushed the pleonard/mongo-text-search branch from 8242ea9 to 91fad12 Compare June 7, 2023 16:57

rickie approved these changes Jun 10, 2023

View reviewed changes

rickie added the new feature label Jun 10, 2023

rickie added this to the 0.12.0 milestone Jun 10, 2023

Stephan202 modified the milestones: 0.12.0, 0.13.0 Jun 21, 2023

rickie force-pushed the pleonard/mongo-text-search branch from b7825c4 to 3a782a7 Compare July 2, 2023 07:44

Stephan202 force-pushed the pleonard/mongo-text-search branch from 3a782a7 to 81441bb Compare July 23, 2023 11:12

Stephan202 approved these changes Jul 23, 2023

View reviewed changes

rickie force-pushed the pleonard/mongo-text-search branch from 81441bb to 94ee99e Compare July 23, 2023 12:36

rickie force-pushed the pleonard/mongo-text-search branch from 94ee99e to 50030d8 Compare July 24, 2023 08:11

philleonard and others added 6 commits July 31, 2023 16:58

Introduce a BugCheck for the useage of Mongo full text searches via t…

4ac184a

…he Java driver

Exclude the org.mongodb:bson-record-codec dependency

c07e073

Better summary

f77e129

Drop enumeration of Mongo core in ThirdPartyLibrary

25d80f8

Suggestions and partially revert unrelated changes

1e8a076

Suggestions

521b1b9

rickie force-pushed the pleonard/mongo-text-search branch from 50030d8 to 521b1b9 Compare July 31, 2023 14:58

rickie merged commit e03d0de into master Jul 31, 2023

rickie deleted the pleonard/mongo-text-search branch July 31, 2023 15:19

Stephan202 changed the title ~~Introduce MongoFullTextSearchQueryUsage check~~ Introduce MongoDBTextFilterUsage check Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `MongoDBTextFilterUsage` check #649

Introduce `MongoDBTextFilterUsage` check #649

philleonard commented May 26, 2023 •

edited

Loading

github-actions bot commented May 26, 2023

github-actions bot commented May 26, 2023

rickie left a comment

rickie May 26, 2023

philleonard May 26, 2023

github-actions bot commented May 26, 2023

philleonard commented Jun 2, 2023

rickie commented Jun 7, 2023

github-actions bot commented Jun 7, 2023

github-actions bot commented Jun 10, 2023

rickie left a comment •

edited

Loading

rickie Jun 10, 2023

rickie Jun 10, 2023

rickie Jun 10, 2023

rickie Jun 10, 2023

rickie Jun 10, 2023

rickie Jun 10, 2023

rickie commented Jun 13, 2023

rickie commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

Stephan202 left a comment

Stephan202 Jul 23, 2023

Stephan202 Jul 23, 2023

Stephan202 Jul 23, 2023

Stephan202 Jul 23, 2023

Stephan202 Jul 23, 2023

github-actions bot commented Jul 23, 2023

rickie commented Jul 23, 2023

github-actions bot commented Jul 23, 2023

github-actions bot commented Jul 24, 2023

rickie commented Jul 31, 2023

sonarqubecloud bot commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

	summary = "Avoid the `$text` operator in Mongo's full-text search queries",
	summary = "Avoid MongoDB's `$text` filter operator, as it can trigger heavy queries and even cause the server to run out of memory",

Introduce MongoDBTextFilterUsage check #649

Introduce MongoDBTextFilterUsage check #649

Conversation

philleonard commented May 26, 2023 • edited Loading

What?

Why?

Implementation notes

Suggested commit message

github-actions bot commented May 26, 2023

github-actions bot commented May 26, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 26, 2023

philleonard commented Jun 2, 2023

rickie commented Jun 7, 2023

github-actions bot commented Jun 7, 2023

github-actions bot commented Jun 10, 2023

rickie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Jun 13, 2023

rickie commented Jul 2, 2023

github-actions bot commented Jul 2, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 23, 2023

rickie commented Jul 23, 2023

github-actions bot commented Jul 23, 2023

github-actions bot commented Jul 24, 2023

rickie commented Jul 31, 2023

sonarqubecloud bot commented Jul 31, 2023

github-actions bot commented Jul 31, 2023

Introduce `MongoDBTextFilterUsage` check #649

Introduce `MongoDBTextFilterUsage` check #649

philleonard commented May 26, 2023 •

edited

Loading

rickie left a comment •

edited

Loading