tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rémy Maucherat <r...@apache.org>
Subject Re: svn commit: r1642360 - in /tomcat/trunk/java/org/apache/tomcat/websocket: Constants.java LocalStrings.properties TransformationFactory.java WsWebSocketContainer.java server/UpgradeUtil.java server/WsHandshakeRequest.java
Date Mon, 01 Dec 2014 12:34:17 GMT
2014-12-01 11:19 GMT+01:00 Mark Thomas <markt@apache.org>:

> On 28/11/2014 20:33, remm@apache.org wrote:
> > Author: remm
> > Date: Fri Nov 28 20:33:20 2014
> > New Revision: 1642360
> >
> > URL: http://svn.apache.org/r1642360
> > Log:
> > - Use the extensions specified by the configuration (and ignore if there
> are no associated transformations).
>
> I can see how this would make sense if there was a way for the
> application to configure its own extensions but there isn't. At the
> moment extensions have to be hard-coded into the container and there is
> no requirement to support any extension. If a server endpoint requests
> an unsupported extension then shouldn't that trigger some sort of error?
>

The tests use extensions (to see if it's supported). They are declared and
of course they do nothing special, but it is supposed to be usable. To
enable some, it uses a ServerEndpointConfig that overrides getExtensions,
declared using a ServerApplicationConfig. Creativity :) Basically, if you
give these guys some APIs that are placeholders but are user accessible
they'll try to test them anyway.

Actually, the test is bad since having a builtin extension breaks it (it
just matches the header), but I ignored that part after verifying it.

>
> > - Add an origin header on the client.
>
> There are multiple issues with this part of the commit:
> - the target is added rather than the origin
> - the full URI is used rather than scheme, host and optional port
>
> See http://tools.ietf.org/html/rfc6454#section-7
>

Yes, I put some random URL I had on hand, I'll have to "improve" it.

>
> > - Add path params as params too.
>
> I don't see that discussed anywhere in the WebSocket spec.
>
> I went back through the EG discussions and the thread that I think led
> up to this [1],[2] was very clear that this was the query parameters and
> did not include the path parameters
>
> [1]
>
> https://java.net/projects/websocket-spec/lists/jsr356-experts/archive/2012-07/message/2
> [2] http://markmail.org/message/qqwqcyg4npxv3bks
>

Yes, I cannot find any mention of that, but the tests are quite explicit
and they use both the query parameters and the path parameters names from
that PathParam annotation. IMO it is not useless, users could maybe have
asked for that. Moar creativity :)

>
> > - Use the case insensitive map for all headers.
>
> Makes sense.
>
> Rémy

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message