caelum / vraptor4

A web MVC action-based framework, on top of CDI, for fast and maintainable Java development.
http://vraptor.org
Apache License 2.0
350 stars 332 forks source link

Problem to instantiating only request present parameters #781

Open pvrsouza opened 10 years ago

pvrsouza commented 10 years ago

Based on this example;

http://www.vraptor.org/en/docs/components/#instantiating-only-request-present-parameters

The useNullForMissingParameters only ensures that the parameters are not instantiated (not his compositions)

JSP:
<form action="${pageContext.request.contextPath}/build" method="post">
<div class="control-group">
    <select name="build.buildType.id">
        <option value=""><fmt:message key="selecione" /></option>
      <c:forEach var="buildType" items="${buildTypeList}">
        <option value="${buildType.id}" <c:if test = "${build.buildType.id == buildType.id}">selected="true"</c:if>>  </option>
      </c:forEach>
    </select>
</div>
</form>
Controller
@Post("/build")
public void create(Build build) {
    //the build.getBuildType() here is an instantiated object with null parameters.
}
Model class:
@javax.persistence.Entity
public class Build {

    private static final long serialVersionUID = -7223400640211489832L;

    @ManyToOne
    private BuildType buildType;

    public void setBuildType(TipoImovel buildType) {
        this.buildType = buildType;
    }

    public BuildType getBuildType() {
        return buildType;
    }
}
Turini commented 10 years ago

@pvrsouza I couldn't reproduce it. A simple exemple:

@Path("/")
public void index(Cat cat) {
    System.out.println(cat);
    System.out.println(cat.getFlea()));
}

The Cat class:

public class Cat {
    private String name;
    private Flea flea;
    // getters and setters
}

The Flea class:

public class Flea {
    private String name;
    // getters and setters
}

The Flea is instantiate only when I send the cat.flea param. It's happening with all of your compositions or just BuildType?

renanigt commented 10 years ago

I think the build.buildType.id is always being sent, may be without data, but is sent, so with this the class will ne instantiated.

pvrsouza commented 10 years ago

@renanigt Did you mean "...the class will never instantiated." ?

@Turini In fact, my expectation is:
1 - no build.buildType.id, no instantiate the class BuildType or 2 - all fields null, no instantiate the class BuildType

garcia-jj commented 10 years ago

In your example, what's printed if you print: 1) build.buildType? 2) build?

pvrsouza commented 10 years ago

@garcia-jj 1 - buildType with null fields. 2 - buildType null. Not instantieted.

garcia-jj commented 10 years ago

@pvrsouza I need to know exactly what's printed when you do:

System.out.println(build.buildType);
System.out.println(build);

That means, most important, is to know what's printed with build and build.buildType. Sorry if my first question was not too clear.

renanigt commented 10 years ago

The correct would be "will be instantiate". Sorry for the errors, sometimes I'm on cellphone.

pvrsouza commented 10 years ago

@garcia-jj

1 - System.out.println(build); Build [name=Test, description=Description, meters=1, buildType=BuildType [name=null, description=null, acronym=null]

2 - System.out.println(build.getBuildType()); BuildType [name=null, description=null, acronym=null]

garcia-jj commented 10 years ago

Can you share with us the request parameters and it's values? Firebug or native development tools available in the browser can help you to see all request parameters.

Thank you

pvrsouza commented 10 years ago

@garcia-jj

build.name=Test&build.description=Description&build.meters=1&build.buildType.id=

Turini commented 10 years ago

@pvrsouza if you send the build.buildType.id parameter, VRaptor will always create the BuiltType object, this is the expected behaviour and not a bug. The useNullForMissingParameters just avoid to instantiate parameter objects when it isn't on the request. To avoid instantiate the BuildType you can: (1) do not send it as a request parameter when the values is null/empty. (2) create a converter or something like this to return null when the parameter value is present but is null.

renanigt commented 10 years ago

I agree with @Turini.

pvrsouza commented 10 years ago

Ok,

I agree that would work with the Converter for the BuildType. But I think it would have a possible limitation in a self-relationship

For example:

class Build{
    private Build buildFather;
}
<select name="build.buildFather.id">
  <option value="">Select one build father</option>
  <c:forEach var="buildFather" items="${fatherList}">
        <option value="${buildFather.id}" >${buildFather.nome} </option>
  </c:forEach>
</select>

The request parameters:

build.name=Teste&build.description=teste&build.meters=1&build.buildType.id=&build.buildFather.id=

In this case I am sending the request without build.id and build.buildFather.id. When submitting this form the converter will be confused.

Any suggestions?

garcia-jj commented 10 years ago

IMHO if the target parameter is null (or something than represents a null value like empty string to long) we can't instantiate the parent object.

In the case exposed by @pvrsouza, if user don't select anything in the select element, the parameter build.buildType.id will sent as empty value. The target property is Long, and empty String is null Long. So in this case we can't instantiate parent object build.buildType because target is null.

Ok, if the type of build.buildType.id was a String we can instantiate the parent object because empty String is a valid value for a String, but not for a Long.

I don't know if I'm right, but this is my opinion. We should only instantiate if target isn't null or similar values.

Turini commented 10 years ago

well, I'd prefer the current behaviour. If there is a parameter for this type (even empty or null) VRaptor instantiate the parent object. I think that if the param key was sent on the request and its null/empty, makes total sense VRaptor using the default value on it. And to do that the parent object should be instantiated (IMHO)

renanigt commented 10 years ago

I agree with @Turini.

garcia-jj commented 10 years ago

@Turini Even useNullForMissingParameters is setted to true?

If our final decision will to keep current behavior, we can relax IogiParametersProvider.parseParameters to allow users to bypass null/empty parameters. Or its too fancy my idea?

garcia-jj commented 10 years ago

I like to invite @asouza, @rponte, @csokol and @lucascs to share their opinion with us.

Turini commented 10 years ago

@Turini Even useNullForMissingParameters is setted to true?

I think so... because this is not about missed parameters. The point here is that the parameter is present but its value is eq null or empty.

lucascs commented 10 years ago

I did once a code that filters out all the request parameters that ends with .id and are empty.

https://gist.github.com/lucascs/2855418

We could add this to VRaptor, or just make the useNullForMissingParamters filter out these cases.

renanigt commented 10 years ago

In the specific case when parameters ends with .id, maybe it makes sense, but in others cases I agree with @Turini.

garcia-jj commented 10 years ago

@lucascs My proposal is do something like your code, but not with fixed ".id", but when the target is not String (like Long, Integer and so on), because there are some cases that our id is not id, but code, código, cliente_id and others...

lucascs commented 10 years ago

We could add something to IOGI, like removeNullTargets, traversing the object graph, or the Parameters, and removing nulls. Not sure how complex it would be...

Turini commented 10 years ago

I think it can be very useful for some users, but I'd not assume this as the default behavior. We can simplify the process to specialize it and add a "how to" on docs.

garcia-jj commented 10 years ago

I agree @Turini and @lucascs. Not by default, but can be enabled if users want. My applications isn't affected by the current behavior because I use dtos to get data from view. But will help users that exposed our entities to the view.

So my question is: how to implements? What's the target release? I don't know IOGI too hard to implement this right now.

pvrsouza commented 9 years ago

@garcia-jj can you show an example using dtos to get data from view?