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

interface level header defaults #184

Closed
jdamick opened this issue Feb 12, 2015 · 9 comments · Fixed by #204
Closed

interface level header defaults #184

jdamick opened this issue Feb 12, 2015 · 9 comments · Fixed by #204
Milestone

Comments

@jdamick
Copy link
Contributor

jdamick commented Feb 12, 2015

I implemented an enhanced contract that extends the default contract but adds the ability to set interface level Headers as defaults unless overridden at the method level..

I also added convenience annotations for ContentType & Accept since those (at least in our experience) are most typically set.
The annotations follow more of the jaxrs style, but another option I could see would be adding these as settables in the Feign.Builder when creating the client.. I'm on fence as which one i like better.

Are either of these of interest as a pull? @adriancole

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Feb 12, 2015 via email

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Feb 12, 2015 via email

@jdamick
Copy link
Contributor Author

jdamick commented Feb 12, 2015

yeah, that's good too. How would you set the media type for that interceptor?
some like:

Feign.builder().requestInterceptor(new RequestMediaTypeInterceptor("application/json"))

or like

Feign.builder().contentType("application/json") // under the covers add the interceptor?

or did you have something else in mind?

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Feb 12, 2015 via email

jdamick added a commit to jdamick/feign that referenced this issue Feb 13, 2015
changed the Headers to be able to be on a type. Some of the naming may
need to be tweaked to match Feign conventions..
@jdamick
Copy link
Contributor Author

jdamick commented Feb 13, 2015

I linked to a commit ^ - before i spend too much more time i wanted to see if this was more inline.. (i'm sure some naming might need tweaking)

@codefromthecrypt
Copy link
Contributor

Looks good! One nit would be to adjust the javadoc on Headers to suggest that they are additive, so don't place anything on the type that isn't meant for all methods. That and a test case that shows this. (ex. adding the same header twice = 2 values). Good job, Jeff!

@codefromthecrypt
Copy link
Contributor

oof.. @jdamick we have a hitch. My intention is that 8.x is feature compatible with 7.x. That way, folks can migrate cleanly off dagger via the 7-8 bridge. Also, Spring implement Contract. We probably want to avoid changing the Contract interface as a part of this change.

What that means is that we'd need some logic to reach up to the type in parseAndValidatateMetadata to find the base headers. Ex.

Headers fromType = method.getDeclaringClass().getAnnotation(Headers.class);

Ack that is more expensive than your approach. Wdyt about saving Contract interface refactors for when we bang our heads around hystrix/rxnetty?

@jdamick
Copy link
Contributor Author

jdamick commented Feb 19, 2015

I have no burning need for it at the moment, so i'm fine with waiting until 9.

@codefromthecrypt codefromthecrypt added this to the 9.0.0 milestone Feb 19, 2015
codefromthecrypt pushed a commit that referenced this issue Mar 10, 2015
This supports interface-level defaults, such as Content-Type.

closes #184
codefromthecrypt pushed a commit that referenced this issue Mar 10, 2015
This supports interface-level defaults, such as Content-Type.

closes #184
codefromthecrypt pushed a commit that referenced this issue Mar 10, 2015
This supports interface-level defaults, such as Content-Type.

closes #184
@codefromthecrypt codefromthecrypt modified the milestones: 7.4.0, 9.0.0 Mar 10, 2015
@codefromthecrypt
Copy link
Contributor

added to 7.4/8.1 release after realizing a simple way to accomplish this portably.

codefromthecrypt pushed a commit that referenced this issue Mar 10, 2015
This supports interface-level defaults, such as Content-Type.

closes #184
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 a pull request may close this issue.

2 participants