Fix NPE in query param detection for $ref parameters#1882
Merged
Conversation
VellummyilumVinoth
requested review from
TharmiganK,
daneshk,
lnash94 and
shafreenAnfar
as code owners
📝 WalkthroughWalkthroughAdds a unit test and OpenAPI test fixture, and updates ClientDocCommentGenerator to resolve Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java (1)
326-334: Existing code has same potential null safety issues.For reference, the existing code block at lines 326-334 has the same pattern and potential NPE issues. The new fix is consistent with this existing implementation, but both would benefit from null checks. Consider addressing this as a follow-up refactor for the entire method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java` around lines 326 - 334, The loop over operation.getParameters() can still throw NPEs when components or their parameters map is null; update the block handling parameter references (the lambda using parameter.get$ref()) to defensively check that GeneratorMetaData.getInstance().getOpenAPI() and its getComponents() and getParameters() are non-null before calling get(...), and after lookup ensure the resolved parameter is non-null before assigning back to the loop variable; also keep the existing InvalidReferenceException catch but return only when resolution truly fails. Use the existing methods extractReferenceType(...) and InvalidReferenceException to locate the logic to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java`:
- Around line 315-325: In ClientDocCommentGenerator, guard the chained calls to
GeneratorMetaData.getInstance().getOpenAPI().getComponents() and
.getParameters() used when resolving a parameter $ref (the hasQueryParams lambda
and the subsequent forEach block) by first checking components and parameters
are non-null before calling .get(extractReferenceType(...)); if either is null,
treat it as a missing reference (return false in the predicate or skip
processing in the forEach). Keep the existing InvalidReferenceException
handling, and apply the same null-safe checks to both the hasQueryParams
stream.anyMatch lambda and the forEach block that follows so NPEs are prevented
when components or parameters are absent.
---
Nitpick comments:
In
`@openapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java`:
- Around line 326-334: The loop over operation.getParameters() can still throw
NPEs when components or their parameters map is null; update the block handling
parameter references (the lambda using parameter.get$ref()) to defensively check
that GeneratorMetaData.getInstance().getOpenAPI() and its getComponents() and
getParameters() are non-null before calling get(...), and after lookup ensure
the resolved parameter is non-null before assigning back to the loop variable;
also keep the existing InvalidReferenceException catch but return only when
resolution truly fails. Use the existing methods extractReferenceType(...) and
InvalidReferenceException to locate the logic to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4eadaf7-0b08-46de-a261-f4e1c065078c
📒 Files selected for processing (4)
openapi-cli/src/test/java/io/ballerina/openapi/CodeGeneratorTest.javaopenapi-cli/src/test/resources/expected_gen/ref_query_parameters.balopenapi-cli/src/test/resources/ref_query_parameters.yamlopenapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java
VellummyilumVinoth
force-pushed
the
fix-NPE
branch
from
a55b33d to
eb63d5b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java`:
- Around line 315-325: The lambda used to compute hasQueryParams can NPE when a
well-formed $ref is not present in components; after retrieving the referenced
parameter via
GeneratorMetaData.getInstance().getOpenAPI().getComponents().getParameters().get(extractReferenceType(parameter.get$ref())),
add a null check and treat a null result the same as catching
InvalidReferenceException (i.e., return false), so that you only call
parameter.getIn() when parameter is non-null; update the anonymous lambda in
ClientDocCommentGenerator's hasQueryParams computation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 037bd0c2-e973-4b57-8299-4653e41f79a8
📒 Files selected for processing (4)
openapi-cli/src/test/java/io/ballerina/openapi/CodeGeneratorTest.javaopenapi-cli/src/test/resources/expected_gen/ref_query_parameters.balopenapi-cli/src/test/resources/ref_query_parameters.yamlopenapi-core/src/main/java/io/ballerina/openapi/core/generators/document/ClientDocCommentGenerator.java
🚧 Files skipped from review as they are similar to previous changes (2)
- openapi-cli/src/test/resources/expected_gen/ref_query_parameters.bal
- openapi-cli/src/test/java/io/ballerina/openapi/CodeGeneratorTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
$Subject
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Overview
This pull request fixes a NullPointerException encountered when the code generator processes OpenAPI parameters defined via $ref. The change improves stability by resolving referenced parameter definitions before inspecting their location.
Changes
Impact
Prevents runtime exceptions during documentation/code generation for APIs that reference parameter definitions, ensuring correct detection of query parameters and more reliable generation behavior.