feat: Skip verify server url #9881
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! ❌ Please fill out all fields with a placeholder |
📝 WalkthroughWalkthroughA new configuration option Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes follow consistent, repetitive patterns for adding a new configuration option across existing structures. The logic change in ParseServer.ts is straightforward conditional wrapping. While affecting 5 files, the modifications are homogeneous and additive, requiring minimal reasoning per file. Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (2)
src/Options/Definitions.js (1)
596-602: Consider adding a security warning to the documentation.The option definition is technically correct and follows the established pattern. However, given the security implications of bypassing server URL verification, consider strengthening the documentation with an explicit warning.
Apply this diff to add a security note:
skipVerifyServerUrl: { env: 'PARSE_SERVER_SKIP_VERIFY_SERVER_URL', help: - 'Set to `true` to skip the server URL verification on startup. This can be useful in environments where the server URL is not accessible from the server itself, such as when running behind a firewall, in certain containerized environments, or in test environments like Jest where the verification may cause issues or unnecessary delays during test execution.<br><br>Default is `false`.', + 'Set to `true` to skip the server URL verification on startup. This can be useful in environments where the server URL is not accessible from the server itself, such as when running behind a firewall, in certain containerized environments, or in test environments like Jest where the verification may cause issues or unnecessary delays during test execution.<br><br>⚠️ Only enable this option when necessary, as server URL verification helps detect configuration errors early. Skipping verification may hide issues that could affect Cloud Code and push notifications.<br><br>Default is `false`.', action: parsers.booleanParser, default: false, },src/ParseServer.ts (1)
377-382: Consider adding a log message when verification is skipped.The conditional logic correctly skips verification when
skipVerifyServerUrlistrue. However, adding an informational log message would help with debugging and make it clear to operators that verification was intentionally skipped.Apply this diff to add logging:
// verify the server url after a 'mount' event is received /* istanbul ignore next */ if (!skipVerifyServerUrl) { api.on('mount', async function () { await new Promise(resolve => setTimeout(resolve, 1000)); ParseServer.verifyServerUrl(); }); + } else { + const logger = (logging as any).logger; + logger.info('Server URL verification is disabled via skipVerifyServerUrl option'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/ParseServer.ts(2 hunks)types/Options/index.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 64-64: 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 15, PostGIS 3.4
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: Docker Build
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (5)
types/Options/index.d.ts (1)
125-125: LGTM! TypeScript declaration is correct.The TypeScript declaration for
skipVerifyServerUrlfollows the established pattern for optional boolean properties in theParseServerOptionsinterface.src/Options/index.js (1)
60-64: LGTM! Flow interface declaration is correct.The Flow type declaration and documentation for
skipVerifyServerUrlare properly formatted and match the patterns used throughout this file.Note: The Biome static analysis error about the
?token is a false positive—Biome is not configured to parse Flow syntax.src/ParseServer.ts (2)
299-306: LGTM! Option destructuring is correct.The
skipVerifyServerUrloption is properly destructured alongside other configuration options.
377-382: Address remaining PR objectives before merge
- No tests cover the new skipVerifyServerUrl option; add specs verifying that verifyServerUrl is skipped when this flag is true.
- External documentation (e.g. README.md or docs site) wasn’t updated; please add or update docs for this feature.
- The planned security check for server URL validation is missing; confirm and implement the required check.
- New Parse Error codes for the JS SDK have not been added; specify which codes and include them in a follow-up commit or PR.
src/Options/docs.js (1)
106-106: LGTM! Documentation follows established patterns.The JSDoc documentation for
skipVerifyServerUrlis properly formatted and aligns with the documentation in other files.Note: If the security warning suggestion for
src/Options/Definitions.jsis accepted, this documentation should be updated to match.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9881 +/- ##
=======================================
Coverage 93.00% 93.01%
=======================================
Files 187 187
Lines 15158 15160 +2
Branches 176 177 +1
=======================================
+ Hits 14098 14101 +3
+ Misses 1048 1047 -1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm not against the new option, but I think the original bug is that See #9882 for how this may be fixed. Feel free to incorporate the changes into your PR, or I'll simply merge my PR and then we can merge yours with only the new option. |
@mtrezza it worked nice before but i seems that Daniel added the timeout to stabilize something, and even if we move the code elsewhere the timeout will still be a problem and introduce a latency of 1sec on each test, if developers use isolated test, the impact of perf is huge. |
|
@Moumouls Did you take a look at my PR? No timeout needed. It's clear why the timeout was added - because the verification should not actually happen on mounting but on startup. |
|
feel free to close this one @mtrezza if you feel your PR resolve the issue and the verifyUrl on stratup with expo backoff works. I think your approach is better than mine :) |
|
@Moumouls sorry, I was just proposing that my PR may solve the root cause. Did you try it out? I've merged #9882 so you can use the alpha release. If you want to add the option to disable the verification, feel free to rebase and we can merge. However, the issue you originally opened this PR for should be solved with #9882. |
|
i think the option is still useful @mtrezza i'll refresh this PR :) |
3 participants
Pull Request
Issue
Adds a new Parse Server option skipVerifyServerUrl that allows skipping the automatic server URL verification that occurs on the mount event.
This option is useful in environments where the server URL is not accessible from the server itself:
Closes: FILL_THIS_OUT
Approach
Tasks
Summary by CodeRabbit
New Features
skipVerifyServerUrlconfiguration option to skip server URL verification on startup. Useful for deployments behind firewalls, in containerized environments, or test scenarios. Configure via environment variablePARSE_SERVER_SKIP_VERIFY_SERVER_URL. Defaults tofalse.