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

KTOR-6946 headers in the default request are overriding new custom headers #4157

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

igloo12
Copy link
Contributor

@igloo12 igloo12 commented Jul 24, 2024

Subsystem
Client

Motivation
I need to override the default headers with custom headers

Solution
The issue is with this code block.

val defaultRequest = DefaultRequestBuilder().apply {
    headers.appendAll(this@intercept.context.headers)
    plugin.block(this)
}

The plugin call is erasing the headers. I reversed the order and added a new method to the builder to replace the ones set by the default block

 val defaultRequest = DefaultRequestBuilder().apply {
    plugin.block(this)
    headers.replaceAll(this@intercept.context.headers)
}

@e5l e5l requested a review from marychatte July 25, 2024 13:27
Copy link
Member

@marychatte marychatte left a comment

Choose a reason for hiding this comment

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

@igloo12 Thank you for the PR! I noticed that these changes break some tests in DefaultRequestTest.kt, and I'm not sure if it's better to change the plugin's behavior. Isn't the next workaround applicable to your case?

defaultRequest {
    headers.appendIfNameAbsent(HttpHeaders.ContentType, ContentType.Application.Json.toString())
}

@igloo12 igloo12 closed this Jul 30, 2024
@igloo12 igloo12 deleted the KTOR-6946 branch July 30, 2024 22:42
@igloo12 igloo12 restored the KTOR-6946 branch July 30, 2024 22:55
@igloo12 igloo12 reopened this Jul 30, 2024
@igloo12
Copy link
Contributor Author

igloo12 commented Jul 30, 2024

@marychatte, I changed the PR to override only the content type. My logic is the content type set via contextType(), which hides the underlying header setting/appending logic. When a user sets contextType, the default should be overridden because there can only be one.

What do you think?

@e5l e5l requested a review from marychatte August 2, 2024 08:10
@e5l
Copy link
Member

e5l commented Oct 7, 2024

@marychatte, could you check?

@marychatte
Copy link
Member

As described in the PR, the plugin DefaultRequest can override headers that the user sets. However, the user's headers should have higher priority:

defaultRequest {
    contentType(ContentType.Application.Json) // defaultRequest header
}

val response = client.get("/") {
    contentType(ContentType.Application.Xml) // users' header
}

The final ContentType should be ContentType.Application.Xml

So, we need to set the user's headers, and also because defaultRequest logic could depend on them, for example:

defaultRequest {
   headers.appendIfNameAbsent("header", "value)
 }

But there is a small problem that some Headers functions override value completely, like with function contentType(...), because they are written through headers.set(...)

Proposed solution:

  1. Set and save headers after user
  2. Apply DefaultRequest plugin
  3. Check that the user's headers weren't overridden: we just append firstly user header values and then DefaultRequest header values
    Note: Different check for the HttpHeaders.Cookie because if it has several values, they are presented as one ,and they are combined automatically in the function cookie(...)

@marychatte
Copy link
Member

@e5l please review the suggested changes, I described them in the comment

Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

lgtm

@marychatte marychatte merged commit 147c7a4 into ktorio:main Oct 29, 2024
9 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants