feat: Allow option publicServerURL to be set dynamically as asynchronous function
#9803
Conversation
|
🚀 Thanks for opening this pull request! |
|
Warning Rate limit exceeded@mtrezza has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 29 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 (3)
📝 WalkthroughWalkthroughAdds support for dynamic/async Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as handleParseHeaders
participant Config
participant AppCache
participant Router as UsersRouter
participant EmailService
Client->>Middleware: HTTP request
Middleware->>Config: validate app config state
Middleware->>Config: loadKeys()
Note right of Config `#E6F2FF`: resolve underscored async keys\n(e.g. `_publicServerURL`) per request
Config->>AppCache: persist updated config if changed
Middleware->>Middleware: attach resolved config to `req.config`
Middleware->>Router: forward request
Router->>EmailService: trigger reset/verify email flow
EmailService->>Config: read `publicServerURL` or fallback `_publicServerURL`
EmailService->>Client: send email containing resolved URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/middlewares.js (1)
216-216: Consider performance implications of loading keys on every request.The
await config.loadKeys()call adds async overhead to every request. While the placement is correct (after config retrieval, before usage), consider implementing caching or memoization to avoid repeatedly resolving the same functions on subsequent requests.Consider adding a cache invalidation strategy or TTL mechanism to avoid unnecessary function calls:
+ // Only load keys if they haven't been loaded or if cache is expired + if (!config._keysLoaded || (config._keysLoadedAt && Date.now() - config._keysLoadedAt > config.keysCacheTtl)) { await config.loadKeys(); + }src/Config.js (2)
35-35: Define asyncKeys as a constant to avoid duplication.The
asyncKeysarray is defined here and again in theloadKeys()method (line 61). This duplication could lead to inconsistencies.Use the constant defined at the top:
async loadKeys() { - const asyncKeys = ['publicServerURL']; - await Promise.all( asyncKeys.map(async key => {
74-81: Consider edge cases in transformConfiguration.The method correctly moves function values to underscored properties, but should validate that the transformation is safe.
Add validation to ensure the transformation doesn't overwrite existing underscored properties:
static transformConfiguration(serverConfiguration) { for (const key of Object.keys(serverConfiguration)) { if (asyncKeys.includes(key) && typeof serverConfiguration[key] === 'function') { + if (serverConfiguration[`_${key}`]) { + throw new Error(`Configuration conflict: both ${key} and _${key} are defined`); + } serverConfiguration[`_${key}`] = serverConfiguration[key]; delete serverConfiguration[key]; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
spec/index.spec.js(1 hunks)src/Config.js(3 hunks)src/middlewares.js(1 hunks)types/Options/index.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Redis Cache
- GitHub Check: Node 18
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
🔇 Additional comments (1)
src/Config.js (1)
477-477: ```bash
#!/bin/bashSearch for any loadKeys references across the repository to determine when publicServerURL is resolved
rg -n "loadKeys" -C5 .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
mtrezza
changed the title
publicServerUrl
mtrezza
changed the title
publicServerUrlpublicServerURL to be set dynamically as async function
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
spec/index.spec.js (2)
629-638: Test title vs. implementation mismatch; add coverage for direct Promise inputThe test is named “from Promise” but uses an async function returning a Promise. Since the type allows a direct Promise, add coverage for that input, or adjust this test to match its title.
Option A — keep current behavior, fix title:
- it('should load publicServerURL from Promise', async () => { + it('should load publicServerURL from async function', async () => {Option B — keep title, pass a direct Promise:
await reconfigureServer({ - publicServerURL: () => Promise.resolve('https://async-server.com/1'), + publicServerURL: Promise.resolve('https://async-server.com/1'), });Additionally, consider adding a separate test to cover both variants (direct Promise and async function). I can draft the full test block if helpful.
654-665: Add test for direct Promise rejection variantTo fully exercise the accepted input types, also cover when publicServerURL is a rejected Promise directly (not via a function), e.g.:
it('should handle publicServerURL direct Promise rejection', async () => { await reconfigureServer({ publicServerURL: Promise.reject(new Error('Async fetch failed')), }); await expectAsync(new Parse.Object('TestObject').save()).toBeRejected(); });This complements the current async-function rejection path.
🧹 Nitpick comments (1)
spec/index.spec.js (1)
618-627: Also assert mount updates when publicServerURL is loaded dynamicallyStatic config sets config.mount to publicServerURL (see Line 348). To avoid regressions, verify that dynamic resolution updates mount too.
Apply this minimal addition:
const config = Config.get(Parse.applicationId); expect(config.publicServerURL).toEqual('https://myserver.com/1'); + expect(config.mount).toEqual('https://myserver.com/1');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/index.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/index.spec.js
⏰ 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 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Docker Build
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Redis Cache
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (1)
spec/index.spec.js (1)
640-653: Error path coverage for throwing function looks goodThis correctly triggers key loading via save and asserts rejection using async/await style, consistent with repo test preferences.
Moumouls
left a comment
Moumouls
left a comment
There was a problem hiding this comment.
question: Do we expect the function to run on every request? If a developer uses it incorrectly, it could result in massive spam ?
Also, the linked issue mentions a forced restart, but Parse Server has many parameters that don’t support "hot modification". A restart (such as in a containerized environment) is normally expected when environment details change. I’m not sure this kind of feature should actually be implemented.
|
@Moumouls I'll try to answer
A cache mechanism would be nice, but not required for a first simple implementation of this feature. No noticeable performance impact is expected if the param is set as string (status quo). Most important, it's not a breaking change. If a developer decides to set the param to a function, they need to consider side effects, e.g. delay if async, implement own cache mechanism, etc.
We are gradually moving to allow changing parse server options without requiring server restart. Started a few years back, we already have options that allow that. Key: no server restart required, #9798 mentions server restart only as alternative. |
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/index.spec.js(1 hunks)src/Config.js(5 hunks)src/Routers/PagesRouter.js(6 hunks)src/Routers/PublicAPIRouter.js(1 hunks)src/batch.js(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/batch.jssrc/Routers/PublicAPIRouter.jssrc/Config.jsspec/index.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/Routers/PublicAPIRouter.jssrc/Config.jsspec/index.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/Routers/PublicAPIRouter.jssrc/Config.jsspec/index.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Config.jsspec/index.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/index.spec.js
🧬 Code graph analysis (5)
src/batch.js (1)
spec/batch.spec.js (1)
publicServerURL(8-8)
src/Routers/PublicAPIRouter.js (2)
src/Config.js (1)
Config(54-807)src/batch.js (1)
publicServerURL(80-80)
src/Config.js (2)
src/middlewares.js (3)
config(207-207)config(643-643)config(645-645)src/batch.js (1)
publicServerURL(80-80)
spec/index.spec.js (2)
spec/helper.js (2)
reconfigureServer(180-214)Parse(4-4)src/Config.js (1)
Config(54-807)
src/Routers/PagesRouter.js (2)
src/middlewares.js (3)
config(207-207)config(643-643)config(645-645)src/batch.js (1)
publicServerURL(80-80)
⏰ 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). (14)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Node 22
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: Docker Build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Config.js (2)
143-151: Update validation to handle Promise type and improve error message.The validation has two issues:
- Doesn't account for
publicServerURLbeing a directPromise<string>(allowed by the type definition)- The error message is misleading - it doesn't mention that functions and Promises are valid inputs
Apply this diff to fix validation:
if (publicServerURL) { if ( typeof publicServerURL !== 'function' && + !(publicServerURL instanceof Promise) && !publicServerURL.startsWith('http://') && !publicServerURL.startsWith('https://') ) { - throw 'publicServerURL should be a valid HTTPS URL starting with https://'; + throw 'publicServerURL should be a valid HTTPS URL starting with https://, a function returning a URL, or a Promise resolving to a URL'; } }
60-72: Add error handling, Promise support, and value validation.The method has several issues:
- No error handling if functions throw or return invalid values
- Only handles functions returning Promises, not direct Promise values (inconsistent with the type definition that allows
Promise<string>)- No validation of resolved values (e.g., ensuring publicServerURL is a valid URL)
- Calling
AppCache.put(this)on every invocation may be expensiveApply this diff to add comprehensive error handling and Promise support:
async loadKeys() { const asyncKeys = ['publicServerURL']; await Promise.all( asyncKeys.map(async key => { + try { + // Handle both functions and direct Promises if (typeof this[`_${key}`] === 'function') { this[key] = await this[`_${key}`](); + } else if (this[`_${key}`] instanceof Promise) { + this[key] = await this[`_${key}`]; } + + // Validate the resolved value for publicServerURL + if (key === 'publicServerURL' && this[key]) { + if (typeof this[key] !== 'string') { + throw new Error('publicServerURL must resolve to a string'); + } + if (!this[key].startsWith('http://') && !this[key].startsWith('https://')) { + throw new Error('publicServerURL must be a valid HTTP/HTTPS URL'); + } + } + } catch (error) { + throw new Error(`Failed to load ${key}: ${error.message}`); + } }) ); AppCache.put(this.appId, this); }
🧹 Nitpick comments (1)
src/Config.js (1)
35-35: Remove duplicateasyncKeysdeclaration.The
asyncKeysarray is declared here at the module level but then redeclared as a local variable inloadKeys()at line 61. This module-level declaration is unused and should be removed to avoid confusion.Apply this diff:
-const asyncKeys = ['publicServerURL']; export class Config {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/index.spec.js(1 hunks)src/Config.js(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/index.spec.jssrc/Config.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/index.spec.jssrc/Config.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/index.spec.jssrc/Config.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/index.spec.jssrc/Config.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/index.spec.js
🧬 Code graph analysis (1)
spec/index.spec.js (2)
spec/helper.js (2)
reconfigureServer(180-214)Parse(4-4)src/Config.js (1)
Config(36-796)
🔇 Additional comments (2)
spec/index.spec.js (1)
689-854: Comprehensive test coverage looks good.The test suite thoroughly covers the new
publicServerURLfunctionality including:
- Basic function and Promise support
- Error handling for both synchronous throws and Promise rejections
- Per-access function invocation with the counter pattern
- Dynamic URL behavior in password reset and verification emails
All tests follow the repository's async/await patterns without callback-based
done().Note: The
fdescribeat line 689 was already flagged and marked as addressed in previous review comments.Based on learnings.
src/Config.js (1)
74-81: Transformation logic is correct.The
transformConfigurationmethod properly moves function-valued async keys to underscored properties (e.g.,publicServerURL→_publicServerURL), which enables the lazy evaluation pattern inloadKeys(). This design allows the function to be preserved in the cached config for repeated execution at request time.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9803 +/- ##
==========================================
+ Coverage 93.04% 93.06% +0.01%
==========================================
Files 187 187
Lines 15160 15187 +27
Branches 177 177
==========================================
+ Hits 14106 14134 +28
+ Misses 1042 1041 -1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Config.js(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/Config.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/Config.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/Config.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Config.js
⏰ 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). (14)
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Docker Build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/index.spec.js (1)
712-736: Consider verifying specific error details.While these tests correctly verify that operations are rejected when
publicServerURLresolution fails, they don't assert the specific error message or type. This could make debugging harder if the wrong error is thrown.Consider updating the tests to verify error details:
it('should handle publicServerURL function throwing error', async () => { const errorMessage = 'Failed to get public server URL'; await reconfigureServer({ publicServerURL: () => { throw new Error(errorMessage); }, }); - // The error should occur when trying to save an object (which triggers loadKeys in middleware) - await expectAsync( - new Parse.Object('TestObject').save() - ).toBeRejected(); + const error = await new Parse.Object('TestObject').save().catch(e => e); + expect(error.message).toContain(errorMessage); });Apply similar changes to the Promise rejection test to verify the error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/index.spec.js(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/index.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/index.spec.js
🧬 Code graph analysis (1)
spec/index.spec.js (2)
spec/helper.js (3)
reconfigureServer(180-214)Parse(4-4)user(296-296)src/Config.js (1)
Config(36-811)
⏰ 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). (14)
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Node 22
- GitHub Check: Node 20
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
🔇 Additional comments (6)
spec/index.spec.js (6)
366-366: LGTM!The updated error message accurately reflects the new validation requirements for
publicServerURLand provides clear guidance to users.
690-699: LGTM!This test properly verifies that
publicServerURLcan be provided as a function and is correctly resolved when accessed.
701-710: LGTM!This test correctly verifies Promise-based
publicServerURLresolution, addressing the comprehensive coverage requested in past reviews.
738-764: Excellent test for per-request execution!This test effectively verifies that the
publicServerURLfunction is executed on every request rather than being cached, using a counter to prove dynamic resolution. This addresses the critical requirement that the URL can change at runtime without server restart.
766-808: LGTM! Strong integration test.This test effectively verifies that password reset emails use the dynamically resolved
publicServerURLon each request. The counter pattern demonstrates that each email can receive a different URL without server restart, which is the key use case for this feature.
810-853: LGTM! Comprehensive email integration coverage.This test completes the email integration coverage by verifying verification emails also use the dynamically resolved
publicServerURL. Together with the password reset test, this demonstrates that the feature works correctly across different email scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Config.js (2)
182-201: Consider documenting the_publicServerURLfallback logic.The fallback
publicServerURL || _publicServerURLat line 196 is subtle: whenpublicServerURLis a function,transformConfiguration()has moved it to_publicServerURLand deleted the original. The fallback ensures email validation receives either the string or the function.Consider adding a brief comment to clarify this for future maintainers:
static validateControllers({ verifyUserEmails, userController, appName, publicServerURL, _publicServerURL, emailVerifyTokenValidityDuration, emailVerifyTokenReuseIfValid, }) { const emailAdapter = userController.adapter; if (verifyUserEmails) { this.validateEmailConfiguration({ emailAdapter, appName, + // If publicServerURL is a function, it's been moved to _publicServerURL by transformConfiguration publicServerURL: publicServerURL || _publicServerURL, emailVerifyTokenValidityDuration, emailVerifyTokenReuseIfValid, }); } }
466-488: Document expected function signature and reference Promise support issue.The validation accepts a function but doesn't document the expected signature. Developers need to know the function should return a string URL (or
Promise<string>once Promise support is added).Consider adding JSDoc to document the expected types:
+ /** + * Validates the publicServerURL configuration option. + * @param {Object} options - Validation options + * @param {string|function():string|Promise<string>} options.publicServerURL - The public server URL or function that returns it + * @param {boolean} [options.required=false] - Whether the option is required + * @throws {string} If validation fails + */ static validatePublicServerURL({ publicServerURL, required = false }) {Note: There's an existing issue (flagged in past review) that the validation doesn't handle
Promise<string>despite it being mentioned in type definitions. This should be addressed separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Config.js(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/Config.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/Config.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/Config.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Config.js
🔇 Additional comments (3)
src/Config.js (3)
35-38: Good approach for extensibility.The
asyncKeysconstant provides a clear, centralized list of configuration options that support async resolution. This makes it easy to add more dynamic options in the future.
83-90: Clean separation of function definition from resolved value.The transformation correctly moves async function-based config values to underscored properties, enabling lazy evaluation at request time while preserving the function for repeated execution.
490-517: Good refactoring with correct required flag.The change to use the centralized
validatePublicServerURL()helper withrequired: trueis appropriate here, as email verification functionality cannot work without a valid public server URL.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Options/docs.js
🧰 Additional context used
🪛 Biome (2.1.2)
src/Options/index.js
[error] 229-229: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ 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). (14)
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 20
- GitHub Check: Node 22
- GitHub Check: Docker Build
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
🔇 Additional comments (1)
src/Options/Definitions.js (1)
496-500: LGTM!The help text has been correctly updated to document the new async function capability. Since this is a generated file (as noted in the header), the change appropriately reflects the documentation update in
src/Options/index.js.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
types/ParseServer.d.ts (1)
29-33: Consider replacinganytypes withExpress.ApplicationandParseServerOptions.The implementation is correctly implemented and matches the type definition. However, the
anytypes can be improved for better type safety:
apiparameter should beExpress.Application(requires addingimport * as express from 'express'or appropriate Express type import)optionsparameter should beParseServerOptions(already imported in the file)This change would provide developers with proper IDE autocomplete and catch type errors at compile time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Config.js(7 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)types/Options/index.d.ts(1 hunks)types/ParseServer.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Options/index.jssrc/Config.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/Config.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/Config.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/Config.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
src/Config.js
🪛 Biome (2.1.2)
src/Options/index.js
[error] 229-229: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 231-231: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🔇 Additional comments (10)
types/Options/index.d.ts (1)
88-88: LGTM! Type definition aligns with implementation.The updated type correctly represents all supported forms of
publicServerURL(string, sync function, or async function), matching the implementation insrc/Config.jswhereloadKeys()resolves functions viaawait this[_${key}]().src/Options/index.js (1)
229-231: LGTM! Flow type definition is consistent across the codebase.The updated Flow type correctly matches the TypeScript definition and implementation, supporting string, sync function, or async function forms. The expanded comment clearly describes the behavior.
Note: The Biome static analysis errors on lines 229 and 231 are false positives caused by Flow's nullable
?syntax, which Biome misinterprets.src/Config.js (7)
35-38: LGTM! Clean and extensible design.The
asyncKeysarray provides a clear, centralized list of configuration keys that need async resolution, making it easy to extend for future async options.
64-85: LGTM! Robust async key resolution with proper error handling.The implementation correctly:
- Resolves all async keys concurrently via
Promise.all- Wraps resolution in try-catch with descriptive error messages
- Updates AppCache with resolved values to persist changes
- Maintains existing cache structure
87-94: LGTM! Transformation correctly separates function definitions from resolved values.The method properly moves function-based configuration values to underscored properties (e.g.,
_publicServerURL), enabling:
- Function preservation in the cache for repeated evaluation
- Clean separation:
_publicServerURLstores the function,publicServerURLstores the resolved string- Lazy evaluation at request time via
loadKeys()
470-492: LGTM! Comprehensive validation with clear error messages.The centralized validation correctly handles:
- Optional vs required scenarios
- String validation (URL format with http/https protocol)
- Function acceptance (deferred validation until invocation)
- Clear error messages indicating expected types and actual type received
156-156: LGTM! Proper use of centralized validation.The refactoring to use
validatePublicServerURL()improves maintainability by centralizing validation logic. The optional nature (norequiredparameter) is correct for general configuration validation.
186-204: LGTM! Correct fallback logic for async key handling.The addition of
_publicServerURLparameter and fallback logic (publicServerURL || _publicServerURL) is essential for email verification features to work correctly whenpublicServerURLis provided as a function. AftertransformConfiguration(), the function is stored in_publicServerURL, so this fallback ensures it's available for validation.
494-507: LGTM! Email configuration validation correctly enforces required URL.The use of
validatePublicServerURL({ publicServerURL, required: true })is appropriate since email verification and password reset features depend on a valid public URL. The validation will accept either a string URL or a function that resolves to a URL.src/Options/docs.js (1)
90-90: LGTM! Documentation accurately reflects the new behavior.The JSDoc update correctly:
- Changes type to
Unionto represent multiple supported forms- Describes that the option can be a string or asynchronously resolved function
- Documents the URL format requirement (http:// or https://)
- Mentions key use cases (password reset, email verification)
This aligns with the implementation in
src/Config.jsand type definitions across the codebase.
mtrezza
changed the title
publicServerURL to be set dynamically as async functionpublicServerURL to be set dynamically as asynchronous function
|
@coderabbitai review the PR again after the latest commit |
|
@mtrezza: I'll review the PR again with the latest changes. 🧠 Learnings used✅ Actions performedReview triggered.
|
# [8.5.0-alpha.1](8.4.0...8.5.0-alpha.1) (2025-11-07) ### Features * Allow option `publicServerURL` to be set dynamically as asynchronous function ([#9803](#9803)) ([460a65c](460a65c))
|
🎉 This change has been released in version 8.5.0-alpha.1 |
4 participants
Pull Request
Issue
Closes: #9798
Approach
Adds mechanism to load publicServerUrl on
handleParseSessionTasks
Summary by CodeRabbit
New Features
Behavior
Tests
Documentation