fix: Relation column is always present in the query result regardless of keys/excludeKeys query parameter #9876
Conversation
… masked when relation fields always returned
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds a test for keys/excludeKeys behavior on relation fields. Updates Mongo and Postgres storage adapters to prune relation fields not requested by keys/excludeKeys. Adjusts GraphQL field normalization to strip nested "edges.node" segments. Minor formatting edits in GraphQL utilities and types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as Parse Server API
participant Adapter as StorageAdapter (Mongo/Postgres)
participant DB as Database
Client->>API: Query (where, keys/excludeKeys)
API->>Adapter: find(className, query, { keys/excludeKeys })
Adapter->>DB: Execute DB query
DB-->>Adapter: Results (raw)
Adapter->>Adapter: Map DB objects -> Parse objects
rect rgb(235,245,255)
note right of Adapter: Post-map filtering (new)
Adapter->>Adapter: If keys provided, remove Relation fields not in keys\nIf excludeKeys provided, remove excluded Relation fields
end
Adapter-->>API: Filtered Parse objects
API-->>Client: Response
sequenceDiagram
autonumber
participant GQL as GraphQL Layer
participant Loader as parseClassQueries / parseClassTypes
participant API as Parse Server API
GQL->>Loader: Selected fields (may include edges.node)
Loader->>Loader: Normalize fields: strip leading "edges.node." and any ".edges.node" segments
Loader-->>API: keys/include projection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
swittk
changed the title
swittk
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Adapters/Storage/Mongo/MongoStorageAdapter.js (1)
667-690: LGTM! Proper guard conditions and filtering logic.The implementation correctly filters non-requested Relation fields with proper guard conditions (
Array.isArray(keys) && keys.length > 0). This approach is safer than the Postgres adapter version and should be used as the reference pattern.Optional: Consider extracting shared logic.
The filtering logic is duplicated between Mongo and Postgres adapters. Consider extracting it into a shared utility function to reduce duplication and ensure consistency:
// In a shared utility file function filterRelationFields(parseObject, schema, requestedKeys) { if (!Array.isArray(requestedKeys) || requestedKeys.length === 0) { return parseObject; } const keysSet = new Set(requestedKeys); Object.keys(schema.fields).forEach(fieldName => { if (schema.fields[fieldName].type === 'Relation' && !keysSet.has(fieldName)) { delete parseObject[fieldName]; } }); return parseObject; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
spec/ParseQuery.spec.js(1 hunks)src/Adapters/Storage/Mongo/MongoStorageAdapter.js(1 hunks)src/Adapters/Storage/Postgres/PostgresStorageAdapter.js(2 hunks)src/GraphQL/loaders/parseClassQueries.js(1 hunks)src/GraphQL/loaders/parseClassTypes.js(2 hunks)src/GraphQL/parseGraphQLUtils.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Adapters/Storage/Mongo/MongoStorageAdapter.js (2)
src/Adapters/Storage/Mongo/MongoTransform.js (1)
mongoObjectToParseObject(1105-1293)src/RestQuery.js (3)
className(962-962)keys(188-191)keys(971-971)
spec/ParseQuery.spec.js (1)
spec/schemas.spec.js (3)
Parse(3-3)request(6-6)masterKeyHeaders(143-147)
src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)
src/RestQuery.js (4)
keys(188-191)keys(971-971)results(752-752)className(962-962)
🔇 Additional comments (6)
src/GraphQL/parseGraphQLUtils.js (1)
23-25: LGTM! Formatting improvement.The multi-line formatting enhances readability without changing behavior.
src/GraphQL/loaders/parseClassQueries.js (1)
110-116: LGTM! Comprehensive edge-node segment removal.The enhanced normalization correctly handles both root-level
edges.node.prefixes and nested.edges.nodesegments within field paths. The explanatory comments clearly document the intent and behavior.src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)
1924-1947: Filtering logic is correct, pending the guard fix above.The post-processing filter correctly removes non-requested Relation fields when keys are specified. The approach mirrors the MongoStorageAdapter implementation (lines 667-690), ensuring consistent behavior across storage backends.
src/GraphQL/loaders/parseClassTypes.js (2)
354-357: LGTM! Formatting adjustment only.Minor formatting change to the
authDataResponsefield declaration with no functional impact.
389-395: LGTM! Consistent field normalization with parseClassQueries.js.The enhanced normalization logic correctly strips both
edges.node.prefixes and nested.edges.nodesegments, mirroring the approach in parseClassQueries.js (lines 110-116). The explanatory comments document the behavior clearly.spec/ParseQuery.spec.js (1)
3773-3836: LGTM! Comprehensive test coverage for relation field filtering.The test thoroughly validates the fix for issue #8500 across four key scenarios:
keys='name'correctly omits the relation fieldkeys='name,children'correctly includes the relation fieldexcludeKeys='children'correctly omits the relation field- Default behavior (no keys/excludeKeys) correctly returns the relation field
The test structure is clear, follows existing patterns in the file, and properly uses async/await with the REST API via the
requesthelper.
| // `users.edges.node.username` -> `users.username`). This preserves the | ||
| // synthetic relation placeholders that Parse injects while still | ||
| // respecting field projections. | ||
| .map(field => field.replace('edges.node.', '').replace(/\.edges\.node/g, '')) |
There was a problem hiding this comment.
issue: i'm not sure to understand what is happening here, because the first replace will strip the edges.node one time, if you try to remove subsequent edges.node like
users.edges.node.aRelation.edges.node.name not sure about the impact and if developers use the "edges.node" inside a object field it can break implementation, i think there is an issue here or wrong approach
What was the issue here ?
There was a problem hiding this comment.
To be honest I'm not exactly sure; I saw the Relation column logic fix as relatively straightforward, but then when I ran the whole test suite I encountered a single test in GraphQL that was failing (ParseGraphQLServer Auto API Schema Data Types should support deep nested creation) ; did a few searches around and found that code block that stripped out the connections.
As far as I understand, the first filter originally had a single replace call only stripping the leading edges.node. segment, which produced values like aRelation.edges.node.name for nested
connection selections. Because those values still contained edges.node, the
final filter removed them entirely before they reached extractKeysAndInclude.
In other words, the original logic intentionally dropped any field that
contained a second connection layer (I'm not sure why that was the case originally, but anyway..).
The hypothesis I had was that, since I prevented the relation column from surfacing in queries when specific keys to return were specified (as we expect), the original relation fields which were originally always sent out regardless of any conditions then stopped appearing for this specific GraphQL test.
So I tried a fix replacing all explicit consecutive .edges.node (which for typical Parse fields shouldn't exist as such explicitly anyway) and that test passed and didn't think too much of it. I would be happy to know if I was correct in the assumption anyway. I must admit that I don't fully get all of the GraphQL part, but I hope I understood it enough to fix the issue.
There was a problem hiding this comment.
I think you need to drop some console.logs on alpha branch to check how this specific GQL test behave, and what is going though the rest call using the GQL api, and then follow the same process on your PR.
You are maybe right something was wrong before, but worked, because keys feature is permissive (like you can select non existant fields)
Keep me updated :)
mtrezza
changed the title
3 participants
Pull Request
Issue
Closes: #8500
Approach
This PR addresses issue #8500 where relation fields are never excluded from the returned objects when excludeKeys or specific keys are specified. This is due to the code of both mongoObjectToParseObject and postgresObjectToParseObject internally always spreading the "Relation" keys to the returned object as pointers.
The approach taken in this PR is we check if specific keys are specified, and if so, we filter out the Relation fields outside. (A different approach would require passing in those specific keys into mongoObjectToParseObject and postgresObjectToParseObject so they can skip internally too).
A minimal edit to GraphQL was necessary too due to previous behaviour not traversing edge.node properly for nested fields, meaning it never addressed nested fields properly, which with this PR would fail. This (hopefully) corrects the behaviour to as intended.
Tasks
Summary by CodeRabbit
Bug Fixes
Tests