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

Add ability to set attributes on a SslContext #9654

Merged
merged 3 commits into from Oct 22, 2019

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Oct 11, 2019

Motivation:

Sometimes it is useful to be able to set attributes on a SslContext.

Modifications:

Add new method that will return a AttributeMap that is tied to a SslContext instance

Result:

Fixes #6542.

Motivation:

Sometimes it is useful to be able to set attributes on a SslContext.

Modifications:

Add new method that will return a AttributeMap that is tied to a SslContext instance

Result:

Fixes #6542.
@normanmaurer normanmaurer requested review from rkapsi and njhill Oct 11, 2019
@slandelle

This comment has been minimized.

Copy link
Contributor

slandelle commented Oct 11, 2019

Do you have an example of a use case?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 14, 2019

@slandelle #6542 would be an example.... that said it is basically the same as SSLSession.putValue etc.

@rkapsi
rkapsi approved these changes Oct 14, 2019
Copy link
Member

rkapsi left a comment

LGTM

@njhill
njhill approved these changes Oct 15, 2019
Copy link
Member

njhill left a comment

Did you consider having SslContext extend DefaultAttributeMap a la AbstractChannel? (I know there are downsides of doing that too)

/**
* Returns the {@link AttributeMap} that is tied to this {@link SslContext} instance.
*/
public final AttributeMap attributeMap() {

This comment has been minimized.

Copy link
@njhill

njhill Oct 15, 2019

Member

WDYT about naming this attributes() instead of attributeMap()?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 15, 2019

@njhill yeah I did consider but I thought how it is done here may be better.. WDYT ?

@njhill

This comment has been minimized.

Copy link
Member

njhill commented Oct 15, 2019

@normanmaurer I don't have a strong opinion but would probably lean towards subclassing, other things being equal. In future if we wanted to extend a different class then DefaultAttributeMap could be moved to a field and just delegate the attr and hasAttr interface methods. Really fine either way though :)

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 16, 2019

@njhill @rkapsi ok did extend at the end to make it consistent.... PTAL again

@rkapsi

This comment has been minimized.

Copy link
Member

rkapsi commented Oct 16, 2019

I'm a sucker for composition. The change is OK but my eye twitches about why SslContext is an instanceof AttributeMap.

@njhill
njhill approved these changes Oct 16, 2019
Copy link
Member

njhill left a comment

LGTM but also fine with reverting if @rkapsi feels strongly!

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 17, 2019

@rkapsi fair enough... use attributes() then.

@normanmaurer normanmaurer requested review from rkapsi and njhill Oct 17, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 17, 2019

@rkapsi @njhill PTAL again

@njhill
njhill approved these changes Oct 17, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 17, 2019

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.43.Final milestone Oct 17, 2019
@rkapsi
rkapsi approved these changes Oct 17, 2019
Copy link
Member

rkapsi left a comment

LGTM and thank you for considering composition over inheritance :)

@normanmaurer normanmaurer merged commit 247a4db into 4.1 Oct 22, 2019
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer normanmaurer deleted the ssl_context_attr_map branch Oct 22, 2019
normanmaurer added a commit that referenced this pull request Oct 22, 2019
Motivation:

Sometimes it is useful to be able to set attributes on a SslContext.

Modifications:

Add new method that will return a AttributeMap that is tied to a SslContext instance

Result:

Fixes #6542.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.