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

Vraptor instantiate even has no request parameters for object #532

Closed
ghost opened this issue May 6, 2014 · 14 comments · Fixed by #660
Closed

Vraptor instantiate even has no request parameters for object #532

ghost opened this issue May 6, 2014 · 14 comments · Fixed by #660
Assignees
Labels

Comments

@ghost
Copy link

ghost commented May 6, 2014

@Post
public void adiciona(Pessoa pessoa, Carro carro){
    System.out.println(pessoa.getNome()); // Print the name correctly
    System.out.println(carro); // Print a empty instance . Must be null
    this.result.redirectTo(this).form();
}
<form action="url" method="post">
    <Nome:  <input type="text" name="pessoa.nome"/>
    <input type="submit" value="Salvar"/>
</form>

In this snippet vraptor instantiate carro , but has no carro.xxx in the request parameters . In my point of view carro must be null not a empty instance . Another point is that break compatibility with ognl that was removed in favor of iogi in version 4 (that instantiate correctly in vraptor 3 with ognl putting null if has no request parameters) .

@Turini
Copy link
Member

Turini commented May 6, 2014

I agree that it should not instantiate... Can anyone see a downside changing it?

@garcia-jj
Copy link
Member

I agree. There is the same issue opened in vraptor3.

@matheusmessora
Copy link

This change can affect developers who already use it. It now can generate a NullPointerException.
Must be well documented in the tutorials to explain the change.

@Turini Turini added the bug label May 27, 2014
@Turini
Copy link
Member

Turini commented Jun 3, 2014

just linking caelum/vraptor#525

@garcia-jj
Copy link
Member

I did some tests, and this behavior (I consider behavior, not a bug) occurs only with mutable objects. Take a look in my code:

@Path("/test")
public void test(PlanDTO plan, UserDTO user) {
    System.out.println(plan);
    System.out.println(user);
    result.use(Results.status()).forbidden("true");
}
public class PlanDTO {
    private final Long id;

    public PlanDTO(Long id) { 
        this.id = id;
    }

    public Long getId() {
        return id;
    }
}

And this is the logs for some requests

With /test
[stdout] (default task-11) null
[stdout] (default task-11) null

With /test?plan
[stdout] (default task-12) null
[stdout] (default task-12) null

With /test?plan.id
[stdout] (default task-14) PlanDTO[id=null]
[stdout] (default task-14) null

With /test?plan.id=1
[stdout] (default task-15) PlanDTO[id=1]
[stdout] (default task-15) null

But if I change my class PlanDTO to have mutability (removing final, constructors and add a setter for id property), the class will be always instantiated in all requests like described above.

I don't know what you think but, IMHO, we can leave this behavior as is, and add a note in the docs describing that only immutable objects won't be instantiated. I have this idea because but it's too dangerous to change this behavior by now. Final release already released, so many apps expects non-null instance as parameters. And as @matheusmessora told, we can create a NullPointerException for users who already uses this behavior.

@ghost
Copy link
Author

ghost commented Jul 1, 2014

Others point of view about this issue :

1 . Ognl was introduced before iogi and keep going on in vraptor since was removed as a dependencie in vraptor 4 . So i think (my opinion) the priority is for him behavior (ognl) for a great reason too (migration to version 4 with compatibility) unless that is not default behaviour anymore in version 4 and who use ognl has to rewrite all the application to be compatible with iogi (in general the same reason you defend not change for iogi (who used iogi has to review your application)) . We will never know who is using what for migration (iogi or ognl have diferents behavior for this scenario) . So that argument is kind of invalid for me since one of the worlds will be affected .

2 . For me again there is no sense vraptor instantiate an object as an "empty instance" if there`s no parameter to populate (independent if a class is mutable or not) null is more sensate in this case (no parameter in request , no object) .

3 . An another ideia is a way to dev choose what kind of behaviour he wants (to not break compatibility with anyone (iogi world and ognl world)) overwriting some class for example .

@lucascs
Copy link
Member

lucascs commented Jul 2, 2014

VRaptor 4 is not meant to be fully compatible with VRaptor 3, and as we dropped OGNL from it, there is no need to support it. So point 1 doesn't hold, in my opinion.

I agree with point 2, but it's a compatibility break. We could argue that it's a bug, but I'd only release it on version 4.1, and make a big note on the release explaining this break change.

3 - I think it doesn't worth the complexity. But maybe a class with a protected method that could be overridden by a @Specializes component

@Turini
Copy link
Member

Turini commented Jul 2, 2014

I'd prefer the 3rd option... like Otávio said, it's a behavior and not necessarily a bug. Some
users would like to receive a null safe parameter instead of null (but this is not my case... I
prefer to get a null param in this case, for example). So it'd be nice to let the user choose.

@garcia-jj
Copy link
Member

I like 3rd option too.

@Turini
Copy link
Member

Turini commented Jul 2, 2014

Can I implement it? Using an environment property to choose

@garcia-jj
Copy link
Member

Environment isn't good because this case is not related to environment. I
vote in Lucas comment, a protected method that users can override.

@Turini
Copy link
Member

Turini commented Jul 2, 2014

Ops, it's true. A protected method looks better

@garcia-jj
Copy link
Member

:shipit:

@lucascs
Copy link
Member

lucascs commented Jul 3, 2014

=)

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

Successfully merging a pull request may close this issue.

4 participants