Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocket: Annotated Endpoints always have contextRoot /websocket #1565

Closed
dansiviter opened this issue Mar 23, 2020 · 7 comments
Closed

WebSocket: Annotated Endpoints always have contextRoot /websocket #1565

dansiviter opened this issue Mar 23, 2020 · 7 comments
Assignees
Labels

Comments

@dansiviter
Copy link
Contributor

Environment Details

  • Helidon Version: MP 2.0.0-M2
  • JDK version: 11
  • OS: Windows
  • Docker version (if applicable): N/A

Problem Description

Using Microprofile WebSocket integration only apps initialised with ServerApplicationConfig can have their contextRoot set. If using pure annotated endpoints, then this always defaults to /websocket. I feel there are two issues here:

  • Default path should not be /websocket as we may want to interleave the routing with Jersey or other routes,
  • Annotated endpoint should be inspected for @RoutingPath and that be honoured, but if it doesn't exist, use purely the value in @ServerEndpoint#value.

Issue is here: WebSocketCdiExtension

Steps to reproduce

Create any annotated WebSocket endpoint. You'll see the message:

INFO: Registering websocket application at /websocket

@spericas spericas self-assigned this Mar 23, 2020
@spericas spericas added enhancement New feature or request works as designed labels Mar 23, 2020
@spericas
Copy link
Member

This is currently by design and somewhat similar to how Grpc is supported. What you suggest is different (and has other issues), but I'm not sure is better, and is certainly less consistent with the SE support.

@dansiviter
Copy link
Contributor Author

TBH my expectation is that the @ServerEndpoint#value is the only specifier of path, similar to how @ApplicationPath#value is. This would be the same regardless of server runtime. However, I will concede that the specification does state that:

If the WebSocket server includes the Jakarta Servlet API, the WebSocket root must be the same as the Servlet context root of the web application [WSC-6.4-1].
https://github.com/eclipse-ee4j/websocket-api/blob/master/spec/src/main/asciidoc/WebSocket.adoc#65-websocket-server-paths

...and as Servlet is not implemented in Helidon, it doesn't have to match. Therefore, can we at least be able to customise it? Otherwise, I'll have to put in horrible URL mappings to make the paths look consistent.

@dansiviter
Copy link
Contributor Author

dansiviter commented Mar 25, 2020

So for an MP example:

  • JAX-RS @ApplicationPath("my-app") & @Path("v1/foo"):
    <scheme>://<host>:<port>/my-app/v1/foo
  • WS @ServerEndpoint("/my-app/v1/ws"):
    <scheme>://<host>:<port>/websocket/my-app/v1/foo

I therefore cannot set a consistent context root, nor configure away from /websocket. What I would want is:

  • <scheme>://<host>:<port>/my-app/v1/foo
  • <scheme>://<host>:<port>/my-app/v1/ws

Therefore in something like Istio I only need to set mapping for:

http:
- match:
    uri:
      prefix: "/my-app/"
  route:
  - destination:
      host: my-app

Instead, it would be:

http:
- match:
    uri:
      prefix: "/my-app/"
  route:
  - destination:
      host: my-app
- match:
    uri:
      prefix: "/my-app/"
    scheme:
      exact: wss
    rewrite:
      uri: /websocket/my-app/
  route:
  - destination:
      host: my-app

@m0mus m0mus added the P3 label Mar 26, 2020
@spericas
Copy link
Member

spericas commented Apr 3, 2020

So for an MP example:

  • JAX-RS @ApplicationPath("my-app") & @Path("v1/foo"):
    <scheme>://<host>:<port>/my-app/v1/foo
  • WS @ServerEndpoint("/my-app/v1/ws"):
    <scheme>://<host>:<port>/websocket/my-app/v1/foo

@ServerEndpoint is analogous to @Path not @ApplicationPath, that is, it applies to an individual endpoint. The default value for an application path in JAX-RS is /, in Web sockets is /websocket to avoid conflicts.

To override the default for Web sockets you need to create a subclass of ServerApplicationConfig and use @RoutingPath on it; just like you need to define a subclass of Application and use @ApplicationPath in JAX-RS.

@dansiviter
Copy link
Contributor Author

@spericas I just tried the ServerApplicationConfig approach and although I don't think /websocket should be the default, similar to other implementations of Jakarta WebSocket it should share the same context root at JAX-RS, Servlet, etc.,, it appears I can force it to share the same namespace which means I can simplify my Istio config.

I'll do some more testing first before I close this issue.

QQ: were there specific conflict issues with gRPC or WebSocket handlers that were observed by sharing the same content root? From my quick test it seems I can interleave JAX-RS and WebSocket endpoints.

@spericas
Copy link
Member

spericas commented Apr 3, 2020

@dansiviter It seemed like the separation of context roots would lead to fewer user surprises, but it sounds like it had the opposite effect in your app. Perhaps we should reconsider this decision before 2.0 goes final ...

@spericas
Copy link
Member

spericas commented Apr 9, 2020

Default context root has been set to "/" for WebSocket endpoints as well (PR #1622, PR #1631). This change should be available in the next milestone release (M3).

@spericas spericas closed this as completed Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

3 participants