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

JacksonJaxbJsonProvider should use the real "value.getClass()" to build the root type #87

Closed
NicoNes opened this issue Jul 1, 2016 · 9 comments
Milestone

Comments

@NicoNes
Copy link
Contributor

NicoNes commented Jul 1, 2016

Hi,

I saw your comment in the code (JacksonJaxbJsonProvider.writeTo() method) : /* * This is still not exactly right; should root type be * further specialized with 'value.getClass()'? Let's see * how well this works before trying to come up with more * complete solution. */ rootType = writer.getTypeFactory().constructType(genericType); so I suppose you already know the issue.

I have two types :

public abstract class Page<E> implements Iterator<E> {

    public static final String PREV_PAGE_REL = "prev";
    public static final String NEXT_PAGE_REL = "next";

    public final Link getPreviousPageLink() {
        return getLink(PREV_PAGE_REL);
    }

    public final Link getNextPageLink() {
        return getLink(NEXT_PAGE_REL);
    }

    abstract Link getLink(String rel);

}

and

@JsonPropertyOrder({ "entities", "links" })
@JsonAutoDetect(fieldVisibility = Visibility.ANY, creatorVisibility = Visibility.ANY, getterVisibility = Visibility.NONE, isGetterVisibility = Visibility.NONE, setterVisibility = Visibility.NONE)
public class PageImpl<E> extends Page<E> {

    public static final String PAGE_QUERY_PARAM = "page";
    public static final String PER_PAGE_QUERY_PARAM = "perPage";
    public static final String SORT_QUERY_PARAM = "sort";

    private List<E> entities;

    @JsonSerialize(contentUsing = JsonLinkSerializer.class)
    @JsonDeserialize(contentUsing = JsonLinkDeserializer.class)
    private final List<Link> links;

}

My service returns

//Runtime time of page can be either PageImpl or any other sub type I can determine at compile time

Page<Data> page = ...;
return Response.ok(new GenericEntity<Page<Date>>(page) {}).build();

The problem is that the all the annotations I put into PageImpl will not be taken into account for serialization.

Do you already have any fix or workaround ?

Thank you

@cowtowncoder
Copy link
Member

I would just strongly recommend never returning instance of generic type, regardless of whether returning can be made to work. It's just a constant source of problems.
Instead the usual work-around of defining a sub-type that binds parameters (class X extends Page....) is reliably and works as expected.

I don't have current plans to work on trying to improve handling of JAX-RS endpoint.

@NicoNes
Copy link
Contributor Author

NicoNes commented Jul 2, 2016

Ok and before you get serious time to work on the improvment, is there any chance we can get any modification such as following one :

            if (genericType.getClass() != Class.class) {
                rootType = writer.getTypeFactory().constructType(genericType);
                if (rootType.getRawClass() == Object.class) {
                    rootType = null;
                }
            }

would become

            if (genericType.getClass() != Class.class) {
               TypeFactory typeFactory = writer.getTypeFactory();
                rootType = typeFactory.constructType(genericType);
                Class<?> rawClass = rootType.getRawClass();
                if (rawClass == Object.class) {
                    rootType = null;
                } else if (type != rawClass && !rootType.isCollectionLikeType() && !rootType.isMapLikeType()
                        && rootType.hasGenericTypes()) {
                    List<JavaType> typeParameters = rootType.getBindings().getTypeParameters();
                    rootType = typeFactory.constructParametricType(type,
                            typeParameters.toArray(new JavaType[typeParameters.size()]));
                }
            }

It will only affect cases where the generic entity raw type is different from the real entity type and only for user custom generic type. Cases where generic type represents a Collection or Map of any sort will not be affected.

new GenericEntity<Page<Data>>(new PageImpl<Data>()) => Impacted by the change
new GenericEntity<PageImpl<Data>>(new PageImpl<Data>()) => not Impacted
new GenericEntity<List<Data>>(new ArrayList<Data>()) => not Impacted
new GenericEntity<ArrayList<Data>>(new ArrayList<Data>()) => not Impacted
new GenericEntity<Map<Data>>(new HashMap<Data>()) => not Impacted
new GenericEntity<HashMap<Data>>(new HashMap<Data>())=> not Impacted

Am I missing other potential impacts ? Do you think this is acceptable as a temporary solution ?

@cowtowncoder
Copy link
Member

@NicoNes I was about to say that this shouldn't do anything, before I noticed that it actually might, if type and genericType do differ. I don't know if code indicate is necessarily exactly right (wrt resolution), but it might be an improvement, depending on how discrepancy between the two comes about.

Would it be possible to write a simple test case to show how change would solve a problem? I haven't used GenericEntity myself so I am not familiar enough, but it sounds like you are, so a unit test should make it much cleared for me.
And so this just make it in 2.8 if we can this closed soon.

@cowtowncoder
Copy link
Member

@NicoNes I think what I'd like to do is to just proceed with pushing 2.8.0 of providers, for now, and with suitable tests I am open to improving type resolution where it makes sense.

One concern I do have about code suggested is just that it seems wrong for genericType to ever have raw type different from type (without knowing how one could pass GenericEntity); but, if it turns out to be valid and legal usage, then my understanding is incorrect or at least incomplete, and we could proceed with changes to support legitimate usage.

So that's a long way of saying that I don't know expected valid usage (perhaps supported by JAX-RS documentation), and with additional tests I could learn more.

@NicoNes
Copy link
Contributor Author

NicoNes commented Jul 6, 2016

Hi @cowtowncoder,

I just created the PR #89 and added two comments. The first one is about the fact that both jersey and resteasy can't coexist properly at runtime. The second is about a behavior I noticed but I don't understand. Maybe you can help me with it.
About your concern with rawType and type, I would say that they can be different. Hope the test I added will help you understand why and how to do. If you take a look at GenericEntity documentation http://docs.oracle.com/javaee/6/api/javax/ws/rs/core/GenericEntity.html in the given example you'll see that raw type is List<String> and type is ArrayList<String>

@tatu-at-sfdc
Copy link

tatu-at-sfdc commented Jul 7, 2016

Hmmh. Odd -- it does not make sense to me; raw type can not be List<String> for obvious reasons (it's type erased), but I'd expect it be ArrayList.class. Note that javadocs do not imply there would be difference: GenericEntity passed is still List<X>.

Now, types certainly ought to be related (either same type-erased class, or one is sub-class of the other), but even there it is not clear if it should always be that one is subtype of the other.

I'll have a look at PR so I hope it clarifies the issue.

@NicoNes
Copy link
Contributor Author

NicoNes commented Jul 7, 2016

Yes my bad sorry. I switched rawType and type.

To be clearer, if the server side returns:

GenericEntity<List<String>> entity = new GenericEntity<List<String>>(new ArrayList<String>()) {};
Response response = Response.ok(entity).build();

then MessageBodyWriter.writeTo() method will be invoked with following parameters:

  • value: new ArrayList()
  • type: value.getClass() => ArrayList.class
  • genericType: List<String>

In this case typeand genericTypeare different. So following code will return true:

ObjectWriter.getTypeFactory().constructType(genericType).getRawClass() != type

Else If server side returns

GenericEntity<ArrayList<String>> entity = new GenericEntity<ArrayList<String>>(new ArrayList<String>()) {};
Response response = Response.ok(entity).build();

then MessageBodyWriter.writeTo() method will be invoked with following parameters:

  • value: new ArrayList()
  • type: value.getClass() => ArrayList.class
  • genericType: ArrayList<String>

In this case typeand genericType are different. So following code will return false:

ObjectWriter.getTypeFactory().constructType(genericType).getRawClass() != type

Javadoc just mention that instance passed to GenericEntity constructor must be of type T so both of the previous cases are legal.

@NicoNes
Copy link
Contributor Author

NicoNes commented Jul 7, 2016

Sure, I understand you're perspective 👍

@cowtowncoder cowtowncoder added this to the 2.8.1 milestone Jul 12, 2016
@cowtowncoder cowtowncoder changed the title JacksonJaxbJsonProvider should maybe use the real "value.getClass" to build the root type JacksonJaxbJsonProvider should use the real "value.getClass()" to build the root type Jul 12, 2016
@cowtowncoder
Copy link
Member

Fixed via #89

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

No branches or pull requests

3 participants