feat: Allow option publicServerURL to be set dynamically as async function
#9803
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughImplements dynamic async Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as handleParseHeaders
participant Config
participant AppCache
participant EmailService
Client->>Middleware: HTTP Request
Middleware->>Config: validate config state
Middleware->>Config: loadKeys()
Config->>Config: resolve async keys<br/>(publicServerURL function)
Config->>AppCache: persist updated config
Middleware->>Middleware: attach config to request
Middleware->>EmailService: send email (password reset/verify)
EmailService->>Config: get publicServerURL<br/>(returns current _publicServerURL)
EmailService->>Client: email with current URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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. |
![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.
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 <[email protected]>
Signed-off-by: Manuel <[email protected]>
Signed-off-by: Manuel <[email protected]>
|
This contains a few bugs, adding more tests... |
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)
spec/index.spec.js(1 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.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 (3)
spec/helper.js (2)
reconfigureServer(180-214)Parse(4-4)src/Config.js (1)
Config(36-796)spec/ValidationAndPasswordsReset.spec.js (1)
MockEmailAdapterWithOptions(3-3)
⏰ 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). (1)
- GitHub Check: Docker Build
🔇 Additional comments (1)
spec/index.spec.js (1)
690-853: Test coverage looks comprehensive.The test suite properly covers:
- Synchronous function returning a string
- Function returning a Promise
- Error handling for both thrown exceptions and rejected Promises
- Per-access execution verification using counters
- Integration with password reset and verification email flows
The counter-based tests (lines 738-853) are particularly valuable as they prevent future regressions where the function might only be executed once and cached. The email integration tests verify that the dynamic publicServerURL is properly used in real user flows.
Based on learnings
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
src/Routers/PagesRouter.js
Outdated
| async getDefaultParams(config) { | ||
| if (!config) { | ||
| return {}; | ||
| } | ||
| const publicServerURL = await config.getPublicServerURL(); | ||
| return { | ||
| [pageParams.appId]: config.appId, | ||
| [pageParams.appName]: config.appName, | ||
| [pageParams.publicServerUrl]: publicServerURL, | ||
| }; |
There was a problem hiding this comment.
Update staticRoute to await the now-async default params.
getDefaultParams now returns a Promise, but staticRoute still calls it synchronously. That means every static page/template receives the unresolved Promise instead of real values—placeholders like {{publicServerUrl}} end up empty and headers get polluted. Convert staticRoute to async and await the call:
- staticRoute(req) {
+ async staticRoute(req) {
// Get requested path
const relativePath = req.params['resource'][0];
@@
- const params = this.getDefaultParams(req.config);
+ const params = await this.getDefaultParams(req.config);That keeps localized/static page rendering working with dynamic publicServerURL.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Routers/PagesRouter.js around lines 534-543, staticRoute still calls
getDefaultParams synchronously while getDefaultParams is now async; change
staticRoute to be async and await getDefaultParams(config) so it receives
resolved values (e.g., const defaults = await this.getDefaultParams(config)),
then merge those defaults into the template render context; update any call
sites of staticRoute to handle the returned Promise (await or then) so rendering
uses real values not unresolved Promises.
src/Routers/PublicAPIRouter.js
Outdated
| if (!config.publicServerURL) { | ||
| return { | ||
| status: 404, | ||
| text: 'Not found.', | ||
| }; | ||
| } | ||
|
|
||
| const publicServerURL = await config.getPublicServerURL(); | ||
|
|
There was a problem hiding this comment.
Do not gate changePassword on the unresolved config.publicServerURL.
When publicServerURL is configured as the new async function, Config.get() strips the public field and keeps only _publicServerURL until getPublicServerURL() executes. The new guard returns the 404 response before you ever await the async resolver, so the password reset page breaks for exactly the async use case this PR introduces. Move the null-check to the resolved value instead:
- if (!config.publicServerURL) {
- return {
- status: 404,
- text: 'Not found.',
- };
- }
-
- const publicServerURL = await config.getPublicServerURL();
+ const publicServerURL = await config.getPublicServerURL();
+ if (!publicServerURL) {
+ return {
+ status: 404,
+ text: 'Not found.',
+ };
+ }This way static strings keep working, and dynamic functions finally do too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!config.publicServerURL) { | |
| return { | |
| status: 404, | |
| text: 'Not found.', | |
| }; | |
| } | |
| const publicServerURL = await config.getPublicServerURL(); | |
| const publicServerURL = await config.getPublicServerURL(); | |
| if (!publicServerURL) { | |
| return { | |
| status: 404, | |
| text: 'Not found.', | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/Routers/PublicAPIRouter.js around lines 98-106, the code currently checks
config.publicServerURL before awaiting config.getPublicServerURL(), which causes
async resolver functions to be treated as missing and returns 404 prematurely;
instead, await config.getPublicServerURL() first, then check the resolved
publicServerURL for null/undefined and only return the 404 if the resolved value
is falsy so both static strings and async functions work correctly.
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.
4 participants
Pull Request
Issue
Closes: #9798
Approach
Adds mechanism to load publicServerUrl on
handleParseSessionTasks
Summary by CodeRabbit
New Features
Tests