You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by "ffang (via GitHub)" <gi...@apache.org> on 2024/01/10 23:13:23 UTC
[PR] Update to Jetty 12 [cxf]
ffang opened a new pull request, #1633:
URL: https://github.com/apache/cxf/pull/1633
(no comment)
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452786421
##########
rt/transports/websocket/src/main/java/org/apache/cxf/transport/websocket/jetty12/Jetty12WebSocketDestination.java:
##########
@@ -335,5 +342,36 @@ private HttpServletRequest getHttpServletRequest(final UpgradeRequest upgradeReq
}
}
+ @org.eclipse.jetty.websocket.api.annotations.WebSocket
+ public class Jetty12WebSocket {
Review Comment:
Yes, this needs to be public, so that the annotation of this class
@org.eclipse.jetty.websocket.api.annotations.WebSocket
can take effect
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452786123
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -1180,5 +1336,137 @@ public int getSessionTimeout() {
public void setSessionTimeout(int sessionTimeout) {
this.sessionTimeout = sessionTimeout;
}
+
+ class CxfJettyErrorHandler extends ErrorHandler {
Review Comment:
A new commit addressed it
##########
rt/transports/websocket/pom.xml:
##########
@@ -156,16 +145,8 @@
</exclusions>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.websocket</groupId>
- <artifactId>websocket-jakarta-server</artifactId>
- <version>${cxf.jetty11.version}</version>
- <optional>true</optional>
- <exclusions>
- <exclusion>
- <groupId>jakarta.servlet</groupId>
- <artifactId>jakarta.servlet-api</artifactId>
- </exclusion>
- </exclusions>
+ <groupId>org.eclipse.jetty.ee10.websocket</groupId>
Review Comment:
A new commit addressed it
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452786758
##########
systests/microprofile/client/weld/pom.xml:
##########
@@ -178,7 +188,7 @@
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock-jre8-standalone</artifactId>
- <version>2.35.1</version>
+ <version>3.0.1</version>
Review Comment:
a new commit addressed it.
And thanks for the review and input @reta !
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451025312
##########
rt/transports/websocket/src/main/java/org/apache/cxf/transport/websocket/undertow/WebSocketUndertowServletRequest.java:
##########
@@ -570,14 +571,31 @@ public <T extends HttpUpgradeHandler> T upgrade(Class<T> arg0) throws IOExceptio
throw new UnsupportedOperationException();
}
- @Override
+
public String getRealPath(String path) {
return path;
}
- @Override
+
public boolean isRequestedSessionIdFromUrl() {
LOG.log(Level.FINE, "isRequestedSessionIdFromUrl");
return false;
}
+
+ public String getRequestId() {
+ // TODO Auto-generated method stub
Review Comment:
Same, let's get rid of this comments please
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #1633:
URL: https://github.com/apache/cxf/pull/1633#issuecomment-1892972237
Thanks @ffang LGTM!
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452786185
##########
rt/transports/websocket/src/main/java/org/apache/cxf/transport/websocket/jetty/WebSocketVirtualServletRequest.java:
##########
@@ -545,14 +543,31 @@ public <T extends HttpUpgradeHandler> T upgrade(Class<T> arg0) throws IOExceptio
throw new UnsupportedOperationException();
}
- @Override
+
public String getRealPath(String path) {
return path;
}
- @Override
+
public boolean isRequestedSessionIdFromUrl() {
LOG.log(Level.FINE, "isRequestedSessionIdFromUrl");
return false;
}
+
+ public String getRequestId() {
+ // TODO Auto-generated method stub
Review Comment:
A new commit addressed it
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451021486
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthRequestFilter.java:
##########
@@ -202,7 +202,7 @@ protected boolean checkRequestURI(HttpServletRequest request, List<String> uris,
if (uris.isEmpty()) {
return true;
}
- String servletPath = request.getPathInfo();
+ String servletPath = request.getServletPath() + request.getPathInfo();
Review Comment:
@ffang just reread the javadoc `This method returns <code>null</code> if there was no extra path information.` - so we still have an issue here, concatenating with `null` would not produce the expected results.
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1448134460
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthRequestFilter.java:
##########
@@ -202,7 +202,7 @@ protected boolean checkRequestURI(HttpServletRequest request, List<String> uris,
if (uris.isEmpty()) {
return true;
}
- String servletPath = request.getPathInfo();
+ String servletPath = request.getServletPath() + request.getPathInfo();
Review Comment:
Should we be vigilant about presence of `/` ?
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1449210825
##########
rt/rs/microprofile-client/pom.xml:
##########
@@ -182,14 +182,18 @@
</dependency>
<dependency>
<groupId>org.wiremock</groupId>
- <artifactId>wiremock</artifactId>
+ <artifactId>wiremock-standalone</artifactId>
Review Comment:
Yes, actually this is the major reason why I hold this PR that long. This is the last piece that requires Jetty11, until I found other projects which face the similar issue using wiremock-standalone
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452785886
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -456,10 +459,23 @@ public synchronized void addServant(URL url, JettyHTTPHandler handler) {
//then it need be on top of JettyHTTPHandler
//set JettyHTTPHandler as inner handler if
//inner handler is null
- ((SecurityHandler)h).setHandler(handler);
+ //((SecurityHandler)h).setHandler(handler);
Review Comment:
this isn't needed, a new commit removed it
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451025116
##########
rt/transports/websocket/src/main/java/org/apache/cxf/transport/websocket/jetty12/Jetty12WebSocketDestination.java:
##########
@@ -335,5 +342,36 @@ private HttpServletRequest getHttpServletRequest(final UpgradeRequest upgradeReq
}
}
+ @org.eclipse.jetty.websocket.api.annotations.WebSocket
+ public class Jetty12WebSocket {
Review Comment:
Do we need this class to be public?
```suggestion
static class Jetty12WebSocket {
```
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1448133250
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/HttpUtils.java:
##########
@@ -475,6 +475,9 @@ public static String getProtocolHeader(Message m, String name, String defaultVal
public static String getBaseAddress(Message m) {
String endpointAddress = getEndpointAddress(m);
+ if (endpointAddress.startsWith("ws")) {
Review Comment:
Curios why we substitute `ws` with `http`? I believe the call will fail (plus we also could have `wss://`)
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1449216081
##########
rt/rs/microprofile-client/pom.xml:
##########
@@ -182,14 +182,18 @@
</dependency>
<dependency>
<groupId>org.wiremock</groupId>
- <artifactId>wiremock</artifactId>
+ <artifactId>wiremock-standalone</artifactId>
Review Comment:
I will open a ticket (but yeah, `wiremock-standalone` is a way to go)
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451024597
##########
rt/transports/websocket/src/main/java/org/apache/cxf/transport/websocket/jetty/WebSocketVirtualServletRequest.java:
##########
@@ -545,14 +543,31 @@ public <T extends HttpUpgradeHandler> T upgrade(Class<T> arg0) throws IOExceptio
throw new UnsupportedOperationException();
}
- @Override
+
public String getRealPath(String path) {
return path;
}
- @Override
+
public boolean isRequestedSessionIdFromUrl() {
LOG.log(Level.FINE, "isRequestedSessionIdFromUrl");
return false;
}
+
+ public String getRequestId() {
+ // TODO Auto-generated method stub
Review Comment:
Let's get rid of this comments?
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1449209095
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/HttpUtils.java:
##########
@@ -475,6 +475,9 @@ public static String getProtocolHeader(Message m, String name, String defaultVal
public static String getBaseAddress(Message m) {
String endpointAddress = getEndpointAddress(m);
+ if (endpointAddress.startsWith("ws")) {
Review Comment:
This change is required to fix some websocket related tests failures with Jetty 12.
We need this change because in Jetty 12 architect and API changes.
For example, if the request url is "ws://localhost:31604/websocket" or "http://localhost:31604/websocket", we want getBaseAddress() returns "/websocket".
I pushed another commit here to honor "wss" protocol as well
https://github.com/apache/cxf/pull/1633/commits/20a0f1c5230c4907b2b03db1a7a12d7a0024baaf
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on PR #1633:
URL: https://github.com/apache/cxf/pull/1633#issuecomment-1887691482
@reta , no rush, take your time. I will keep this PR open for a while
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452786038
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -937,25 +1035,81 @@ private ThreadPool getThreadPool() {
*/
public synchronized void removeServant(URL url) {
- final String contextName = HttpUriMapper.getContextName(url.getPath());
- final String smap = HttpUriMapper.getResourceBase(url.getPath());
+ String contextName = HttpUriMapper.getContextName(url.getPath());
+ if (contextName.length() == 0) {
+ contextName = "/"; //ensure it is mapped as root context,
+ }
+ String smap = HttpUriMapper.getResourceBase(url.getPath());
+ if (!smap.equals("/")) {
+ //add /* to enable wild match
+ smap = smap + "/*";
+ }
+
+ if (contextName.endsWith("/*")) {
+ //This is how Jetty ServletContextHandler handle the contextName
+ //without suffix "/", the "*" suffix will be removed
+ contextName = contextName + "/";
+ }
boolean found = false;
if (server != null && server.isRunning()) {
- for (Handler handler : contexts.getChildHandlersByClass(ContextHandler.class)) {
- if (handler instanceof ContextHandler) {
- ContextHandler contextHandler = (ContextHandler) handler;
- Handler jh = contextHandler.getHandler();
- if (jh instanceof JettyHTTPHandler
+ for (Handler handler : contexts.getHandlers()) {
+ if (handler instanceof ServletContextHandler) {
+ ServletContextHandler contextHandler = (ServletContextHandler) handler;
+ ServletHandler servletHandler = contextHandler.getServletHandler();
+
+ MatchedResource<MappedServlet> mappedServlet = servletHandler.getMatchedServlet(smap);
+ if (mappedServlet == null) {
+ continue;
+ }
+ ServletHolder servletHolder = mappedServlet.getResource().getServletHolder();
+ Servlet servlet = null;
+
+ try {
+ servlet = servletHolder.getServlet();
+ } catch (ServletException ex) {
+ LOG.log(Level.WARNING, "REMOVE_HANDLER_FAILED_MSG", new Object[] {
+ ex.getMessage()
+ });
+ continue;
+ }
+ if (servlet != null && servlet instanceof JettyHTTPHandler
&& (contextName.equals(contextHandler.getContextPath())
|| (StringUtils.isEmpty(contextName)
&& "/".equals(contextHandler.getContextPath())))
- && ((JettyHTTPHandler)jh).getName().equals(smap)) {
+ && smap.startsWith(((JettyHTTPHandler)servlet).getName())) {
try {
- contexts.removeHandler(handler);
- handler.stop();
- handler.destroy();
+ servletHolder.stop();
+ contextHandler.getContext().destroy(servlet);
+ //need to remove path from ServletHandler._servletMappings
+ //and has to access the private field.
+
+ Field privateField
Review Comment:
We have to use ReflectionUtil here. A new commit addressed it
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452785762
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthRequestFilter.java:
##########
@@ -202,7 +202,7 @@ protected boolean checkRequestURI(HttpServletRequest request, List<String> uris,
if (uris.isEmpty()) {
return true;
}
- String servletPath = request.getPathInfo();
+ String servletPath = request.getServletPath() + request.getPathInfo();
Review Comment:
a Null checker added to address this
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang merged PR #1633:
URL: https://github.com/apache/cxf/pull/1633
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451022086
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -456,10 +459,23 @@ public synchronized void addServant(URL url, JettyHTTPHandler handler) {
//then it need be on top of JettyHTTPHandler
//set JettyHTTPHandler as inner handler if
//inner handler is null
- ((SecurityHandler)h).setHandler(handler);
+ //((SecurityHandler)h).setHandler(handler);
Review Comment:
It is commented out accidentally or not needed?
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451024215
##########
rt/transports/websocket/pom.xml:
##########
@@ -156,16 +145,8 @@
</exclusions>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty.websocket</groupId>
- <artifactId>websocket-jakarta-server</artifactId>
- <version>${cxf.jetty11.version}</version>
- <optional>true</optional>
- <exclusions>
- <exclusion>
- <groupId>jakarta.servlet</groupId>
- <artifactId>jakarta.servlet-api</artifactId>
- </exclusion>
- </exclusions>
+ <groupId>org.eclipse.jetty.ee10.websocket</groupId>
Review Comment:
```
<optional>true</optional>
```
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1452786522
##########
rt/transports/websocket/src/main/java/org/apache/cxf/transport/websocket/undertow/WebSocketUndertowServletRequest.java:
##########
@@ -570,14 +571,31 @@ public <T extends HttpUpgradeHandler> T upgrade(Class<T> arg0) throws IOExceptio
throw new UnsupportedOperationException();
}
- @Override
+
public String getRealPath(String path) {
return path;
}
- @Override
+
public boolean isRequestedSessionIdFromUrl() {
LOG.log(Level.FINE, "isRequestedSessionIdFromUrl");
return false;
}
+
+ public String getRequestId() {
+ // TODO Auto-generated method stub
Review Comment:
a new commit addressed it
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1448133667
##########
rt/rs/microprofile-client/pom.xml:
##########
@@ -182,14 +182,18 @@
</dependency>
<dependency>
<groupId>org.wiremock</groupId>
- <artifactId>wiremock</artifactId>
+ <artifactId>wiremock-standalone</artifactId>
Review Comment:
Ah yeah, wiremock still uses Jetty 11
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1449213099
##########
rt/rs/security/oauth-parent/oauth2/src/main/java/org/apache/cxf/rs/security/oauth2/filters/OAuthRequestFilter.java:
##########
@@ -202,7 +202,7 @@ protected boolean checkRequestURI(HttpServletRequest request, List<String> uris,
if (uris.isEmpty()) {
return true;
}
- String servletPath = request.getPathInfo();
+ String servletPath = request.getServletPath() + request.getPathInfo();
Review Comment:
I don't think we need to worry about it.
From the java doc of servlet-api 6
```
/**
* Returns any extra path information associated with the URL the client sent when it made this request. The extra path
* information follows the servlet path but precedes the query string and will start with a "/" character.
*
* <p>
* This method returns <code>null</code> if there was no extra path information.
*
* @return a <code>String</code> specifying extra path information that comes after the servlet path but before the
* query string in the request URL; or <code>null</code> if the URL does not have any extra path information. The path
* will be canonicalized as per section 3.5 of the specification. This method will not return any encoded characters
* unless the container is configured specifically to allow them.
* @throws IllegalArgumentException In standard configuration, this method will never throw. However, a container may be
* configured to not reject some suspicious sequences identified by 3.5.2, furthermore the container may be configured
* to allow such paths to only be accessed via safer methods like {@link #getRequestURI()} and to throw
* IllegalArgumentException if this method is called for such suspicious paths.
*/
public String getPathInfo();
```
So a "/" character is guaranteed here.
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #1633:
URL: https://github.com/apache/cxf/pull/1633#issuecomment-1887686849
@ffang just fyi, I had time to go over the half of the change (sorry about that), will try to finish it till the end of the week but please don't wait for me if you need it asap.
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on PR #1633:
URL: https://github.com/apache/cxf/pull/1633#issuecomment-1904483798
Thanks for the review @reta !
Merged
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451027447
##########
systests/microprofile/client/weld/pom.xml:
##########
@@ -178,7 +188,7 @@
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock-jre8-standalone</artifactId>
- <version>2.35.1</version>
+ <version>3.0.1</version>
Review Comment:
The 2.35.x line was used for Jetty 9 support, if we don't need that, let's use the latest one (please notice group change, `org.wiremock` now):
```
<dependency>
<groupId>org.wiremock</groupId>
<artifactId>wiremock-standalone</artifactId>
<version>3.3.1</version>
<scope>test</scope>
</dependency>
```
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451023510
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -1180,5 +1336,137 @@ public int getSessionTimeout() {
public void setSessionTimeout(int sessionTimeout) {
this.sessionTimeout = sessionTimeout;
}
+
+ class CxfJettyErrorHandler extends ErrorHandler {
Review Comment:
```suggestion
static class CxfJettyErrorHandler extends ErrorHandler {
```
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update to Jetty 12 [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1633:
URL: https://github.com/apache/cxf/pull/1633#discussion_r1451023310
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -937,25 +1035,81 @@ private ThreadPool getThreadPool() {
*/
public synchronized void removeServant(URL url) {
- final String contextName = HttpUriMapper.getContextName(url.getPath());
- final String smap = HttpUriMapper.getResourceBase(url.getPath());
+ String contextName = HttpUriMapper.getContextName(url.getPath());
+ if (contextName.length() == 0) {
+ contextName = "/"; //ensure it is mapped as root context,
+ }
+ String smap = HttpUriMapper.getResourceBase(url.getPath());
+ if (!smap.equals("/")) {
+ //add /* to enable wild match
+ smap = smap + "/*";
+ }
+
+ if (contextName.endsWith("/*")) {
+ //This is how Jetty ServletContextHandler handle the contextName
+ //without suffix "/", the "*" suffix will be removed
+ contextName = contextName + "/";
+ }
boolean found = false;
if (server != null && server.isRunning()) {
- for (Handler handler : contexts.getChildHandlersByClass(ContextHandler.class)) {
- if (handler instanceof ContextHandler) {
- ContextHandler contextHandler = (ContextHandler) handler;
- Handler jh = contextHandler.getHandler();
- if (jh instanceof JettyHTTPHandler
+ for (Handler handler : contexts.getHandlers()) {
+ if (handler instanceof ServletContextHandler) {
+ ServletContextHandler contextHandler = (ServletContextHandler) handler;
+ ServletHandler servletHandler = contextHandler.getServletHandler();
+
+ MatchedResource<MappedServlet> mappedServlet = servletHandler.getMatchedServlet(smap);
+ if (mappedServlet == null) {
+ continue;
+ }
+ ServletHolder servletHolder = mappedServlet.getResource().getServletHolder();
+ Servlet servlet = null;
+
+ try {
+ servlet = servletHolder.getServlet();
+ } catch (ServletException ex) {
+ LOG.log(Level.WARNING, "REMOVE_HANDLER_FAILED_MSG", new Object[] {
+ ex.getMessage()
+ });
+ continue;
+ }
+ if (servlet != null && servlet instanceof JettyHTTPHandler
&& (contextName.equals(contextHandler.getContextPath())
|| (StringUtils.isEmpty(contextName)
&& "/".equals(contextHandler.getContextPath())))
- && ((JettyHTTPHandler)jh).getName().equals(smap)) {
+ && smap.startsWith(((JettyHTTPHandler)servlet).getName())) {
try {
- contexts.removeHandler(handler);
- handler.stop();
- handler.destroy();
+ servletHolder.stop();
+ contextHandler.getContext().destroy(servlet);
+ //need to remove path from ServletHandler._servletMappings
+ //and has to access the private field.
+
+ Field privateField
Review Comment:
@ffang could we avoid using reflection? If we really really need that - please use `org.apache.cxf.common.util.ReflectionUtil` (but I would love to see the solution without reflection)
--
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@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org