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

Creating WithoutRoot annotation to force JSON deserialization without root. #713

Closed

Conversation

renanigt
Copy link
Contributor

Related: #657.

As I said there, I think the best way is to create another annotation, because many user use something like @Consumes("application/json"), so if we add another attribute as @jayrmotta said, it doesn't work.

What do you think @lucascs @garcia-jj @Turini ?

@renanigt renanigt changed the title Adding WithoutRoot annotation to force JSON deserialization without root. Creating WithoutRoot annotation to force JSON deserialization without root. Jul 29, 2014
@renanigt
Copy link
Contributor Author

I used the annotation aproach suggested by @lucascs.

* the License.
*/

package br.com.caelum.vraptor;
Copy link
Member

Choose a reason for hiding this comment

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

Move to serialization's package please

@Turini
Copy link
Member

Turini commented Jul 29, 2014

Please add docs at VRaptor's site.

@Turini
Copy link
Member

Turini commented Jul 29, 2014

Add the docs here, in the same PR. it will be merged after 4.1.0.Final release. Ok?

@renanigt
Copy link
Contributor Author

Ok @Turini.
I'll do this.

Thanks.

@renanigt
Copy link
Contributor Author

Done with refactoring.

What do you think ? @Turini

Today I'll write docs.

@renanigt
Copy link
Contributor Author

I did a simple text on docs.

What do you think ?

@garcia-jj
Copy link
Member

What you think to use Consumes.withoutRoot(true/false) instead a new annotation?

@renanigt
Copy link
Contributor Author

If I understand you're saying to use @Consumes annotation.

Is it ?

Sorry if I'm wrong.

@renanigt
Copy link
Contributor Author

@garcia-jj sorry, but I think that I didn't understand your suggestion. 😒

@garcia-jj
Copy link
Member

Instead of using a new annotation, add a field into Consumes annotation
called withoutRoot using false as default value.

And change your code to check this new field to use or not root element.

@renanigt
Copy link
Contributor Author

Adding another field, we'll don't have problem with something like this: @Consumes("application/json") ?

@garcia-jj
Copy link
Member

Nope. Try it.
On Jul 29, 2014 1:37 PM, "Renan Montenegro" notifications@github.com
wrote:

Adding another field, we'll don't have problem with something like this:
@consumes("application/json") ?


Reply to this email directly or view it on GitHub
#713 (comment).

@renanigt
Copy link
Contributor Author

The value passed into @Consumes as in @Consumes("application/json") is just to value attribute ?

@garcia-jj
Copy link
Member

Yes. You can use like this:

@consumes("blah") is equal to @consumes(value="blah")
On Jul 29, 2014 1:47 PM, "Renan Montenegro" notifications@github.com
wrote:

The value passed into @consumes as @consumes("application/json") is just
to value attribute ?


Reply to this email directly or view it on GitHub
#713 (comment).

@renanigt
Copy link
Contributor Author

Good idea @garcia-jj. Thanks.

What do you think now ?

~~~
#!java
@Consumes(value="application/json", withoutRoot=true)
@WithoutRoot
Copy link
Member

Choose a reason for hiding this comment

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

@WithoutRoot don't exists anymore.

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

It's probably ugly, but we could have something like:

@Consumes(options=WithoutRoot.class)

and options is Class<? extends DeserializerConfig>[]

and:

interface DeserializerConfig {
    void config(Deserializer deserializer);
}

or something like this.

Sorry to get at this discussion too late =/

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

This solution supports things like:

class NoPassword implements DeserializerConfig {
    public void config(Deserializer deserializer) {
       deserializer.exclude("password");
    }
}

//...
@Consumes(value="application/json", options={WithoutRoot.class, NoPassword.class})
public void update(User user) {...}

@renanigt
Copy link
Contributor Author

The Deserializer interface just has the deserialize method.

The exclude method is from Serializer interface.

@renanigt
Copy link
Contributor Author

But it's a good idea.

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

The point is to implement this extension ;)

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

Any suggestions, @Turini @garcia-jj ? we could use Serializee, or Deserializee, or any other thing like this.

@renanigt
Copy link
Contributor Author

I really liked the idea. 😃

@renanigt
Copy link
Contributor Author

With your help, I can implement this. @lucascs @Turini @garcia-jj 😃

@Turini
Copy link
Member

Turini commented Jul 29, 2014

I like the idea! It provides much more flexibility when using consumes.
But maybe instead of:

@Consumes(value="application/json", options={WithoutRoot.class, NoPassword.class})
public void update(User user) {...}

we could do something like this:

@Consumes(value="application/json")
@With(options={WithoutRoot.class, NoPassword.class})
public void update(User user) {...}

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

@With is a very generic name...

what is the advantage of creating another annotation, @Turini ?

@renanigt
Copy link
Contributor Author

What do you prefer, I try implement it with your help or you do ?

@Turini
Copy link
Member

Turini commented Jul 29, 2014

I know, With was just an example :) the point is the new annotation.
I think using the same annotation is kinda ugly, if we need 3 options
it will require a break line or an really big line, just like this:

@Consumes(value="application/json", options={WithoutRoot.class, NoPassword.class,
 AnotherElegantOption.class})

I'd prefer my code like this:

@Consumes(value="application/json")
@Xpto({WithoutRoot.class, NoPassword.class, AnotherElegantOption.class})

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

It would be only one option most of time, so:

@Consumes(value="application/json", options=WithoutRoot.class)

is small enough ;)

and we could break the line this way:

@Consumes(value="application/json", 
          options={WithoutRoot.class, NoPassword.class,AnotherElegantOption.class})

@Turini
Copy link
Member

Turini commented Jul 29, 2014

Ok for me, there is no technical reason to create one more annotation anyway ;)

Sent from my iPhone

On 29/07/2014, at 17:55, Lucas Cavalcanti notifications@github.com wrote:

It would be only one option most of time, so:

@consumes(value="application/json", options=WithoutRoot.class)
is small enough ;)

and we could break the line this way:

@consumes(value="application/json",
options={WithoutRoot.class, NoPassword.class,AnotherElegantOption.class})

Reply to this email directly or view it on GitHub.

@renanigt
Copy link
Contributor Author

About the parameter of config method, the right way would be create an interface or increase the Deserializer interface ?

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

@garcia-jj
Copy link
Member

I agree too.
On Jul 29, 2014 6:30 PM, "Rodrigo Turini" notifications@github.com wrote:

Ok for me, there is no technical reason to create one more annotation
anyway ;)

Sent from my iPhone

On 29/07/2014, at 17:55, Lucas Cavalcanti notifications@github.com
wrote:

It would be only one option most of time, so:

@consumes(value="application/json", options=WithoutRoot.class)
is small enough ;)

and we could break the line this way:

@consumes(value="application/json",
options={WithoutRoot.class,
NoPassword.class,AnotherElegantOption.class})

Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#713 (comment).

@renanigt
Copy link
Contributor Author

I think I understood. So, does we need to create a class, maybe Deserializee like Serializee ?

@renanigt
Copy link
Contributor Author

If do you want, I can begin the implementation and growing up the feature with your help. Or do you prefer implement it ?

What do you think ?

@renanigt
Copy link
Contributor Author

If WithoutRoot has a configuration to force without root and WithRoot a configuration to force with root, what's the priority ?

@Consumes(value="application/json", options={WithoutRoot.class, WithRoot.class})
public void update(User user) {...}

@Turini
Copy link
Member

Turini commented Jul 29, 2014

I think that is better close this PR and open a new one. Ok?

Sent from my iPhone

On 29/07/2014, at 19:29, Renan Montenegro notifications@github.com wrote:

If do you want, I can begin the implementation and growing up with your help. Or do you prefer implement it ?

What do you think ?


Reply to this email directly or view it on GitHub.

@renanigt
Copy link
Contributor Author

Ok.

Do I begin the other or do you do ?

@lucascs
Copy link
Member

lucascs commented Jul 29, 2014

Please begin, @renanigt

Start with a Deserializee with just setWithRoot(boolean), WithoutRoot.class and WithRoot.class setting it as false and true and the existing deserializer code to start the Deserializee with the default as it is today.

@renanigt renanigt closed this Jul 30, 2014
@renanigt renanigt deleted the improvingDeserializationWithoutRoot branch July 30, 2014 01:38
@renanigt
Copy link
Contributor Author

@lucascs today we doesn't have Deserializee class, so I don't understood when you said "start the Deserializee with the default as it is today.".

Thanks for the support.

@renanigt
Copy link
Contributor Author

When you said that, mean to start Deserializer with default behavior used today ?

@lucascs
Copy link
Member

lucascs commented Jul 30, 2014

I mean that you use the existing logic to decide if there is a root or not to set the initial value of Deserializee#withRoot, and than pass it to the options.

@renanigt
Copy link
Contributor Author

Oh, now I understand. Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants