On 01/12/2014 12:34, Rémy Maucherat wrote: > 2014-12-01 11:19 GMT+01:00 Mark Thomas <ma...@apache.org>: > >> On 28/11/2014 20:33, r...@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.
I can see where this test is coming from but disabling the check that a valid extension is being requested just to pass the test is the wrong thing to do - particularly given your next comment. > 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. OK. That test needs to be challenged as well then. I see two issues from this thread: - The test requires the server to support arbitrary extensions which prevents the server from throwing an exception and preventing deployment when a server endpoint requests an unsupported extension. - The test does not expect the server to support any other extensions and it is likely that implementations will support at some. I'm not sure the best way of handling the first point. I can see the need to test the API but it is more important to me that configuration / coding errors that request an unsupported extension are reported early. Possible options: a) Require servers to support a NO-OP extension. Kind of pointless since it would just be there to pass the tests. b) Drop the test. Not ideal. If the API is there it should be tested. c) Add a "ignore unknown extensions flag" we use just for running the tests. d) Require compression support and then use that to test the API. e) Add support for application provided extensions and then use that to test the API. c) for now followed by e) is probably the best solution with c) being removed once e) was in place. <snip/> >>> - 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 :) I agree it has some uses. I think this also needs raising with the EG for clarification. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
