fix #4708 refining what is possible via the requestconfig #4837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shawkins
requested review from
manusa,
oscerd,
rohanKanojia and
sunix
as code owners
shawkins
force-pushed
the
iss4708
branch
2 times, most recently
from
4af7ab0
to
66798c0
Compare
|
Tried to quickly merge master but it needs more work 😓 |
That's likely - there have been some overlapping changes of late. After this goes in and #4877, then the extra fields will be removed as well. |
|
Shall we merge #4877 first? (seems cleaner at the moment) |
That's good with me, I'll clean this one up after that. |
|
@manusa should be good now |
|
SonarCloud Quality Gate failed. |
manusa
reviewed
Feb 22, 2023
| } | ||
| public interface RequestTags { | ||
|
|
||
| <T> T getTag(Class<T> type); |
There was a problem hiding this comment.
In the future we should document what a Request tag is and what can it be used for
manusa
approved these changes
Feb 22, 2023
17 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #4708
There are several goals here. The first is to ensure that only config values that can be changed without creating a new httpclient can be set on the RequestConfig - that means connectionTimeout cannot be there. The next is that allowing for authentication related overrides complicates the logic and any documentation we'd want to create around the refresh interceptors, so it's not a good idea to allow that to be set on a per request basis. It also seems unnecessary to override the oauthtokenprovider as the user already has control over how the token should be provided.
The next goal was to minimize our changes to the Config (the full application of those changes is often unclear and/or may require a new httpclient) - after this the config is nearly immutable and not copied - we're no longer creating copies to override the RequestConfig, nor for the namespace handling. This means that places in the client which were referencing the config to get RequestConfig values were changed to reference a contextual RequestConfig. The only logic that is still modifying the config is the token refresh - and this will likely get refined with #4802.
To clean up how Interceptors reference the config, there is not an explicit parameter that finds tags associated with the request - this is not actually on the HttpRequest as we create those in other places and it's simpler not to have to find the tags that were applicable at that point and only deal with this concept as part of the Interceptors. Due to this and the change to using the standard logic we can pass the full request, not just the headers into the interceptor methods. After #4825 we'll be able to also add a generalized after method for regular requests - this should give us the full set of methods needed to introduce generalized logging interception.
This did not attempt to introduce new entry logic for override the requestconfig - it is still through the withRequestConfig method. If we do provide other DSL entry points for such modifications they should use the same internal method for constructing a new Client / derived HttpClient that will have the appropriate overrides.
This does have conflicts with #4825 - but they will be straight-forward to resolve after one of them is committed.
Type of change
test, version modification, documentation, etc.)
Checklist