FIXED unsupported jaxrs-2.1 annotations should not break entire interface - #672
Conversation
| <groupId>javax.ws.rs</groupId> | ||
| <artifactId>jsr311-api</artifactId> | ||
| <version>1.1.1</version> | ||
| <artifactId>javax.ws.rs-api</artifactId> |
There was a problem hiding this comment.
Hmm. Dangerous thing to do in a minor, changing a dependency like this
There was a problem hiding this comment.
how? jax-rs-2.1 is fully backwards compatible down to 1.1.
There was a problem hiding this comment.
I think one way to address this, is to keep the original dependency and add the new one as provided.
For those using jax-rs 2, they would need to explicitly declare the dependency. And for the ones already using jax-rs 1 no need for changes.
There was a problem hiding this comment.
@spencergibb how is this a minor btw? I'm actually considering jax-rs-2.x breakage pretty much a major considering there's no applicable workaround and the spec has been around for half a decade by now. In consideration of 2.x being backwards compatible thus applicable as drop-in substitute, would make jsr-311 legacy support pretty much a minor instead.
|
Some testing would be nice, but my main concern is breaking for people using Jax RS 1 |
|
Agreed. Changing a dependencies major version may cause users of the client headaches if they aren't prepared when they update. |
|
while I don't share any concerns for breakage technically, I would agree it's formally not appropriate to bump this spec as part of a minor patch. |
| // masc20180327. parameter with unsupported jax-rs annotations should not be passed as body params. | ||
| // this will prevent interfaces from becoming unusable entirely due to single (unsupported) endpoints. | ||
| // https://github.com/OpenFeign/feign/issues/669 | ||
| if (annotationType == Suspended.class || annotationType == Context.class) { |
There was a problem hiding this comment.
Can we use a String check here?
annotationType.getClass().equals("javax.ws.rs.container.Suspended")
Why? To prevent class def not found for anyone that still wanna use jax-rs 1
There was a problem hiding this comment.
yes that may work, but it's ugly and prone to breakage on maintenance so I won't supply this. I'd rather have feign-jaxrs-2. also I don't see any reason (for anyone) to even include jax-rs-1.
There was a problem hiding this comment.
how do you plan to support jax-rs-2 in feign in general?
There was a problem hiding this comment.
it's ugly and prone to breakage on maintenance
agree
may be a try-catch for the ClassDefNotFound
There was a problem hiding this comment.
@velo is it universally predictable when jvm's throw this?
There was a problem hiding this comment.
I would assume this is thrown by jvm before processAnnotationsOnParameter is invoked even, but I could be wrong.
There was a problem hiding this comment.
I made a PR with an "example" and the exception as well
https://github.com/masc3d/feign/pull/1
There was a problem hiding this comment.
Not sure if it's overkill or not, but you could always use the maven-invoker-plugin to test compatibility.
| <groupId>javax.ws.rs</groupId> | ||
| <artifactId>jsr311-api</artifactId> | ||
| <version>1.1.1</version> | ||
| <artifactId>javax.ws.rs-api</artifactId> |
There was a problem hiding this comment.
I think one way to address this, is to keep the original dependency and add the new one as provided.
For those using jax-rs 2, they would need to explicitly declare the dependency. And for the ones already using jax-rs 1 no need for changes.
Except for the people who can't or aren't willing to upgrade. |
|
If it's acceptable to have |
|
@masc3d have you considered creating a Thoughts? |
|
I think those are the only two options |
|
@kdavisk6 yes I proposed this one comment above, still waiting for feedback. |
if you can agree on a major version bump anytime soon it probably doesn't make sense to spawn a new artifact now. |
|
I'm in the camp of creating a major release of |
|
It would be the whole project since modules are not released independently |
|
Oh, I was not aware of that. |
|
I decided I won't provide a jsr311 compatible setup. those consumers which are allegedly adversely affected in a way that can't be easily mitigated probably don't even exist. |
|
After reading into this further, it's difficult to say with any confidence how many users of Feign are still using JAX-RS 1.x (jsr311) libraries. With that in mind, my suggestion would be deprecate This will allow users of the library who are still on JSR-311 to receive core feign updates and those that want to take advantage of JAX-RS 2.0 features the ability to do so. Thoughts? |
velo
left a comment
There was a problem hiding this comment.
Awesome job, just 2 small pointers, if you can't do then, lemme know and I will
| <scope>test</scope> | ||
| </dependency> | ||
|
|
||
| <dependency> |
There was a problem hiding this comment.
I don't think this is being used, is it?
There was a problem hiding this comment.
it's required for test as jsr311-api is not pulled in transitively any longer. will add scope.
| @@ -0,0 +1,2 @@ | |||
| # Feign JAXRS 2 | |||
|
|
|||
There was a problem hiding this comment.
Can you just "copy" README from jaxrs?!
UPDATED jaxrs readme
…face (#672) * FIXED unsupported jaxrs-2.1 annotations should not break entire interface, resolving #669 * UPDATED jaxrs: more defensive jaxrs2 support * ADDED jsr311-api dependency to httpclient (as jsr311 is `provided` in feign-jaxrs now) * UPDATED httpclient `jsr311-api` scope to test UPDATED jaxrs readme
…face (#672) * FIXED unsupported jaxrs-2.1 annotations should not break entire interface, resolving #669 * UPDATED jaxrs: more defensive jaxrs2 support * ADDED jsr311-api dependency to httpclient (as jsr311 is `provided` in feign-jaxrs now) * UPDATED httpclient `jsr311-api` scope to test UPDATED jaxrs readme
resolving #669