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

Implementation in gson deserialization to able deserialize generics parameters #553

Merged
merged 11 commits into from
Aug 3, 2013

Conversation

dtelaroli
Copy link
Contributor

The GsonDeserialization not is able to deserialize generics parameters like this:

class GenericCtrl {
void method(T type)
}
class ExtendedCtrl extends GenericCtrl{
}

edit: removed the old description

@lucascs
Copy link
Member

lucascs commented Jul 31, 2013

It's a little too specific... We don't need to create a new annotation for this... It is possible to discover the generic type if you have a subclass:

public class DogController extends GenericController<Dog> {...}

Class<?> dogClass = DogController.class.getGenericSuperclass().getGenericTypeArguments[0];

@dtelaroli
Copy link
Contributor Author

Ok, I change to keep simple after your recommendation.
I don't think that generics is too specific, but I'm learning that generics is the cause of many problems in our software design.

Now it is able to deserialize generics, if method type is equal to generics types.

@@ -91,6 +91,19 @@ else if(node != null){
return params;
}

protected Class<?>[] getTypes(ResourceMethod method) {
Type superclass = method.getResource().getType().getGenericSuperclass();
if(!superclass.equals(Object.class)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right test...

you should do something like if (superclass instanceof ParameterizedType)

@dtelaroli
Copy link
Contributor Author

Done

for (int i = 0; i < genericsTypes.length; i++) {
classes[i] = (Class<?>) genericsTypes[i];
}
return classes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to point it out: It won't cover the case when the generic controller method receives more than one argument:

public class GenericController<T> {
    public void save(T obj, String something) {...}
}

@dtelaroli
Copy link
Contributor Author

the deserializer will deserialize only the first ParameterizedType, so, will be possible add arguments after the generic type if the GenericController has more than one.
are you agree?

@lucascs
Copy link
Member

lucascs commented Aug 2, 2013

Not really...
the GSon deserializer supports deserializing multiple parameters:

{ "user": { "name": "jao", "email": "[email protected]" },
  "something" : "else" }

@dtelaroli
Copy link
Contributor Author

in the last commit is require that it everytime make overlay of the first parameter, forming a convention.

I think that we can to create a boolean property in @consumes annotation, with default false, to enable this feature, just when to need.
what do you think?

the name property could be same enableGenericsOverlay.

@lucascs
Copy link
Member

lucascs commented Aug 3, 2013

As far as I know, if you don't have a root, it tries do deserialize to the first argument. But if you have a root, with the names of the parameters, you can deserialize as many properties as you want.

I don't like changing the @Consumes annotation because of generics... I don't like using generic controllers either ;)

The point is, even supporting multiple parameters and the Generic parameter, it's possible to know which parameter is the generic one. If I'm not wrong:

Type[] parameterTypes = method.getGenericParameterTypes();

for (Type type : parameterTypes) {
   if (type instanceof TypeVariable)
      // use the controller generic type
   else
      // use type
}

@dtelaroli
Copy link
Contributor Author

We need compare parameter with TypeArgumet

if(parameterTypes[i].isAssignableFrom(type.getClass())) {
    parameterTypes[i] = (Class<?>) type;
}

to me is working fine
can you check?

@lucascs
Copy link
Member

lucascs commented Aug 3, 2013

Write a test for it =) If the tests are green It works =)

}
}
}
return parameterTypes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd extract the generic extraction to a method:

Class<?>[] parameterTypes = method.getMethod().getParameterTypes();
Type genericType = getGenericType(method.getResource().getType);
if (genericType != null) {
    for ...
}
return parameterTypes;

@dtelaroli
Copy link
Contributor Author

the tests are
shouldDeserializeFromGenericTypeOneParam
shouldDeserializeFromGenericTypeTwoParams
shouldDeserializeWithoutGenericType

@@ -53,8 +53,8 @@ public GsonDeserialization(ParameterNameProvider paramNameProvider, Collection<J
}

public Object[] deserialize(InputStream inputStream, ResourceMethod method) {
Method jMethod = method.getMethod();
Class<?>[] types = jMethod.getParameterTypes();
Method jMethod = getJMethod(method);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method.getMethod() is already clear. You don't need to extract this method. Please get back to method.getMethod()

@lucascs
Copy link
Member

lucascs commented Aug 3, 2013

I hope you are not getting upset, coding with reflection is always messy :P

@dtelaroli
Copy link
Contributor Author

relax man, I just did not understand
me too want the better code, but I have much to learn

@dtelaroli
Copy link
Contributor Author

verify please

@lucascs lucascs merged commit e2ba470 into caelum:master Aug 3, 2013
@lucascs
Copy link
Member

lucascs commented Aug 3, 2013

Merged, thanks! =)

@dtelaroli
Copy link
Contributor Author

thank you too

@lucascs
Copy link
Member

lucascs commented Aug 3, 2013

In fact, the line parameterTypes[i].isAssignableFrom(genericType) was working because parameterTypes[i] is Object.class for the generic type, so it is assignable from all the classes... I'm fixing it =/

@lucascs
Copy link
Member

lucascs commented Aug 3, 2013

Take a look: 9c5a0a8

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 this pull request may close these issues.

2 participants