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

Register websocket endpoints for /mapping/ for /mapping/* servlets #2326

Open
wants to merge 2 commits into
base: master-deprecated
Choose a base branch
from

Conversation

Artur-
Copy link
Contributor

@Artur- Artur- commented Apr 25, 2018

Another attempt for #2316

Only adds a root mapping for the servlet and not the unsupported /{path1}/{path2}/ type paths

Previously for a servlet mapped using /mapping/*, there were mappings done for:
* /mapping
* /mapping/{path1}
* /mapping/{path1}/{path2}
etc

which do not match /mapping/

Tomcat disallows registering websocket endpoints using a trailing slash, except
for /servletMapping/. According to the code this is because
"As per EG discussion, all other empty segments are invalid"
@jfarcand
Copy link
Member

@Artur- I did try that here

3a6a9bc

But it regressed existing apps as well.

@Artur-
Copy link
Contributor Author

Artur- commented Apr 25, 2018

Interesting... in what way as / or /servlet/ should be valid mappings?

@jfarcand
Copy link
Member

@Artur- Actually do you have a test/app I can play with that fail to deploy properly? The issue here is we may need to bump the release to 2.5 to prevent breaking compatibility.

@Artur-
Copy link
Contributor Author

Artur- commented Apr 25, 2018

Have not tested with atmosphere samples but I guess if you deploy a servlet to /* and try to use / for the websocket connection, it won’t work

@fenik17
Copy link
Contributor

fenik17 commented Apr 26, 2018

Static analizer in IntelliJ IDEA found that regexp for cleaning servlet path contains unclosed character class:

private final static Pattern SERVLET_PATH_PATTERN = Pattern.compile("([\\/]?[\\w-[.]]+|[\\/]\\*\\*)+");

@Artur- you can try to use this fixed pattern:

private final static Pattern SERVLET_PATH_PATTERN = Pattern.compile("([/]?[a-zA-Z_0-9.]+?|[/]\\*\\*)+");

Does this solve your issue?

@fenik17
Copy link
Contributor

fenik17 commented May 2, 2018

@Artur- ping

@fenik17
Copy link
Contributor

fenik17 commented May 15, 2018

@Artur-
copy of my question from #2331:

Can you describe the core problem that you are trying to solve?

We can looking for other solutions. Or let me close this PR.

@Artur-
Copy link
Contributor Author

Artur- commented May 15, 2018

The core problem: Deploying a servlet using "/*" should deploy a websocket endpoint at /. Similarly, deployingn a servlet as /foo/* should deploy a websocket endpoint at /foo/

@fenik17
Copy link
Contributor

fenik17 commented May 15, 2018

should deploy

But where does this requirement come from?
From javadoc of the ServerEndpointConfig.Builder' constructor [1]:

path - The URI or URI template where the endpoint will be deployed. A trailing "/" will be ignored and the path must begin with /.

This means that, as a minimum, there is no difference beetwen /foo and /foo/.

[1] https://docs.oracle.com/javaee/7/api/javax/websocket/server/ServerEndpointConfig.Builder.html#create-java.lang.Class-java.lang.String-

@Artur-
Copy link
Contributor Author

Artur- commented May 16, 2018

JSR356AsyncSupport.java:

if (servletPath == null) {
    servletPath = IOUtils.guestServletPath(config);
    if (servletPath.equals("") || servletPath.equals("/") || servletPath.equals("/*")) {
        servletPath = PATH +"}";
    }
}

For /*, IOUtils.guestServletPath(config); returns "" and servletPath becomes /{path}. This matches /something but not /. This is quite unexpected.

Might be that a /foo/* mapping actually works correctly with current master. I can't see directly why it would not, as servletPath will then become /foo, without any initially added {path}

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.

3 participants