chore: upgrade datastore sample from java8 to java21 #10161
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
product-auto-label
bot
added
the
samples
Alan5142
force-pushed
the
alanra-chore-migrate-java8-to-java21-datastore
branch
from
18862c5 to
fcc2e93
Compare
Alan5142
force-pushed
the
alanra-chore-migrate-java8-to-java21-datastore
branch
from
fcc2e93 to
b921e34
Compare
|
/gemini review |
![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 upgrades the datastore sample to Java 21 by adding a new set of files. The overall structure and code quality are good, demonstrating various Datastore features effectively. However, I've found a few issues in the pom.xml concerning dependency management that could lead to runtime errors, and a minor issue in one of the test classes regarding test isolation. Please see my detailed comments.
| <dependency> | ||
| <groupId>taglibs</groupId> | ||
| <artifactId>standard</artifactId> | ||
| <version>1.1.2</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>jakarta.servlet.jsp.jstl</groupId> | ||
| <artifactId>jakarta.servlet.jsp.jstl-api</artifactId> | ||
| <version>1.2.7</version> | ||
| </dependency> |
There was a problem hiding this comment.
The taglibs:standard dependency is not suitable for a Jakarta EE application as it relies on the javax.* namespace. This will cause conflicts with jakarta.servlet-api and can lead to runtime errors such as NoClassDefFoundError. You should use a JSTL implementation that is compatible with Jakarta EE.
The jakarta.servlet.jsp.jstl-api dependency is correct, but taglibs:standard should be replaced with its Jakarta-compatible equivalent.
| <dependency> | |
| <groupId>taglibs</groupId> | |
| <artifactId>standard</artifactId> | |
| <version>1.1.2</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>jakarta.servlet.jsp.jstl</groupId> | |
| <artifactId>jakarta.servlet.jsp.jstl-api</artifactId> | |
| <version>1.2.7</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>jakarta.servlet.jsp.jstl</groupId> | |
| <artifactId>jakarta.servlet.jsp.jstl-api</artifactId> | |
| <version>1.2.7</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.glassfish.web</groupId> | |
| <artifactId>jakarta.servlet.jsp.jstl</artifactId> | |
| <version>1.2.6</version> | |
| </dependency> |
There was a problem hiding this comment.
@alarconesparza can you take a look at this and provide the recommended path here? I'm not sure that we need to chagne this over to include org.glassfish.web or not, and also not sure if taglib would stay the same in the actual application or not.
| static int newJobCounter = 0; | ||
| static int existingJobCounter = 0; |
There was a problem hiding this comment.
The fields newJobCounter and existingJobCounter are static. This creates mutable shared state between tests, which can lead to flaky and order-dependent tests, especially if tests are ever run in parallel. These should be instance fields to ensure test isolation.
| static int newJobCounter = 0; | |
| static int existingJobCounter = 0; | |
| int newJobCounter = 0; | |
| int existingJobCounter = 0; |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
Checklist
pom.xmlparent set to latestshared-configurationmvn clean verifyrequiredmvn -P lint checkstyle:checkrequiredmvn -P lint clean compile pmd:cpd-check spotbugs:checkadvisory only