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

L00kian
Copy link
Contributor

@L00kian L00kian 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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 10, 2019
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 10, 2019
@philwebb philwebb added this to the 2.2.x milestone Jun 10, 2019
@L00kian
Copy link
Contributor Author

L00kian 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
Copy link
Contributor

ielatif 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 17072-default-headers-support branch from 20f5026 to baf4021 Compare June 11, 2019 10:18
@L00kian
Copy link
Contributor Author

L00kian commented Jun 11, 2019

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

@ielatif
Copy link
Contributor

ielatif commented Jun 11, 2019

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

Yep I am curious too :)

@wilkinsona
Copy link
Member

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 17072-default-headers-support branch from baf4021 to d905761 Compare June 11, 2019 11:34
philwebb pushed a commit that referenced this pull request Jun 12, 2019
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
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
Copy link
Member

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
Copy link
Contributor Author

L00kian 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 17072-default-headers-support branch June 13, 2019 07:39
@philwebb
Copy link
Member

@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
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants