KTOR-7722 content negotiation client accept header control

[rocketraman]

Subsystem
Client, ContentNegotiation

Motivation
See https://youtrack.jetbrains.com/issue/KTOR-7722/ContentNegotiation-client-plugin-no-way-to-opt-out-of-Accept-on-a-per-request-basis.

Solution
Implement the two recommendations outlined in the previous issue.

1. Allow configuring the default q-value for registered content types. By default, for backward compatibility, the behavior is the same as today: no q-value is set. However, this config parameter allows users to set configure the default accept with a lower q setting. With this configuration, user's can easily override the preferred Accept on a per-request basis.
2. If the registered content type is already set on the request with additional parameters, the plugin does not add the accept header again.

[osipxd]

Thank you for the pull request!

@marychatte, could you take a look?

[marychatte]

Thank you for the idea! I think it's better to introduce a new method exclude(contentType) instead. So it will look like:

install(ContentNegotiation) {
    register(ContentType.Application.Json, TestContentConverter())
}

client.get("https://test.com/") {
    accept(ContentType.Application.Pdf)
    exclude(ContentType.Application.Json)
}

[rocketraman]

Thank you for the idea! I think it's better to introduce a new method exclude(contentType) instead.

@marychatte I can do that. However, it seems like that change could be made in addition to the changes I have already made rather than instead of. It would still seem useful to be able to set the default q-value, as well as the change to not add a duplicate accept type in the presence of an existing one with different parameters. Thoughts?

[osipxd]

I'd say adding the same content type with parameters should simply replace the default one.
At least I can't come up with any use case when we would keep the default content type and the same content type with parameters. Is there any?

[rocketraman]

I'd say adding the same content type with parameters should simply replace the default one.
At least I can't come up with any use case when we would keep the default content type and the same content type with parameters. Is there any?

Correct. Keeping the content type and adding the same content type with parameters is what the existing code does, and one of the items fixed by my PR. So therefore it seems like we agree this change should be made.

The other change that is in the PR so far is the ability to set a default q value for the registered content types. Which I think is also useful. But less so than the ability to override parameters.

[rocketraman]

@marychatte I've added the exclude functionality which was requested. As per earlier comments, I've left the other functionality added as well. Please review.

[osipxd]

The other change is the ability to set a default q value for the registered content types.

Couldn't this be solved by adding a "q" value to the default content type?

install(ContentNegotiation) {
    register(ContentType.Application.Json.withParameter("q", "0.8"), TestContentConverter())
}

However I agree it might look weird.

[rocketraman]

The other change is the ability to set a default q value for the registered content types.

Couldn't this be solved by adding a "q" value to the default content type?

install(ContentNegotiation) {
    register(ContentType.Application.Json.withParameter("q", "0.8"), TestContentConverter())
}

However I agree it might look weird.

I thought about that, but there were a couple of reasons I didn't go with that approach.

First, the value that is passed to register is used in quite a few places. I saw that registering a content type with parameters would violate some assumptions the existing code was making (for example, this line of code).

Second, most people don't call the register function directly, but instead use type-specific registration functions, like this one. I didn't want to complicate or change this API.

[marychatte]

@rocketraman Thanks for the changes! I wrote some comments, but the general idea is good
@osipxd What do you think about the name defaultAcceptHeaderQValue ?

[marychatte]

First, the value that is passed to register is used in quite a few places. I saw that registering a content type with parameters would violate some assumptions the existing code was making (for example, this line of code).

I think it's a bug in the code, and it should be:

val matcher = when  {
    contentType.match(ContentType.Application.Json) -> JsonContentTypeMatcher
    else -> defaultMatcher(contentType)
}

Can we fix it in this PR as well?

[rocketraman]

@marychatte Thanks for the review. I've pushed 3 additional commits.

[marychatte]

LGTM