Issue #3458 - jetty websocket upgrades without using websocket-servlet classes #3460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lachlan-roberts
changed the title
gregw
requested changes
Mar 13, 2019
| ExtensionConfig parse(String parameterizedName); | ||
| List<ExtensionConfig> parseEnum(Enumeration<String> valuesEnum); | ||
| List<ExtensionConfig> parseList(String... rawSecWebSocketExtensions); | ||
| String toHeaderValue(List<ExtensionConfig> configs); |
There was a problem hiding this comment.
are all these needed on the interface?
| } | ||
|
|
||
| public ExtensionConfig(String parameterizedName) | ||
| static String toHeaderValue(List<ExtensionConfig> configs) |
There was a problem hiding this comment.
Is this needed on the interface?
gregw
requested changes
Mar 13, 2019
lachlan-roberts
force-pushed
the
jetty-10.0.x-3458-jettyWebSocket-API
branch
from
b704dea
to
0c1e34d
Compare
|
@gregw i think it would be worth looking at the problems with internal extensions and changing extension parameters from the requested parameters in separate pull requests |
gregw
requested changes
Mar 14, 2019
| } | ||
| catch (IllegalArgumentException e) | ||
| { | ||
| resp.setExtensions(List.of(permessageDeflate)); |
...tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketExtensionConfigTest.java
Show resolved
Hide resolved
| assertEquals(req.getExtensions().stream().filter(e -> e.getName().equals("permessage-deflate")).count(), 1); | ||
| assertEquals(resp.getExtensions().stream().filter(e -> e.getName().equals("permessage-deflate")).count(), 1); | ||
|
|
||
| ExtensionConfig permessageDeflate = ExtensionConfig.parse("permessage-deflate"); |
There was a problem hiding this comment.
should test setting parameters here!
lachlan-roberts
force-pushed
the
jetty-10.0.x-3458-jettyWebSocket-API
branch
from
0c1e34d
to
89bee3f
Compare
|
@gregw I think ExtensionConfig needs further testing to test internal extensions (which are not currently implemented properly) and to test things like the client validation of the upgrade response (which currently has some bugs) however i think these things are better to be tested in core, and this test should be more about whether extension config is wired up with the jetty-websocket-api rather than testing every case of ExtensionConfig |
ExtensionConfig is now interface in jetty-websocket-api and implemented in jetty-websocket-common, for its static methods it now uses a ExtensionConfig.Parser found by the ServiceLoader the api tests were moved to jetty-websocket-tests Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
websocket-servlet exposes core classes as it is used by the jetty and javax sides, so this introduces a way to add websocket mappings with jetty-server which does not depend on websocket-servlet to set websocket mappings through the JettyWebSocketServerContainer you now need to use the JettyWebSocketCreator which abstracts away the core classes from use with the jetty websocket api Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
wire up the jetty-api extensions by copying the ExtensionConfig from the jetty-api upgrade request to the core upgrade request make UpgradeListener interface methods default for convenience introduce test to test the functionality of API ExtensionConfig Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: lachan-roberts <lachlan@webtide.com>
lachlan-roberts
force-pushed
the
jetty-10.0.x-3458-jettyWebSocket-API
branch
from
7aa6c62
to
dab3b64
Compare
gregw
approved these changes
Mar 18, 2019
|
The force push makes it very hard to review the changes.... but as WS is still a WIP, I willl merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#3458
remove the dependence on jetty-servlet to use websocket server with the jetty-websocket-api
jetty-servlet exposes core classes as it is used by both the javax and jetty sides
a jetty-server
WebSocketCreatorcan now be made and be mapped to a pathspec inJettyWebSocketServerContainerjetty-websocket-api now defines interface
ExtensionConfigwhich is implemented byJettyExtensionConfigin jetty-websocket-common which delegates to the core version, this is to not reimplement this in API and to also not expose the core classes