Adding new Doc Samples for Java25 #10193
Conversation
product-auto-label
bot
added
samples
Summary of ChangesHello @maigovannon, 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 significantly enhances the Java documentation samples by introducing new examples for Java 25 across Google App Engine Standard, App Engine Flexible, and Cloud Run. It also includes updates to existing 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
|
![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 adds new documentation samples for Java 25 and updates some existing ones. My review focuses on ensuring consistency, correctness, and adherence to best practices across the new and updated samples. I've identified several areas for improvement, including correcting Java versions in configuration files and documentation, fixing broken links, updating security protocols, and resolving dependency issues. Addressing these points will enhance the quality and usability of the new samples.
| String trackingId = System.getenv("GA_TRACKING_ID"); | ||
| URIBuilder builder = new URIBuilder(); | ||
| builder | ||
| .setScheme("http") |
There was a problem hiding this comment.
For security, you should use https to communicate with the Google Analytics endpoint.
| .setScheme("http") | |
| .setScheme("https") |
| <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding> | ||
| <maven.compiler.target>25</maven.compiler.target> | ||
| <maven.compiler.source>25</maven.compiler.source> | ||
| <spring-boot.version>3.5.6</spring-boot.version> |
There was a problem hiding this comment.
The Spring Boot version 3.5.6 does not exist. The latest stable version is 3.2.5. Please use a valid version.
| <spring-boot.version>3.5.6</spring-boot.version> | |
| <spring-boot.version>3.2.5</spring-boot.version> |
...ne-java25/ee11/analytics/src/main/java/com/example/appengine/analytics/AnalyticsServlet.java
Show resolved
Hide resolved
| if (!httpClient.isRunning()) { | ||
| try { | ||
| httpClient.start(); | ||
| } catch (URISyntaxException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
| if (!webSocketClient.isRunning()) { | ||
| try { | ||
| webSocketClient.start(); | ||
| } catch (URISyntaxException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
| ClientUpgradeRequest request = new ClientUpgradeRequest(); | ||
| // Attempt connection | ||
| Future<Session> future = | ||
| webSocketClient.connect(clientSocket, new URI(getWebSocketAddress()), request); | ||
| // Wait for Connect | ||
| Session session = future.get(); | ||
| // Send a message | ||
| session.getRemote().sendString(message); | ||
| // Close session | ||
| session.close(); |
There was a problem hiding this comment.
The try-catch blocks for starting httpClient and webSocketClient are incorrect. The start() method throws Exception, not URISyntaxException. Also, simply printing the stack trace is not good error handling. Since the method signature already throws Exception, you can simplify this by removing the try-catch blocks and letting the exceptions propagate.
private void sendMessageOverWebSocket(String message) throws Exception {
if (!httpClient.isRunning()) {
httpClient.start();
}
if (!webSocketClient.isRunning()) {
webSocketClient.start();
}
ClientUpgradeRequest request = new ClientUpgradeRequest();
// Attempt connection
Future<Session> future =
webSocketClient.connect(clientSocket, new URI(getWebSocketAddress()), request);
// Wait for Connect
Session session = future.get();
// Send a message
session.getRemote().sendString(message);
// Close session
session.close();
}| operating_system: ubuntu22 | ||
| runtime_version: 17 |
There was a problem hiding this comment.
This sample is for Java 25, but the app.yaml specifies runtime_version: 17 and operating_system: ubuntu22. This should be updated to runtime_version: 25 and operating_system: ubuntu24 to match the sample.
operating_system: ubuntu24
runtime_version: 25| holderJsp.setInitParameter("compilerTargetVM", "1.8"); | ||
| holderJsp.setInitParameter("compilerSourceVM", "1.8"); |
There was a problem hiding this comment.
The JSP compiler source and target VM versions are hardcoded to 1.8. These should be updated to match the project's Java version (25).
| holderJsp.setInitParameter("compilerTargetVM", "1.8"); | |
| holderJsp.setInitParameter("compilerSourceVM", "1.8"); | |
| holderJsp.setInitParameter("compilerTargetVM", "25"); | |
| holderJsp.setInitParameter("compilerSourceVM", "25"); |
...ne-java25/ee11/analytics/src/main/java/com/example/appengine/analytics/AnalyticsServlet.java
Outdated
Show resolved
Hide resolved
| public void testOnMessage() { | ||
| assertEquals(ClientSocket.getReceivedMessages().size(), 0); | ||
| socket.onMessage("test"); | ||
| assertEquals(ClientSocket.getReceivedMessages().size(), 1); | ||
| } |
There was a problem hiding this comment.
This test modifies the state of a static collection (ClientSocket.messages), which can lead to flaky tests if they are run in parallel or in a specific order. It's a good practice to clean up the state after the test runs, for example, in an @After method.
...ne-java25/ee11/analytics/src/main/java/com/example/appengine/analytics/AnalyticsServlet.java
Outdated
Show resolved
Hide resolved
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
| <properties> | ||
| <maven.compiler.target>25</maven.compiler.target> | ||
| <maven.compiler.source>25</maven.compiler.source> | ||
| <appengine.sdk.version>2.0.29</appengine.sdk.version> |
Ack. Created separate PRs:
|
Description
Creating Java25 doc samples and updating some of the existing ones to Java21.
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
pom.xmlparent set to latestshared-configurationmvn clean verifyrequiredmvn -P lint checkstyle:checkrequiredmvn -P lint clean compile pmd:cpd-check spotbugs:checkadvisory only