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 general purpose header support to RestTemplateBuilder #17091

Closed

Conversation

Projects
None yet
6 participants
@L00kian
Copy link
Contributor

commented Jun 10, 2019

See #17072 for details.

Added convenience methods for customising the RestTemplate built HttpHeaders handling. The #17010 approach was taken as a baseline. The underlying BasicAuthenticationClientHttpRequestFactory reworked to accept any number of HttpHeadersCustomizer instances to handle http request headers. Added convenience methods to RestTemplateBuilder to populate a 'default header' as well as custom HttpHeadersCustomizer implementations. Refactored #17010 changes to follow the same HttpHeadersCustomizer concept

@L00kian

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Hello,
I was trying to re-trigger the build as the failure doesn't seem to be related to the changes above. However I was unable to login into concourse using GitHub authentication. Clicking on Login button downloads empty 'callback.dms' file. Not sure if that's an expected behaviour as well as to whom to address it to. Still I think it worth mentioning

Thanks.

@ielatif

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

https://ci.spring.io/builds/79665

[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ReactiveElasticsearchRepositoriesAutoConfigurationTests ? ExceptionInInitializer
[ERROR] ReactiveRestClientAutoConfigurationTests ? ExceptionInInitializer
[INFO]
[ERROR] Tests run: 2185, Failures: 0, Errors: 2, Skipped: 11

You have to rebase against upstream master @wilkinsona has fixed this in #17092

@L00kian L00kian force-pushed the L00kian:17072-default-headers-support branch from 20f5026 to baf4021 Jun 11, 2019

@L00kian

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Yep, will do. However still curious about the CI login :)

@ielatif

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Yep, will do. However still curious about the CI login :)

Yep I am curious too :)

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

It is intentional that you can't log in to Concourse. That's only needed for administrative purposes. AFAIK, adding commits or force-pushing should be sufficient to trigger a new build.

@L00kian L00kian force-pushed the L00kian:17072-default-headers-support branch from baf4021 to d905761 Jun 11, 2019

philwebb added a commit that referenced this pull request Jun 12, 2019

Support default headers with RestTemplateBuilder
Update `RestTemplateBuilder` so that it is easier to apply custom
headers to the outgoing request. The update is particularly useful
for setting the `User-Agent` header, for example so that a GitHub
username can be used when calling `api.github.com`.

See gh-17091

philwebb added a commit that referenced this pull request Jun 12, 2019

Polish "Support default headers with RestTemplateBuilder"
Broaden the scope of customizer support so that instead of focusing
just on headers, we can now customize any outgoing `HttpClientRequest`.
Also update auto-configuration to automatically add any
`RestTemplateRequestCustomizer` beans to the builder.

See gh-17091

@philwebb philwebb closed this in 2888dde Jun 12, 2019

@philwebb

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Thanks for the PR @L00kian! This has now been merged into master.

Whilst working on the merge it hit me that we can generalize the customizer even more and one that applies to ClientHttpRequest rather than just the headers. I've done this in commit aad21d1. I've also made a small framework change that makes the "add a header if not already present" use-case much easier.

Thanks again for the contribution!

@mbhave mbhave modified the milestones: 2.2.x, 2.2.0.M4 Jun 13, 2019

@L00kian

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Thanks!
Just curious why basic authentication needs a special case for itself. Is there any specific reason why it shouldn't be handled by a specific request customiser as well?

@L00kian L00kian deleted the L00kian:17072-default-headers-support branch Jun 13, 2019

@philwebb

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@L00kian I started to get worried that users might call requestCustomizers and accidentally wipe out their basicAuth settings. In other words, the following would work:

builder.requestCustomizers(customizers).basicAuth("user", "password");

but this version would not:

builder.basicAuth("user", "password").requestCustomizers(customizers);

In the end I decided to make the basicAuth and defaultHeader stuff distinct from the requestCustomizers. The BasicAuthentication class itself could have been a RestTemplateRequestCustomizer, but since it's package private and only really needs the headers I just decided to call the applyTo method directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.