You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tapestry.apache.org by "benweidig (via GitHub)" <gi...@apache.org> on 2026/06/28 09:04:41 UTC
[PR] TAP5-2832: LocalhostOnly detect IPv6 correctly [tapestry-5]
benweidig opened a new pull request, #64:
URL: https://github.com/apache/tapestry-5/pull/64
`LocalhostOnly` didn't detect a few of the IPv6 variants.
Furthermore, it might trigger a DNS lookup, which could impact performance.
Therefore, it now relies on `HttpServletRequest.getRemoteAddr` (via Request).
I've decided to extend the `Request` interfrace with `getRemoteAddr` instead of trying to provide `LocalhostOnly` with the `HttpServletRequest` somehow.
My reasoning is that `Request` is already a more high-level/convenient wrapper for `HttpServletRequest`, delegating multiple methods already to the underlying `HttpServletRequest`.
Adding another one makes sense, as anything relying on `LocalhostOnly would need to provide the full/correct environment.
This was an issue during initial testing with using `RequestGlobals` in `LocalhostOnly`, as it became an invisible dependency.
By extending `Request` and the related `TestableRequest` and `DelegatingRequest`, the `Request`-based code will behave correctly, regardless of how it's used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org
Re: [PR] TAP5-2832: LocalhostOnly detect IPv6 correctly [tapestry-5]
Posted by "thiagohp (via GitHub)" <gi...@apache.org>.
thiagohp commented on code in PR #64:
URL: https://github.com/apache/tapestry-5/pull/64#discussion_r3609022013
##########
tapestry-http/src/main/java/org/apache/tapestry5/http/services/Request.java:
##########
@@ -209,6 +209,15 @@ public interface Request
*/
String getRemoteHost();
+ /**
+ * Returns the IP address of the client or last proxy that sent the request,
+ * always as a numeric address string (never a hostname).
+ *
+ * @return a <code>String</code> containing the IP address of the client that sent the request
+ * @since 5.10
+ */
+ String getRemoteAddr();
Review Comment:
Yes, I'd like to have the new method to be default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org
Re: [PR] TAP5-2832: LocalhostOnly detect IPv6 correctly [tapestry-5]
Posted by "benweidig (via GitHub)" <gi...@apache.org>.
benweidig commented on code in PR #64:
URL: https://github.com/apache/tapestry-5/pull/64#discussion_r3608580359
##########
tapestry-http/src/main/java/org/apache/tapestry5/http/services/Request.java:
##########
@@ -209,6 +209,15 @@ public interface Request
*/
String getRemoteHost();
+ /**
+ * Returns the IP address of the client or last proxy that sent the request,
+ * always as a numeric address string (never a hostname).
+ *
+ * @return a <code>String</code> containing the IP address of the client that sent the request
+ * @since 5.10
+ */
+ String getRemoteAddr();
Review Comment:
In general, I totally agree and get where you're coming from, but I would argue that `Request` is a "consume-only" interface with little reason to implement yourself.
For example, if `MarkupRendererFilter` would be changed, a lot of code would break.
But with `Request`, I don't see much risk.
If someone implements it, they are doing some special stuff, most likely in a library, and then needing to adapt to such a change seems reasonable to me, or not, as the change is binary-compatible.
However, if you want me to include a `default` method anyway, I would stay with `String`/`null`, as it's not an optional value according to spec, even though it's not guaranteed by all environments.
Lately, I try to avoid `Optional`, as it does not provide much utility if it only represents a singular value that is directly consumed after and not chained with multiple calls, and simple `null`-check is cheaper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org
Re: [PR] TAP5-2832: LocalhostOnly detect IPv6 correctly [tapestry-5]
Posted by "thiagohp (via GitHub)" <gi...@apache.org>.
thiagohp commented on code in PR #64:
URL: https://github.com/apache/tapestry-5/pull/64#discussion_r3608524594
##########
tapestry-http/src/main/java/org/apache/tapestry5/http/services/Request.java:
##########
@@ -209,6 +209,15 @@ public interface Request
*/
String getRemoteHost();
+ /**
+ * Returns the IP address of the client or last proxy that sent the request,
+ * always as a numeric address string (never a hostname).
+ *
+ * @return a <code>String</code> containing the IP address of the client that sent the request
+ * @since 5.10
+ */
+ String getRemoteAddr();
Review Comment:
For backward compatibility purposes, couldn't we make this a default method that returns null? Or maybe return type Optional<String> and return Optional.empty() by default? While we cannot change the existing Tapestry methods that may return null to return Optional instead, I believe we should try to use them for new methods we introduce in Java interfaces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org
Re: [PR] TAP5-2832: LocalhostOnly detect IPv6 correctly [tapestry-5]
Posted by "benweidig (via GitHub)" <gi...@apache.org>.
benweidig merged PR #64:
URL: https://github.com/apache/tapestry-5/pull/64
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org