search

jasminb / jsonapi-converter

JSONAPI-Converter is a Java/Android library that provides support for working with JSONAPI spec
Apache License 2.0
272 stars 81 forks source link

Should ALLOW_UNKNOWN_TYPE_IN_RELATIONSHIP be less restrictive #233

Closed jsteinbrunner closed 4 years ago

jsteinbrunner commented 4 years ago

Hello @jasminb,

The library has 2 deserialization features that allow for missing types in included properties and relationships properties.

ALLOW_UNKNOWN_INCLUSIONS - When set to true, it silently ignores unknown types found in includes ALLOW_UNKNOWN_TYPE_IN_RELATIONSHIP - When set to true, ignores unknown types found in relationships, but only if the class attribute is an interface. This code was added specifically to handle supporting polymorphic relationships and allowing for new implementations to be added in a backwards compatible way.

However, we have run into scenarios in our project where a POST/PATCH to an endpoint has side effects and we return these in the response. In some cases, we may side load an affected resource type that is not understood on the client (if it was recently added).

What are your thoughts on removing the isInterface check at https://github.com/jasminb/jsonapi-converter/blob/develop/src/main/java/com/github/jasminb/jsonapi/ResourceConverter.java#L513 so the behavior is consistent with ALLOW_UNKNOWN_INCLUSIONS?

CC: @Kevinrob

Kevinrob commented 4 years ago

I don't use this library anymore. We have switched to https://github.com/kamikat/moshi-jsonapi. 😉

iamareebjamal commented 4 years ago

@Kevinrob Any library which requires you to extend your model classes must be avoided like plague

jsteinbrunner commented 4 years ago

@jasminb I wanted to follow up to get your thoughts. I'm happy to submit a PR with this change, but wanted to see if you agreed with this behavior before doing so.

Kevinrob commented 4 years ago

@Kevinrob Any library which requires you to extend your model classes must be avoided like plague

Yeah, it's a good point 😀 But jackson library is a pain for Android application (performance, size, etc...).

jasminb commented 4 years ago

@jsteinbrunner Please correct me if I'm wrong, you are saying that you have a case where client encounters completely unknown relationship field and the lib fails to ignore it?

jsteinbrunner commented 4 years ago

@jasminb I'm embarrassed to admit that I may have created this issue when there isn't actually a problem. We identified this workflow, had seen the issue in the past on an earlier version, and looking through the code, I made a bad assumption that it still existed.

I apologize for not doing my due diligence and verifying the problem existed by writing a test against the latest version. I am unable to write a test now that reproduces the issue, and can see in the code why it now works. I will close this issue and if I discover something new in the future, I will let you know.

jasminb commented 4 years ago

@jsteinbrunner no problem at all.