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

Issue #3661 - review of exposed classes in jetty-websocket-server #3779

Merged
merged 2 commits into from Jun 17, 2019

Conversation

lachlan-roberts
Copy link
Contributor

Issue #3661

removing any exposed classes from websocket-common and websocket-servlet in the jetty-websocket-server package

added javadoc for JettyWebSocketServletFactory

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@gregw
Copy link
Contributor

gregw commented Jun 13, 2019

@lachlan-roberts I bit more description would be useful on this one, as I'd like to get feedback from @janbartel @sbordet @joakime on what we did here.

The org.eclipse.jetty.websocket.server contains 3 types of classes:

  • ones that implement the jetty API and are exposed to the webapp (eg JettyWebSocketServlet)
  • those that are internal implementation classes (eg UpgradeResponseAdapter) put in the internal sub package, which is hidden from the webapp.
  • Those that are needed by a user to configure, but not exposed to the webapp (eg JettyWebSocketConfiguration and JettyWebSocketServletContainerInitializer), which this PR moves to a config sub package, which is hidden from the webapp.

So firstly @lachlan-roberts can you verify that you have got this right with no common or core classes in the webapp API by looking at the OSGi headers in the generated manifest for the jar. The exports clause for org.eclipse.jetty.websocket.server should have no uses clauses on it for config or internal. @janbartel, I'm guessing it will still export internal and config, so how do we stop that? @sbordet ditto for JPMS? OR should it export config ??

Should the SCI and Configuration just be in internal? They are both mainly discovered with SystemLoader?

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

@gregw
This is what the MANIFEST file looks like at the moment:

Export-Package: 
org.eclipse.jetty.websocket.server.config;
uses:="
javax.servlet,
org.eclipse.jetty.servlet,
org.eclipse.jetty.webapp,
org.eclipse.jetty.websocket.server";
version="10.0.0",

org.eclipse.jetty.websocket.server;
uses:="
javax.servlet,
javax.servlet.http,
org.eclipse.jetty.util.component,
org.eclipse.jetty.websocket.api,
org.eclipse.jetty.websocket.api.extensions,
org.eclipse.jetty.websocket.common,
org.eclipse.jetty.websocket.servlet";
version="10.0.0"

there is a few packages that we still don't want, I think these are here because we are exposing classes which extend classes from these packages.

org.eclipse.jetty.util.component,
org.eclipse.jetty.websocket.common
org.eclipse.jetty.websocket.servlet

these classes are JettyWebSocketServerContainer which extends ContainerLifeCycle and implements org.eclipse.jetty.websocket.common.WebSocketContainer
and JettyWebSocketServlet extends org.eclipse.jetty.websocket.servlet.WebSocketServlet

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM

@lachlan-roberts lachlan-roberts merged commit 372ed14 into jetty-10.0.x Jun 17, 2019
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-3661-JettyWebSocketServer branch June 17, 2019 07:12
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

Successfully merging this pull request may close these issues.

None yet

2 participants