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

Fix issue-211 - Add method for use by front controller #375

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

markt-asf
Copy link
Contributor

No description provided.

@joakime joakime linked an issue Oct 5, 2021 that may be closed by this pull request
@joakime
Copy link
Contributor

joakime commented Oct 5, 2021

Wow. really?
This seems awkward to me.
And the reasons laid out in issue #211 were never a problem for libraries outside of spring and sockjs. (cometd for example was able to accomplish the end goals laid out in issue #211 without the need for this API)

@joakime
Copy link
Contributor

joakime commented Oct 5, 2021

There needs to be some restrictions in place to protect previously registered ServerEndpointConfig.

Using this API cannot replace a previously registered ServerEndpointConfig path in the ServerContainer (either from annotations, or direct Container.addEndpoint() usage)

@markt-asf
Copy link
Contributor Author

Agreed. This API isn't meant to trigger a registration. I think of it more like the equivalent of a dispatch to a WebSocket endpoint. I can clarify the language around that.

@joakime
Copy link
Contributor

joakime commented Oct 5, 2021

Don't we have text saying ServerContainer.addEndpoint() can only be called during Servlet initialization phase?
If so, we should probably relax that rule as well.

@markt-asf
Copy link
Contributor Author

I've tried to clarify the language on upgrade. I've also removed the "deployment only during initialisation" restriction. I think I have updated the spec everywhere required.

@markt-asf markt-asf merged commit 8c8ef54 into jakartaee:master Oct 7, 2021
@markt-asf markt-asf deleted the issue-221 branch October 7, 2021 16:47
* @throws DeploymentException if a configuration error prevents the establishment of a WebSocket connection
*/
public void upgradeHttpToWebSocket(Object httpServletRequest, Object httpServletResponse, ServerEndpointConfig sec,
Map<String,String> pathParameters) throws IOException, DeploymentException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Map<String, String[]> as in ServletRequest#getParameterMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Different concept. See section 4.3 of the WebSocket spec.

Copy link
Contributor

@joakime joakime Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to the relevant section in the spec - https://github.com/eclipse-ee4j/websocket-api/blob/2.0.0-RELEASE/spec/src/main/asciidoc/WebSocket.adoc#43-pathparam

This is the map of URI Template Path parameters.

So if the websocket endpoint declared ...

@ServerEndpoint("/bookings/{hotel}/{guest-id}")

And the websocket upgrade request arrived at resource path of ...

/bookings/fernando/808

This upgrade's path parameters would be ...

Map Entry Map Value
"hotel" "fernando"
"guest-id" "808"

This has nothing to do with Servlet form / query parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove restriction on when a server endpoint can be deployed
3 participants