feat: Allow option publicServerURL to be set dynamically as async function
#9803
Conversation
|
🚀 Thanks for opening this pull request! |
📝 Walkthrough""" WalkthroughSupport for dynamic and asynchronous configuration of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Config
Client->>Middleware: Incoming request
Middleware->>Config: get(appId)
Middleware->>Config: await config.loadKeys()
Config->>Config: Resolve async keys (e.g., publicServerURL)
Config-->>Middleware: Config with resolved keys
Middleware->>Middleware: Attach config to request
Middleware-->>Client: Continue request handling
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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 -->
| it('should load publicServerURL', async () => { | ||
| await reconfigureServer({ | ||
| publicServerURL: () => 'https://myserver.com/1', | ||
| }); | ||
|
|
||
| await new Parse.Object('TestObject').save(); | ||
|
|
||
| const config = Config.get(Parse.applicationId); | ||
| expect(config.publicServerURL).toEqual('https://myserver.com/1'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for complete functionality.
The test only covers the function case but the type definition also allows Promise<string>. Consider adding test coverage for:
- Promise-based
publicServerURL - Error handling scenarios (function throws, invalid URL returned)
Add comprehensive test coverage:
it('should load publicServerURL from Promise', async () => {
await reconfigureServer({
publicServerURL: Promise.resolve('https://myserver.com/2'),
});
await new Parse.Object('TestObject').save();
const config = Config.get(Parse.applicationId);
expect(config.publicServerURL).toEqual('https://myserver.com/2');
});
it('should handle publicServerURL function errors', async () => {
await expectAsync(reconfigureServer({
publicServerURL: () => { throw new Error('Function failed'); },
})).toBeRejected();
});🤖 Prompt for AI Agents
In spec/index.spec.js around lines 618 to 627, the test only covers the case
where publicServerURL is a function returning a string, but it also supports a
Promise<string> and error scenarios. Add tests to cover when publicServerURL is
a Promise resolving to a URL string and when the function throws an error,
verifying that reconfigureServer rejects appropriately. This will ensure full
coverage of the publicServerURL option's behavior.
mtrezza
changed the title
publicServerUrl
mtrezza
changed the title
publicServerUrlpublicServerURL to be set dynamically as async function
Pull Request
Issue
Closes: #9798
Approach
Adds mechanism to load publicServerUrl on
handleParseSessionTasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests