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

Review exposure of JavaxWebSocketServletContainerInitializer #3872

Closed
sbordet opened this issue Jul 13, 2019 · 1 comment
Closed

Review exposure of JavaxWebSocketServletContainerInitializer #3872

sbordet opened this issue Jul 13, 2019 · 1 comment
Assignees

Comments

@sbordet
Copy link
Contributor

sbordet commented Jul 13, 2019

Following #3661, the Jetty WebSocket classes have been moved to proper packages so that they can be hidden from web apps and properly exposed via JPMS to embedded users.

The same should be done for Javax WebSocket classes.

@gregw
Copy link
Contributor

gregw commented Jul 13, 2019

@lachlan-roberts So I'm thinking that there should be no org.eclipse.jetty.websocket.javax.server, everything should either be an implementation of the standard API and thus in org.eclipse.jetty.websocket.javax.server.internal or something used to init/configure the implementation and thus in org.eclipse.jetty.websocket.javax.server.config. There should be no Jetty API classes exposed to the webapp for javax ?

gregw added a commit that referenced this issue Jul 14, 2019
Moved JaxaxWebSocketConfiguration and SCI to config package.
Limited classpath exposure in JavaxConfiguration (more needed)
Updated tests with work around for those that needs more classes exposed

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Jul 14, 2019
Moved all remaining classes from org.eclipse.jetty.websocket.javax.server
to org.eclipse.jetty.websocket.javax.server.internal.

This works when running on the classpath, but the tests fail when running
on the modulepath (eg in commandline mvn run).   The issue appears to be
that the tests don't load test classes from WEB-INF/lib or WEB-INF/classes.
Instead the test classes were themselves in  org.eclipse.jetty.websocket.javax.server,
which is no longer exported from module-info.java.

The hacked "fix" for this has been to create a org.eclipse.jetty.websocket.javax.server.tests
package which is exported and to move all the tests to that.  A better fix is needed.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Jul 14, 2019
improve comments
tighten exposed classes more

Signed-off-by: Greg Wilkins <gregw@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 15, 2019
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Jul 15, 2019
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
gregw added a commit that referenced this issue Aug 28, 2019
* Issue #3872 Javax Websocket Packaging

Moved JaxaxWebSocketConfiguration and SCI to config package.
Limited classpath exposure in JavaxConfiguration (more needed)
Updated tests with work around for those that needs more classes exposed

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #3872 Javax Websocket Packaging

Moved all remaining classes from org.eclipse.jetty.websocket.javax.server
to org.eclipse.jetty.websocket.javax.server.internal.

This works when running on the classpath, but the tests fail when running
on the modulepath (eg in commandline mvn run).   The issue appears to be
that the tests don't load test classes from WEB-INF/lib or WEB-INF/classes.
Instead the test classes were themselves in  org.eclipse.jetty.websocket.javax.server,
which is no longer exported from module-info.java.

The hacked "fix" for this has been to create a org.eclipse.jetty.websocket.javax.server.tests
package which is exported and to move all the tests to that.  A better fix is needed.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #3872 Javax Websocket Packaging

improve comments
tighten exposed classes more

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #3872 - fixing tests

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

* Issue #3872 - move ContainerDefaultConfigurator to config package

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

* Issue #3873 - fix javax websocket test classloader issues

move websocket endpoints for test webapps to com.acme.websocket package

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants