FIX check for required query parameters for services #1665
Conversation
Summary of ChangesHello @jeandersonbc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the API client generation template where required query parameters were not being enforced. The changes ensure that generated service methods will explicitly validate the presence of required query parameters at runtime, preventing potential API call failures due to missing essential data. This fix improves the robustness and reliability of the generated API clients by providing immediate feedback when a required parameter is omitted. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds a check for required query parameters, throwing an IllegalArgumentException if they are missing. This is a good improvement for API robustness. I've added one suggestion to refactor the template logic to be more efficient by using a single loop instead of two to process the query parameters. This will improve maintainability and performance.
| {{! Add first all required query params }} | ||
| {{#queryParams}} | ||
| if ({{{paramName}}} != null) { | ||
| {{#required}} | ||
| if ({{{paramName}}} == null) { | ||
| throw new IllegalArgumentException("Please provide the {{{paramName}}} query parameter"); | ||
| } | ||
| queryParams.put("{{baseName}}", {{{paramName}}}{{^isString}}.toString(){{/isString}}); | ||
| {{/required}} | ||
| {{/queryParams}} | ||
|
|
||
| {{! Then, add all the optional query params }} | ||
| {{#queryParams}} | ||
| {{^required}} | ||
| if ({{{paramName}}} != null) { | ||
| queryParams.put("{{baseName}}", {{{paramName}}}{{^isString}}.toString(){{/isString}}); | ||
| } | ||
| {{/required}} | ||
| {{/queryParams}} |
There was a problem hiding this comment.
The current implementation iterates over the queryParams list twice: once for required parameters and once for optional ones. This can be optimized by combining these into a single loop. This refactoring will make the template more efficient and easier to read.
{{! Process all query params }}
{{#queryParams}}
{{#required}}
if ({{{paramName}}} == null) {
throw new IllegalArgumentException("Please provide the {{{paramName}}} query parameter");
}
queryParams.put("{{baseName}}", {{{paramName}}}{{^isString}}.toString(){{/isString}});
{{/required}}
{{^required}}
if ({{{paramName}}} != null) {
queryParams.put("{{baseName}}", {{{paramName}}}{{^isString}}.toString(){{/isString}});
}
{{/required}}
{{/queryParams}}
There was a problem hiding this comment.
I tried this locally, so as expected, you loop once but you have required/optionals mixed (in the order they are declared). I don't have any preference on that, so I'm leaving for reviewers.
The current template does not enforce the presence of required query parameters. The change in the template allows to throw a runtime exception for missing required parameters. Optional query params are added next to the required params with the existing null check.
jeandersonbc
force-pushed
the
fix-required-query-params
branch
from
f283499 to
af73c26
Compare
Description
The current template does not enforce the presence of required query parameters. The change in the template allows to throw a runtime exception for missing required parameters. Optional query params are added next to the required params with the existing null check.
Tested scenarios
CapitalService-v1.jsonspec with a sequence of required/optional query params and re-generated the code.Fixed issue: