Fix #340 - Clarify extension handling

[markt-asf]

Keeping this in draft at this point as further changes are expected.

The specification document changes are mostly complete although I am expecting the wording to be improved based on review.

The API changes are intended as a starting point. I'll add some Javadoc once the API is more settled.

Feedback welcome. I'll leave it at least a couple of weeks before moving this to non-draft and merging.

[markt-asf]

This has been open for almost a month. I was expecting more feedback. I'm moving this out of draft and currently intend to merge in the next week or so.

[joakime]

Sorry, I was out on break and missed this one.

[joakime]

I'll try to get feedback on this within the next 24 hours.

[lachlan-roberts]

I would consider moving this outside of ClientEndpoint so it could be used on the Server side as well, because there is ServerEndpointConfig.getExtensions but no equivalent on the ServerEndpoint annotation.

[markt-asf]

Moving this outside of ClientEndpoint would create a conflict with the Extension interface. Making the annotation available on the server side was also discussed in #340 and no use case was identified. Do you have a use case for this?

[lachlan-roberts]

It is the use case of ServerEndpointConfig#getExtensions which does not make sense to me.

But it seems to me that either ServerEndpointConfig#getExtensions should be deprecated, or there should be an equivalent API on the @ServerEndpoint annotation.

But for example I would imagine something like this, which would allow you to negotiate a specific extension without setting a custom configurator.

@ServerEndpoint(value = "/", extensions = { @Extension(name = "permessage-deflate")})
public class MyServerEndpoint
{
    // ...
}

[markt-asf]

I think that would work for ServerEndpointConfig#getExtensions (matched with new @Extension support in @ServerEndpoint) if the intersection of those extensions and what the container supports was passed as installed to ServerEndpointConfig.Configurator.getNegotiatedExtensions. That would also need explicitly documenting in the spec.

That then begs the question what do we name @Extension. We can't use the obvious jakarta.websocket.Extension as that clashes with the existing interface. @ExtensionConfig?

[markt-asf]

Thanks for the review. I'll make the updates as per my comments shortly and we can continue to discuss the unresolved points.

[markt-asf]

Moving this outside of ClientEndpoint would create a conflict with the Extension interface. Making the annotation available on the server side was also discussed in #340 and no use case was identified. Do you have a use case for this?

[lachlan-roberts]

It is the use case of ServerEndpointConfig#getExtensions which does not make sense to me.

But it seems to me that either ServerEndpointConfig#getExtensions should be deprecated, or there should be an equivalent API on the @ServerEndpoint annotation.

But for example I would imagine something like this, which would allow you to negotiate a specific extension without setting a custom configurator.

@ServerEndpoint(value = "/", extensions = { @Extension(name = "permessage-deflate")})
public class MyServerEndpoint
{
    // ...
}

[joakime]

How would we handle the common use case of rejecting or denying a specific extension from negotiating?

Can someone setup a server endpoint to reject permessage-deflate?

@ServerEndpoint(value = "/", reject-extensions = { @Extension(name = "permessage-deflate")})
public class MyServerEndpoint
{
    // ...
}

or

@ServerEndpoint(value = "/", extensions = { @Extension(name = "permessage-deflate", reject=true)})
public class MyServerEndpoint
{
    // ...
}

or

@ServerEndpoint(value = "/", extensions = { @Extension(name = "!permessage-deflate")})
public class MyServerEndpoint
{
    // ...
}

[markt-asf]

How would we handle the common use case of rejecting or denying a specific extension from negotiating?

Can someone setup a server endpoint to reject permessage-deflate?

Do we need to? The mode for the client is that it declares the list of extensions it is prepared to support. Given the small number of extensions, that model should work for the server too. If we did adopt this feature, I'd lean towards the first option - although I'd use rejectExtensions for the annotation parameter name.

[joakime]

How would we handle the common use case of rejecting or denying a specific extension from negotiating?
Can someone setup a server endpoint to reject permessage-deflate?

Do we need to? The mode for the client is that it declares the list of extensions it is prepared to support. Given the small number of extensions, that model should work for the server too. If we did adopt this feature, I'd lean towards the first option - although I'd use rejectExtensions for the annotation parameter name.

Yes, it is a common use case.
Take for example the use case of a Web Browser connecting to a developed WebSocket Server Endpoint.
The Web Browser will have hard-coded list of extensions it advertises (list can change with a new version of the web browser).
The developer of the WebSocket Server Endpoint knows that one of the extensions is supported and available by the server, but they do not want to use that extension for whatever reason. (bugs in server, bugs in websocket client, network auditing tools, debugging reasons, network capture reasons, etc...)

[markt-asf]

Sorry, not progressing this as fast as I would like. Other tasks keep getting in the way.
My current intention is to update this PR along the following lines:

• Rename the Extension (and nested Parameter annotation) annotation to jakarta.websocket.ExtensionConfig
• Use this annotation on ClientEndpoint and ServerEndpoint
• Use a naming convention (TBD - see below) to allow denying an extension but only for the ServerEndpoint (the client always has to list all the extensions it would like to use)

While I like the use of ! as a prefix to indicate the extension should not be allowed, ! is a permitted character in an extension name. I know it is unlikely to be used but I'd rather not design in that potential risk given we can avoid it. We have a choice of "(),/:;<=>?@[\]{}. How about <?

[markt-asf]

Updated PR for review. Once we have agreement, I'll likely refactor the commits to make the changes to main cleaner.

[joakime]

I'll have a chance to go over this better early next week.

[volosied]

Small comment -- Change should be listed under the change log of the spec document.

[markt-asf]

Re-opening

[markt-asf]

Just rebasing the PR.

[markt-asf]

@volosied change log entry added

@joakime Have you had a change to review this? I don't want to start work on some TCK tests until the expected behaviour is agreed.

[joakime]

I'll defer to @lachlan-roberts

[lachlan-roberts]

I think we should consider the use-case of declaring that no extensions should be negotiated, because an extensionConfigs = {} declaration will allow negotiation of all extensions the server supports.

Say the server supports extensions {A, B, C, D, E}, all are offered to negotiate by the Client.

@Extension Declaration	Negotiated Extensions
{<A}	{B, C, D, E}
{<A, B}	{B}
{B}	{B}
{}	{A, B, C, D, E}
(currently not possible)	{}

Perhaps this include/exclude semantic adds too much complexity considering there is only two extensions defined in the extension registry.

But if we are to keep it, think we should at least specify that the allow-list takes precedence, so that {<A, B} results in {B} and not {B, C, D, E}.

[markt-asf]

I think the case has been made above for the allow/deny semantic so I am assuming we need to keep it.

I like your explanatory table. I'll add that to the spec doc.

I agree the allow list should take precedence and can document that.

How about {<} for deny all?

I think that covers all of the points raised. If there is agreement on all of these I can update the PR and add start working on the TCK.

[lachlan-roberts]

How about {<} for deny all?

Okay, that works.

I think that covers all of the points raised. If there is agreement on all of these I can update the PR and add start working on the TCK.

+1 from me

[joakime]

I think that covers all of the points raised. If there is agreement on all of these I can update the PR and add start working on the TCK.

+1 from me too.