feat: Allow option publicServerURL to be set dynamically as async 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 5 minutes and 41 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 (1)
📝 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. 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.✅ 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.
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. |
Pull Request
Issue
Closes: #9798
Approach
Adds mechanism to load publicServerUrl on
handleParseSessionTasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests