Skip to content

Conversation

@NicoNes
Copy link
Contributor

@NicoNes NicoNes commented Aug 13, 2019

No description provided.

@NicoNes
Copy link
Contributor Author

NicoNes commented Aug 13, 2019

I don' t know why but one travis build failed...

@ronsigal ronsigal added the main label Aug 22, 2019
Copy link
Member

@asoldano asoldano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @NicoNes . On top of the request for changes below, I'm wondering wether we should have a mention to this in the documentation, otherwise users might simply wonder why their cookies do not work by default... WDYT?

log.info("starting testSessionScope()");
client.close();
client = ClientBuilder.newClient();
client = new ResteasyClientBuilderImpl().cookieManagementEnabled(true).build();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not reference the ResteasyClientBuilderImpl directly; use:

client = ((ResteasyClientBuilder)(ClientBuilder.newBuilder())).cookieManagementEnabled(true).build();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@BeforeClass
public static void init() {
client = ClientBuilder.newClient();
client = new ResteasyClientBuilderImpl().cookieManagementEnabled(true).build();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: NicoNes <nicolas.nesmon@gmail.com>
Signed-off-by: NicoNes <nicolas.nesmon@gmail.com>
…yClientBuilderImpl.

- Enrich userguide

Signed-off-by: NicoNes <nicolas.nesmon@gmail.com>
@NicoNes
Copy link
Contributor Author

NicoNes commented Sep 1, 2019

Hi @asoldano ,

You're right we should add documentation about this.
I tried to do so adding few words in RESTEasy_Client_Framework.xml. Let me know what you think about it. (And do no hesitate to correct my english also if necessary ;)).

Thanks

@asoldano
Copy link
Member

@NicoNes sorry for the late feedback. Thanks for the addition to the doc. I'm merging this because I don't want to make the process over complicated. However, I've just realized that it would probably be better to have a enableCookieManagement() method in the builder, similarly to what we have for using async, disabling trustmanager, etc. I'll change that just after merging. Thanks.

@asoldano asoldano merged commit 2edbeaa into resteasy:master Sep 20, 2019
@NicoNes
Copy link
Contributor Author

NicoNes commented Sep 20, 2019

Hey @asoldano,

I thought about it when I did it and I hesitated because, once enableCookieManagement() will be called, it will not longer be possible to build a ResteasyClient instance without cookie management from this builder.
In other way the builder is no longer usable as we expect it to be.

But as you said it's already the case for other methods so let's keep it consistent.

Thanks !

@asoldano
Copy link
Member

Follow-up on #2169

asoldano added a commit to asoldano/Resteasy that referenced this pull request Sep 24, 2019
…ent cookie management feature by default (resteasy#2135)"

This reverts commit 2edbeaa.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants