feat: Allow option publicServerURL to be set dynamically as async function
#9803
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds support for dynamic and asynchronous resolution of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Config
Client->>Middleware: HTTP request with appId
Middleware->>Config: get(appId)
Middleware->>Config: await config.loadKeys()
Config->>Config: Resolve async keys (e.g. call _publicServerURL)
Config-->>Middleware: config with resolved publicServerURL
Middleware->>Request: attach app info and config
Middleware-->>Client: continue request processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![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.
Pull Request
Issue
Closes: #9798
Approach
Adds mechanism to load publicServerUrl on
handleParseSessionTasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests