feat: Add menu item description and rating fields (v1.0.1) - #6
fxb-google wants to merge 5 commits into
Conversation
… version to 1.0.1
|
/gemini review |
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds description and rating fields to the Menu entity, implements validation logic (ratings must be between 1 and 5) in the MenuResource endpoints, updates unit tests to cover these new fields, and switches the database configuration from H2 to PostgreSQL. The review feedback highlights a potential data-overwriting bug in the update method where partial updates can silently reset the primitive spiceLevel field to 0. Additionally, it is recommended to extract the duplicated rating validation logic into a helper method and to revert the test database configuration back to H2 to avoid unnecessary Docker dependencies in CI/CD environments.
|
|
||
| /** Tests updating a menu item with valid rating and description. */ | ||
| @Test | ||
| public void testUpdateMenuSuccess() { |
There was a problem hiding this comment.
In testUpdateMenuSuccess, the update payload only contains description and rating. However, because spiceLevel in Menu.java is a primitive int and is updated unconditionally in MenuResource.java (line 150: entity.spiceLevel = menu.spiceLevel;), any partial update that omits spiceLevel will silently reset it to 0 (the default value for int).
To fix this data-overwriting bug, consider:
- Changing
spiceLevelfrominttoIntegerinMenu.java. - Wrapping the assignment in
MenuResource.javawith a null check:
if (menu.spiceLevel != null) {
entity.spiceLevel = menu.spiceLevel;
}| if (menu.rating < 1 || menu.rating > 5) { | ||
| throw new WebApplicationException("Rating must be an integer between 1 and 5", 400); | ||
| } |
There was a problem hiding this comment.
The rating validation logic (rating < 1 || rating > 5) and its corresponding error message are duplicated between the create and update methods. To improve maintainability and avoid duplication, consider extracting this validation into a private helper method.
| @@ -1,3 +1,2 @@ | |||
| quarkus.datasource.db-kind=h2 | |||
| quarkus.datasource.jdbc.url=jdbc:h2:mem:test | |||
| quarkus.datasource.db-kind=postgresql | |||
There was a problem hiding this comment.
Switching the test database from H2 to PostgreSQL introduces a dependency on Docker (via Quarkus Dev Services) for running unit tests. This can significantly slow down test execution and cause tests to fail in CI/CD environments where Docker is not available or permitted. Unless PostgreSQL-specific features are being utilized, it is recommended to continue using an in-memory H2 database for unit and integration tests to keep them fast and self-contained.
quarkus.datasource.db-kind=h2
quarkus.datasource.jdbc.url=jdbc:h2:mem:test
There was a problem hiding this comment.
Code Review
This pull request introduces a rating and description field to the Menu entity, along with corresponding validation logic in MenuResource and comprehensive unit tests. It also migrates the database configuration from H2 to PostgreSQL for both production and test environments. The review feedback suggests wrapping the validation exceptions in a Response object to ensure descriptive error messages are returned to the client. Additionally, it is recommended to revert the test database configuration back to H2 to avoid requiring a Docker daemon for running tests in CI/CD environments.
| if (menu.rating == null) { | ||
| throw new WebApplicationException("Rating is required and cannot be null", 400); | ||
| } | ||
| if (menu.rating < 1 || menu.rating > 5) { | ||
| throw new WebApplicationException("Rating must be an integer between 1 and 5", 400); | ||
| } |
There was a problem hiding this comment.
Throwing a WebApplicationException with a plain string message and status code (e.g., 400) might not include the message in the response body, depending on the JAX-RS provider's default configuration. This results in clients receiving a generic 400 Bad Request without knowing why the request failed.
To ensure the client receives the descriptive error message, wrap the message in a Response object with the appropriate media type.
if (menu.rating == null) {
throw new WebApplicationException(
Response.status(Response.Status.BAD_REQUEST)
.entity("Rating is required and cannot be null")
.type(MediaType.TEXT_PLAIN)
.build());
}
if (menu.rating < 1 || menu.rating > 5) {
throw new WebApplicationException(
Response.status(Response.Status.BAD_REQUEST)
.entity("Rating must be an integer between 1 and 5")
.type(MediaType.TEXT_PLAIN)
.build());
}| if (menu.rating != null) { | ||
| if (menu.rating < 1 || menu.rating > 5) { | ||
| throw new WebApplicationException("Rating must be an integer between 1 and 5", 400); | ||
| } | ||
| entity.rating = menu.rating; | ||
| } |
There was a problem hiding this comment.
Similar to the creation endpoint, throwing a WebApplicationException with a plain string message and status code might not return the message in the response body. Wrap the message in a Response object to ensure the client receives the descriptive error message.
if (menu.rating != null) {
if (menu.rating < 1 || menu.rating > 5) {
throw new WebApplicationException(
Response.status(Response.Status.BAD_REQUEST)
.entity("Rating must be an integer between 1 and 5")
.type(MediaType.TEXT_PLAIN)
.build());
}
entity.rating = menu.rating;
}| @@ -1,3 +1,2 @@ | |||
| quarkus.datasource.db-kind=h2 | |||
| quarkus.datasource.jdbc.url=jdbc:h2:mem:test | |||
| quarkus.datasource.db-kind=postgresql | |||
There was a problem hiding this comment.
Changing the test database to PostgreSQL forces the use of Quarkus Dev Services, which requires a running Docker daemon. This can significantly slow down test execution and cause tests to fail in CI/CD environments where Docker is not available or permitted.
It is recommended to keep using an in-memory database like H2 for testing. You can achieve this by keeping the quarkus-jdbc-h2 dependency in pom.xml with test scope, and configuring the test profile in application.properties (e.g., %test.quarkus.datasource.db-kind=h2).
|
/gemini summary |
Summary of ChangesThis pull request introduces new Highlights
Changelog
Activity
|
1 participant
This PR implements the requested updates to the
menu-serviceapp:descriptionandratingto theMenuJPA entity.ratingonPOST /menu(must be non-null and between 1 and 5) and onPUT /menu/{id}(validates range if supplied), returning HTTP 400 Bad Request on failure.1.0.0to1.0.1inpom.xmlandindex.html.MenuResourceTest.javaasserting successful API paths and the input validation constraint rules. All 6 tests execute and pass successfully.