-
Couldn't load subscription status.
- Fork 88
API push from API designer will be created as V4 API #13816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @aurelien-pacaud, 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 modernizes the API creation and management flow originating from the API designer. It introduces the capability to create new APIs as V4 APIs by default, while maintaining support for updating existing V2 APIs. The underlying service logic has been refactored to consolidate API deployment and state management, leading to a more streamlined and robust system for handling different API definition versions. Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the API deployment from the API designer to create v4 APIs by default, which aligns with the stated goal. The changes simplify the command handling by consolidating logic into DeployModelCommandHandler and removing now-redundant handlers. The separation of v2 API updates from v4 API creation is clear and improves code structure. The new logic is also well-supported by new and updated tests. My review includes a few suggestions to enhance exception handling and improve maintainability by replacing hardcoded strings with constants.
| final var executionContext = new ExecutionContext(getEnvironment(environmentId)); | ||
| final var user = userService.findBySource(executionContext.getOrganizationId(), "cockpit", userId, true); | ||
| final var optApiId = apiSearchService.findIdByEnvironmentIdAndCrossId(executionContext.getEnvironmentId(), apiCrossId); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try block starts after several calls that could throw runtime exceptions, such as getEnvironment() (line 81) and userService.findBySource() (line 82). If an exception like EnvironmentNotFoundException or UserNotFoundException is thrown, it won't be caught by the catch (Exception e) block at line 133. This would result in an unhandled exception instead of a graceful DeployModelReply with an error status. To ensure all potential exceptions during setup are handled correctly, the try block should be moved to before these calls (e.g., before line 75), ensuring the payload variable remains in scope for the catch block.
| logger.warn("Update of v4 API is not yet supported."); | ||
| return Single.just(new DeployModelReply(command.getId(), "Update of v4 API is not yet supported.")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message is hardcoded in both the log and the reply. It's a good practice to extract such strings into a constant to improve maintainability and consistency.
For example, you could define a constant in the class:
private static final String V4_API_UPDATE_NOT_SUPPORTED_MESSAGE = "Update of v4 API is not yet supported.";
logger.warn(V4_API_UPDATE_NOT_SUPPORTED_MESSAGE);
return Single.just(new DeployModelReply(command.getId(), V4_API_UPDATE_NOT_SUPPORTED_MESSAGE));| String errorDetails = "Failed to import API [%s].".formatted(result.getErrorMessage()); | ||
| logger.error(errorDetails); | ||
| } catch (InvalidPathsException ipe) { | ||
| final var errorDetails = "Failed to import API [context path not available]."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message is hardcoded. It's a good practice to extract such strings into constants to improve maintainability and make them easier to manage.
For example, you could define a constant in the class:
private static final String FAILED_TO_IMPORT_API_CONTEXT_PATH_NOT_AVAILABLE = "Failed to import API [context path not available].";
final var errorDetails = FAILED_TO_IMPORT_API_CONTEXT_PATH_NOT_AVAILABLE;| return Single.just(new DeployModelReply(command.getId(), errorDetails)); | ||
| } catch (Exception e) { | ||
| String errorDetails = "Error occurred when importing api [%s]".formatted(payload.modelId()); | ||
| final var errorDetails = "Error occurred when importing api [%s]".formatted(payload.modelId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message format is hardcoded. It's a good practice to extract such strings into constants to improve maintainability.
For example, you could define a constant in the class:
private static final String ERROR_OCCURRED_WHEN_IMPORTING_API = "Error occurred when importing api [%s]";
final var errorDetails = ERROR_OCCURRED_WHEN_IMPORTING_API.formatted(payload.modelId());| .mode(PlanMode.STANDARD) | ||
| .status(io.gravitee.definition.model.v4.plan.PlanStatus.PUBLISHED) | ||
| .definitionVersion(DefinitionVersion.V4) | ||
| .security(PlanSecurity.builder().type("key-less").build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The security type "key-less" is hardcoded. It's better to use a constant for such values to avoid typos and improve maintainability. Please consider defining a constant for this value, for example:
private static final String KEYLESS_PLAN_SECURITY_TYPE = "key-less";
| .security(PlanSecurity.builder().type("key-less").build()) | |
| .security(PlanSecurity.builder().type(KEYLESS_PLAN_SECURITY_TYPE).build()) |
|
|
||
| // Force API deployment if out of sync | ||
| if (!apiStateDomainService.isSynchronized(api, audit)) { | ||
| apiStateDomainService.deploy(api, "Managed by Gravitee Cloud", audit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deployment label "Managed by Gravitee Cloud" is hardcoded. Extracting this to a constant would improve maintainability and reusability.
For example, you could define a constant for this value:
private static final String DEPLOYMENT_LABEL_COCKPIT = "Managed by Gravitee Cloud";
| apiStateDomainService.deploy(api, "Managed by Gravitee Cloud", audit); | |
| apiStateDomainService.deploy(api, DEPLOYMENT_LABEL_COCKPIT, audit); |
| @Builder.Default | ||
| @Valid | ||
| private List<Step> request; | ||
| private List<Step> request = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes necessary? Because it seems to me that this could be nullable. For example, in the case of native, these properties are not useful 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other parts of the code assume both lists are present. To avoid NPEs or null checks, the simplest approach is to always have an empty list.
The tests seem fine; I hope this change doesn't introduce any regressions or bugs.
7527927 to
5374951
Compare
...ain/java/io/gravitee/rest/api/service/cockpit/command/handler/DeployModelCommandHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/gravitee/apim/core/api/use_case/cockpit/DeployModelToApiCreateUseCase.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/gravitee/apim/core/api/use_case/cockpit/DeployModelToApiCreateUseCase.java
Outdated
Show resolved
Hide resolved
bbbf38a to
298cc85
Compare
298cc85 to
b4e1a9a
Compare
Issue
https://gravitee.atlassian.net/browse/CJ-2571
Description
All new API created from API designer will be created as v4 API on APIM side
Additional context
Screen.Recording.2025-10-24.at.08.44.03.mov