[CALCITE-7668] MongoDB adapter should escape embedded quotes in field references - #5109
Conversation
|
We do not oppose using AI tools to contribute code. Multiple reviewers have repeatedly advised creating a Jira ticket before submitting contributions—you acknowledged this but have yet to follow through. Moving forward, non-compliant PRs will no longer be reviewed. |
| import static org.hamcrest.MatcherAssert.assertThat; | ||
|
|
||
| /** Tests for {@link MongoRules}. */ | ||
| class MongoRulesTest { |
There was a problem hiding this comment.
I am not crazy about these tests, but a complete query covering these new rules whose output is validated on Mongo would be nice.
There was a problem hiding this comment.
Fair. Dropped MongoRulesTest and replaced it with an end-to-end test in MongoAdapterTest: the datatypes collection now carries two fields whose names are x', injected: {$literal: 1}, y: 'z and a\, and the test selects them through _MAP, checks the generated $project stage, and checks the values Mongo returns.
On the old code the first query parses as {$project: {C: '$x', injected: {$literal: 1}, y: 'z'}}, so the injected field lands in the pipeline and the query fails; with the escaping it returns C=quoted. :mongodb:test is green (53 passed, 6 pre-existing skips).
There was a problem hiding this comment.
If this query was validated against a real mongodb installation, please add a comment saying so.
We have had incorrect "expected" test results in the past.
There was a problem hiding this comment.
The new test has been placed in MongoAdapterTest, so I guess this was correctly validated.
There was a problem hiding this comment.
Yes, the expected values come from running the query against the Mongo instance the test spins up, not hand-written. Added a comment on the test method spelling that out.
|
Also still on the Jira: my ASF account request hasn't come through yet, so the PR title isn't prefixed with a CALCITE- id. I'll file the ticket and retitle this and my other open PRs as soon as it does. |
|
You don't need an ASF account, just a JIRA account to file issues. |
|
Got it, I had the two mixed up. Requesting the JIRA account now and I'll file the ticket for this one, then retitle this PR (and #5075, #5106) with the CALCITE- ids. Nothing else pending on the code side: the unit test you weren't keen on is gone, replaced by an end-to-end case in MongoAdapterTest that runs the query against Mongo and checks both the generated $project stage and the returned values. |
rubenada
changed the title
|
I have created https://issues.apache.org/jira/browse/CALCITE-7668 to track this issue. |
|
Fix LGTM. |
|
I have approved, but left a comment. |
|
Thanks for filing the issue. |
|
Friendly reminder: Please don't forget the comments on mihai. |
|
We have several approvals, so this should be ready for merge. |
rubenada
added
the
LGTM-will-merge-soon
c014cf7 to
3a496ce
Compare
|
Done, squashed into a single commit with that message. |
|
5 participants
Changes Proposed
MongoDB pipeline injection through unescaped quotes in pushed-down field references
The Mongo adapter renders
$project/$groupstages as strings that are later parsed withBsonDocument.parse.MongoRules.quotewrapped a value in single quotes but left embedded single quotes and backslashes untouched (the old// TODO: handle embedded quotes), and theITEMpath that handles_MAP['key']built'$key'the same way. Since the item key and field names come straight from the SQL statement, a value carrying a single quote closes the quoted token early and the rest is parsed as further pipeline syntax.Reproduction: a projection such as
SELECT _MAP['x'', evil: {$literal: 1}, y: ''z'] FROM mongogenerates{$project: {..., evil: {$literal: 1}, ...}}, so an extra stage field lands in the pipeline sent to MongoDB. After the fix the key stays a single string.The escaping lives in
quotebecause that is the one place every field reference passes through; the item-key branch now routes through it, and the aggregate paths already call it. Added a unit test that fails on the old code.