Issue #325 - fixes and testing for HttpServletRequest getServerName

[lachlan-roberts]

Fixes for #325

• Remove the getServerName override in the HttpServletRequest to allow it to get the serverName from the request metadata.
• improve testing in RemoteAddressTest with additional test cases with Host header, and test all relevant methods on HttpServletRequest.

[code-review-assist]

Hi @lachlan-roberts and reviewers,

I'm currently reviewing this pull request and will post my full review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request addresses issue #325, focusing on improvements to the HttpServletRequest.getServerName() method and its associated testing.

Intent: The primary goal is to fix inconsistencies and enhance the reliability of obtaining the server name within the application environment. This involved removing a custom override and relying on request metadata for a more robust solution. Additionally, the testing suite has been expanded to cover various scenarios, including the presence of a Host header, to ensure comprehensive test coverage.

Changes:

• runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java: Removed the overridden getServerName method from the custom HttpServletRequestWrapper (lines 338-341). Added an import for org.eclipse.jetty.util.HostPort (lines 68-76) and used HostPort.normalizeHost to normalize the user IP address (line 290).
• runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/RemoteAddressTest.java: Significantly expanded test cases to include scenarios with and without the Host header (lines 81-192). Added imports for org.eclipse.jetty.http.HttpHeader and org.eclipse.jetty.http.HttpVersion (lines 24-32). Added a url variable to simplify test setup (lines 56-60 and 70-76).
• runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/remoteaddrapp/EE10RemoteAddrServlet.java and runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/remoteaddrapp/EE8RemoteAddrServlet.java: Added req.getRemoteHost(), req.getLocalName(), and ensured req.getServerPort() is printed to provide more comprehensive output in the test applications (lines 28-37).

The changes ensure that getServerName correctly retrieves the server name from the request metadata and that the tests are more comprehensive.

Here's a little something to lighten the mood:

Code flows like a river,
Tests catch bugs, a swift current,
Clean code, pure delight.

Let me know if you have any questions. Looking forward to a thorough review!

Gemini

[code-review-assist]

Gemini encountered an error creating the review. You can try again by commenting @code-review-assist review.

[ludoch]

@code-review-assist review

[code-review-assist]

This pull request correctly implements the requested changes for issue #325. The removal of the getServerName override allows the server name to be obtained from the request metadata as expected. The test improvements in RemoteAddressTest provide comprehensive coverage for various scenarios, including those with and without the Host header, as well as IPv6 addresses. The changes are well-structured and easy to understand. The commit message clearly describes the changes made. Here's a summary of the style guide elements I referenced, per the Google Java Style Guide:

• Import Order: Static imports should be grouped together and all imports should be grouped by top-level domain, then by package.
• Ternary Operator: Avoid using the ternary operator for complex conditional assignments. Prefer regular if-else statements for better readability.
• Variable Naming: Avoid reusing variable names within the same scope, especially when one is a class member and the other is a local variable.
• Newlines: Add a newline at the end of each file.

[maigovannon]

Is b3de558#diff-28a36762cb84139431767aee493287f06752c86ae601c79580d77dece0d4d821L350 safe as well? I just realised that the method name is LocalHostName and that is the description of the parent method but we are returning the Local IP instead.

[lachlan-roberts]

Not sure exactly what you are referring to here.

But we are not doing any reverse DNS lookups so we never resolve IPs to a hostname.

[maigovannon]

Sorry I should have clarified the link better:

appengine-java-standard/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java

Line 354 in 3ad247f

public String getLocalName() {

If you see we are returning 0.0.0.0 when actually this is supposed to return the host name rather than ip as per https://docs.oracle.com/javaee/6/api/javax/servlet/ServletRequest.html#getLocalName()

[maigovannon]

[nit] Just went and checked the source of this change and came across this CL: https://critique.corp.google.com/cl/400610752. Curious if this flag is ever used by the customers? There is a typo in the naming of the property in that it is is called nomalize_inet_addr instead of normalize_inet_addr

[lachlan-roberts]

I'm not sure which customers could be using this. But this should maintain the behavior of this flag for the HttpConnector mode on the jetty9.4 runtime.

We don't currently have this flag available for the Jetty 12 runtimes. It behaves as if com.google.appengine.nomalize_inet_addr == false because the correct behavior for getRemoteAddr and getRemoteHost is to not include the [] brackets.

But would maybe @gregw or @ludoch have more context on this issue and if customers are require com.google.appengine.nomalize_inet_addr == true behavior.

[code-review-assist]

Hi @lachlan-roberts and reviewers,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request addresses issue #325, focusing on improvements to the HttpServletRequest's getServerName method and enhancing related testing.

Intent:
The primary goal is to fix inconsistencies and improve the reliability of obtaining the server name within the HttpServletRequest object. The changes remove redundant overriding of getServerName and improve test coverage to ensure correctness across various scenarios, including those involving the Host header.

Changes:

• Removed redundant getServerName override: The getServerName method in JettyRequestAPIData (both Jetty9 and Jetty12 versions) was overridden unnecessarily. This PR removes the override, allowing the standard behavior to retrieve the server name from request metadata.
• Improved RemoteAddressTest: The RemoteAddressTest class has been significantly enhanced with additional test cases. These tests cover scenarios with the Host header and verify the behavior of all relevant methods in HttpServletRequest (including getRemoteAddr, getRemoteHost, getRemotePort, getLocalAddr, getLocalName, getLocalPort, getServerName, and getServerPort). Specific additions include tests for IPv6 addresses and scenarios without a Host header.
• Minor code style improvements: Some minor formatting adjustments were made to improve code readability in JettyRequestAPIData.

Files Changed:

• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/http/JettyRequestAPIData.java (Minor comment formatting changes, lines 124-131)
• runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyRequestAPIData.java (Import added, lines 51-57; Import added, lines 67-78; Minor comment formatting changes, lines 128-136; NORMALIZE_INET_ADDR used for IP normalization, lines 286-294; getServerName override removed, lines 335-341)
• runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/RemoteAddressTest.java (Imports added, lines 24-32; Added url variable, lines 54-62; url initialized, lines 67-76; New test cases added, lines 77-201)
• runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/remoteaddrapp/EE10RemoteAddrServlet.java (Added getRemoteHost, getLocalName, lines 28-37)
• runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/remoteaddrapp/EE8RemoteAddrServlet.java (Added getRemoteHost, getLocalName, lines 28-37)

Let me know if you have any questions.

And now, a little haiku to brighten your day:

Code flows like a stream,
Tests catch bugs before they fly,
Smooth release is near.

[code-review-assist]

Gemini encountered an error creating the review. You can try again by commenting @code-review-assist review.