Upgrade Feign

[pkoenig10]

Fixes #790
Fixes #1330
Fixes #1331

Possible downsides?

This upgrade causes a regression in the handling of optional<string> header parameters when the value is an empty string. Before this PR these parameters would be serialized as a present header with an empty string value. After this PR these parameters will be omitted from the request and the server will deserialize the parameter as an absent optional value.

However, this PR does fix the incorrect handling of absent optional values (all types of optional values are affected by this bug). This tradeoff seems like a net positive, see #790 (comment). We're correcting the behavior of absent optional values of any type at the cost of empty string optional values.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Feign has been upgraded to 10.6.0. This involved a maven group change from com.netflix.feign -> io.github.openfeign. This fixes the serialization of absent optional header parameters.

Check the box to generate changelog(s)

• Generate changelog entry

[pkoenig10]

Feign removed the ability to escape expansion of variables by using {{ in OpenFeign/feign#778. I'm not sure what the best solution is to correctly serialize the hr-path-template and use it as the span name.

[iamdanfox]

Looks awesome @pkoenig10 thank you for chasing this down.

For the hr-path-template thing, perhaps something like << and >> would do the trick?

Also I think I remember something to do with possible classpath conflicts - both io.github.openfeign:feign-core and com.netflix.feign:feign-core provide some identically named classes right? So there's a bit of a danger that if someone happens to have remoting3 or directly depend on old feign they might get spicy nosuchmethoderrors?

[pkoenig10]

For the hr-path-template thing, perhaps something like << and >> would do the trick?

Yeah, I just wanted to make sure it was acceptable to change the format. I think this is only used to generate span names, so it should be fine to just use another delimiter.

Also I think I remember something to do with possible classpath conflicts - both io.github.openfeign:feign-core and com.netflix.feign:feign-core provide some identically named classes right? So there's a bit of a danger that if someone happens to have remoting3 or directly depend on old feign they might get spicy nosuchmethoderrors?

Yeah this is definitely a risk. But it's probably something we will need to address eventually. Do we have any tooling to handle these types of upgrades? Maybe we could update the conjure upgrade excavator to somehow handle this?

[iamdanfox]

@pkoenig10 I think the TODO above might be out of date... afaik the reason this has to be api is because people may want to catch a RetryableException

[iamdanfox]

So I think for most consumers of c-j-r the dependency problems should be fine and the PR will clearly show all the com.netflix.feign:feign-core lines disappearing from versions.lock and io.github.openfeign:feign-core appearing, but for anyone who builds their own feign clients (e.g. several of @gatesn's services), they'll need to change their gradle.

I think before rolling this out I'd like to figure out the right gradle concept to make sure these don't fight (might be capabilities or it might be plain old dependency substitution). e.g.

allprojects {
    dependencies {
        modules {
            module('com.netflix.feign:feign-core') {
                replacedBy('io.github.openfeign:feign-core', 'Feign is now published under new maven coordinates')
            }
        }
    }
}

I think Gradle will now consider the two modules as a single module in dependency resolution and will never include both, but there's still a danger of NoSuchMethodErrors.

[j-baker]

Please can we just shade the Feign we use? This stuff ends up really hard to upgrade, we shouldn't let diamond dependencies hurt us.

[pkoenig10]

Please can we just shade the Feign we use?

Yeah this seems like the best approach. We would also have to keep the old Feign dependency to avoid runtime errors if users are transitively depending on it, right?

[carterkozak]

We would also have to keep the old Feign dependency to avoid runtime errors if users are transitively depending on it, right?

If we rev the major version we can avoid keeping the old feign dependency, it would cause a potential compile break, not a runtime break.

[pkoenig10]

I was under the impression that GCV does not have a concept of major versions. So GCV could resolve to a 4.x version without the feign dependency even though you're dependency was compiled with a 3.x version and relies on the transitive feign dependency being present.

[pkoenig10]

Also, shading feign will be a breaking change because we have users that catch RetryableException (see #1337 (comment)).

Granted, they probably should just be catching RuntimeException. Maybe this is something we can introduce a baseline check for?

[stale]

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

[pkoenig10]

@j-baker @carterkozak any thoughts on how we could safely shade feign?

[stale]

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.