feat: Add deprecation on explain #9890
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
✅ 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. |
|
Warning Rate limit exceeded@coratgerl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds deprecation DEPPS12 restricting use of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as REST API
participant Dep as Deprecator
rect rgb(250,240,230)
Note over API: Non-master request with explain
Client->>API: GET /classes/Thing?explain=true
API->>Dep: logRuntimeDeprecation("explain requires master key")
Dep-->>API: recorded
API-->>Client: explain result (returned, deprecated)
end
rect rgb(230,250,240)
Note over API: Master-key request with explain
Client->>API: GET /classes/Thing?explain=true + useMasterKey
API-->>Client: explain result (allowed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
spec/ParseQuery.spec.js (1)
5407-5411: Clean up commented code or document the intention.The commented-out code suggests this deprecation is planned but not yet enforced. If this is a phased approach (warn now, enforce later), consider adding a comment explaining the timeline:
- // fail('Should have thrown an error'); } catch (error) { - // Uncomment this after the Deprecation DEPPS12 - // expect(error.code).toEqual(Parse.Error.INVALID_QUERY); - // expect(error.message).toEqual('Using the explain query parameter without the master key'); + // TODO: Enforce restriction in next major version (DEPPS12) + // expect(error.code).toEqual(Parse.Error.INVALID_QUERY); + // expect(error.message).toEqual('Using the explain query parameter without the master key'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
DEPRECATIONS.md(1 hunks)changelogs/CHANGELOG_alpha.md(1 hunks)spec/ParseQuery.spec.js(1 hunks)src/rest.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/rest.js
🧬 Code graph analysis (2)
src/rest.js (1)
spec/helper.js (1)
auth(394-394)
spec/ParseQuery.spec.js (2)
spec/helper.js (2)
TestObject(279-281)Parse(4-4)src/rest.js (1)
Deprecator(16-16)
🪛 GitHub Check: Lint
spec/ParseQuery.spec.js
[failure] 5394-5394:
'Deprecator' is not defined
🪛 markdownlint-cli2 (0.18.1)
changelogs/CHANGELOG_alpha.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
DEPRECATIONS.md
17-17: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
17-17: Table column count
Expected: 7; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
18-18: Table column count
Expected: 7; Actual: 9; Too many cells, extra data will be missing
(MD056, table-column-count)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
🔇 Additional comments (3)
changelogs/CHANGELOG_alpha.md (1)
1-6: LGTM!The changelog entry follows the established format and clearly documents the DEPPS12 deprecation with appropriate links.
src/rest.js (2)
16-16: LGTM!The Deprecator import is correctly added to support the new deprecation logging functionality.
39-50: LGTM!The deprecation logging is correctly implemented:
- Properly detects when
explainis used without master key authorization- Logs a clear deprecation message without breaking existing functionality
- Preserves the commented error path for future enforcement when the deprecation period ends
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9890 +/- ##
==========================================
- Coverage 93.01% 93.01% -0.01%
==========================================
Files 187 187
Lines 15163 15170 +7
Branches 177 177
==========================================
+ Hits 14104 14110 +6
- Misses 1047 1048 +1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This PR requires a new Parse Server config option to enable / disable explain requiring master key. The option will be deprecated, so this is not a RuntimeDeprecation but a deprecation of the config option.
The new option should be in the DatabaseOptions interface, maybe called allowPublicExplain which is true by default.
Then replace the current test with 3 tests:
- reconfigure server with
allowPublicExplain: trueand ensure it works with and without master key - reconfigure server with
allowPublicExplain: falseand ensure it works with master key and not without master key - do not reconfigure server, i.e. don't explicitly set
allowPublicExplainto test default value and ensure it works with and without master key
…arse-server into feat-add-deprecation-explain
|
Done @mtrezza. Note that I had to remove this line in MongoStorageAdapter because it mutates the databaseOptions. This mutation causes the test to remove the value before it is injected into ParseApp, which then defaults to using the default options. This deletion is weird since we already remove the option below in this._mongoOptions. 
|
3 participants
Pull Request
Issue
Fixes: #7519
Summary by CodeRabbit
New Features
Deprecations
Behavior
Tests