fix: Add objectParser for ObjectTypeAnnotation (e.g. PARSE_SERVER_AUTH_PROVIDERS) #9912
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! |
📝 WalkthroughWalkthroughAdded ObjectTypeAnnotation handling in mapperFor to return Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment (env var)
participant Mapper as buildConfigDefinitions.mapperFor
participant Parsers as parsers module
participant Options as ParseServerOptions.auth
note over Env,Mapper `#E8F6FF`: Map AST type annotation → parser resolver
Env->>Mapper: provide AST type annotation node
alt ObjectTypeAnnotation
Mapper-->>Parsers: resolve to parsers.objectParser
else ArrayTypeAnnotation
Mapper-->>Parsers: resolve to parsers.arrayParser
else BooleanTypeAnnotation
Mapper-->>Parsers: resolve to parsers.booleanParser
else NumberTypeAnnotation
Mapper-->>Parsers: resolve to parsers.numberParser
else GenericTypeAnnotation
Mapper-->>Parsers: resolve to specific parser or default
end
Parsers-->>Options: assign action = chosen parser (auth → objectParser)
note over Parsers,Options `#E8F6FF`: Runtime use: parse env string into object via objectParser
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
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 (1)
resources/buildConfigDefinitions.js (1)
176-242: Consider adding ObjectTypeAnnotation handling for future-proofing.The
parseDefaultValuefunction handles various type annotations but doesn't include a case forObjectTypeAnnotation. While this isn't currently an issue (since theauthoption likely doesn't have a default value), adding this handling would make the function more complete and prevent potential issues if default values are added to object-type fields in the future.Consider adding after line 214:
} else if (t.isBooleanTypeAnnotation(elt)) { literalValue = t.booleanLiteral(parsers.booleanParser(value)); + } else if (t.isObjectTypeAnnotation(elt)) { + const object = parsers.objectParser(value); + const props = Object.keys(object).map(key => { + const val = object[key]; + if (typeof val === 'string') { + return t.objectProperty(t.identifier(key), t.stringLiteral(val)); + } else if (typeof val === 'number') { + return t.objectProperty(t.identifier(key), t.numericLiteral(val)); + } else if (typeof val === 'boolean') { + return t.objectProperty(t.identifier(key), t.booleanLiteral(val)); + } else { + return t.objectProperty(t.identifier(key), t.identifier('undefined')); + } + }); + literalValue = t.objectExpression(props); } else if (t.isGenericTypeAnnotation(elt)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/buildConfigDefinitions.js(1 hunks)src/Options/Definitions.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/Options/Definitions.js
🧬 Code graph analysis (1)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
⏰ 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: Node 20
- GitHub Check: Node 22
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Docker Build
🔇 Additional comments (2)
src/Options/Definitions.js (1)
110-115: LGTM! Generated code correctly adds objectParser action.The addition of
action: parsers.objectParserto theauthoption is consistent with other object-type configuration options throughout this file (e.g.,accountLockout,customPages,fileUpload). This ensures that thePARSE_SERVER_AUTH_PROVIDERSenvironment variable is properly parsed as a JSON object rather than treated as a raw string, which directly addresses the PR objective.Note: This is generated code. The actual source changes are in
src/Options/index.jsand the generator scriptresources/buildConfigDefinitions.js.resources/buildConfigDefinitions.js (1)
156-157: LGTM! ObjectTypeAnnotation handling correctly added.The addition of
ObjectTypeAnnotationhandling inmapperForis correct and consistent with the existingAnyTypeAnnotationhandling. This ensures Flow object types (e.g.,?{ [string]: AuthAdapter }) are properly mapped toobjectParser.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9912 +/- ##
==========================================
- Coverage 93.08% 91.91% -1.17%
==========================================
Files 187 188 +1
Lines 15275 15466 +191
Branches 177 177
==========================================
- Hits 14219 14216 -3
- Misses 1044 1238 +194
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…into fix/auth-env-bug
…rse-server into fix/auth-env-bug
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/buildConfigDefinitions.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
🧬 Code graph analysis (1)
spec/buildConfigDefinitions.spec.js (1)
resources/buildConfigDefinitions.js (1)
wrap(148-148)
🔇 Additional comments (1)
spec/buildConfigDefinitions.spec.js (1)
40-185: Good test coverage for the new ObjectTypeAnnotation mapping.The test suite provides comprehensive coverage of the
mapperForfunction, including the newly addedObjectTypeAnnotationmapping that this PR introduces. All major type annotation cases are covered.However, please address the issues raised in previous comments regarding:
- Importing the actual implementation instead of recreating it locally
- Correcting the Babel AST structure access patterns
- Verifying the source of the
nameproperty
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (2)
resources/buildConfigDefinitions.js(2 hunks)spec/buildConfigDefinitions.spec.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/buildConfigDefinitions.spec.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
resources/buildConfigDefinitions.js
🔇 Additional comments (1)
resources/buildConfigDefinitions.js (1)
158-159: LGTM! ObjectTypeAnnotation handling correctly implemented.The new handling for
ObjectTypeAnnotationcorrectly maps Flow object types toobjectParser, which fixes the issue where environment variables likePARSE_SERVER_AUTH_PROVIDERSwere treated as raw strings. The implementation is consistent with the existingAnyTypeAnnotationhandling and properly placed in the type-checking flow.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (2)
resources/buildConfigDefinitions.js(2 hunks)spec/buildConfigDefinitions.spec.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/buildConfigDefinitions.spec.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
resources/buildConfigDefinitions.js
🧬 Code graph analysis (1)
resources/buildConfigDefinitions.js (1)
spec/buildConfigDefinitions.spec.js (2)
t(1-1)require(2-2)
⏰ 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: Benchmarks
🔇 Additional comments (3)
resources/buildConfigDefinitions.js (3)
158-159: LGTM! ObjectTypeAnnotation handling correctly implemented.The addition properly handles Flow's
ObjectTypeAnnotationby mapping it toobjectParser, which aligns with the PR objective to fix environment variable parsing for object-typed configuration options likePARSE_SERVER_AUTH_PROVIDERS.
377-388: Execution guard correctly prevents side effects during test imports.The transformation is now properly guarded with
if (require.main === module), preventing file system writes when the module is imported by tests. This addresses the concern raised in the previous review.✅ Based on past review comments
391-391: LGTM! Export enables testing without side effects.Exporting
mapperForallows unit tests to verify the type-to-parser mappings without triggering the file generation logic, working correctly with the execution guard above.
|
Hi @mtrezza, Thanks! |
| }); | ||
| require('fs').writeFileSync('./src/Options/Definitions.js', res.code + '\n'); | ||
| require('fs').writeFileSync('./src/Options/docs.js', docs); | ||
| // Only run the transformation when executed directly, not when imported by tests |
There was a problem hiding this comment.
What's the reason for this change?
4 participants
Pull Request
Issue
Closes: #9907
The issue occurs because the
mapperForfunction inbuildConfigDefinitions.jsdid not include a specific case for theObjectTypeAnnotationtype in Flow definitions.As a result, configuration options defined as Flow objects - such as:
auth: ?{ [string]: AuthAdapter };were not mapped to the appropriate parser (objectParser) when generating CLI definitions.
This caused environment variables like PARSE_SERVER_AUTH_PROVIDERS to be treated as raw strings instead of being parsed as JSON objects.
In practice, this broke the built-in Microsoft authentication adapter (and any other adapter relying on object-type auth configs), throwing errors such as:
Error: Microsoft options are required.The missing objectParser action in the generated definitions.js file prevented the environment variable from being correctly parsed into an object structure.
Approach
mapperForfunction inbuildConfigDefinitions.jsto handleObjectTypeAnnotationby mapping it to theobjectParser, ensuring object types are parsed correctly.objectParseraction to theauthoption inParseServerOptionsinDefinitions.js, enabling proper parsing of theauthconfiguration as an object.Tasks
Summary by CodeRabbit
Bug Fixes
Tests
Chores