fixing problem with null headers

[Turini]

Closes #595.

[garcia-jj]

Header have more priority than request parameters. With this code if header
is null, vraptor will use request parameter. But the expected behavior is
get null.

[lucascs]

@garcia-jj it makes sense, but should we do a considerably much more complex code to support it?

it seems like an unlikely corner case

[garcia-jj]

@lucascs Why IOGI don't accept null values? The exception is: br.com.caelum.iogi.parameters.Parameter.notNull(Parameter.java:24)

Adding a NullDecorator to RequestAttributeInstantiator solves this case?

[garcia-jj]

If not, we can change the docs explaining that header only override request parameters if is not null.

[lucascs]

null is the billion dollar mistake. It's evil.
The code is almost always clearer if you don't have to think if something is null or not.

I wouldn't add the docs, because it will only confuse the user. I don't think there is a fair use case when we'd use a @HeaderParam to override a request (query) param.

Anyway, we could (perhaps should) implement the @HeaderParam in such a way that you simply can't set it via request (query) params.

something like

parameterInstantiator
   - get the path params and set on request
   - call iogi to use the params and create the objects
   - override all @HeaderParam's to the header content, even if it's nil

what do you think?

[garcia-jj]

I agree that nullis too evil, but we need to represent when we don't have a value. We can't use Optional yet.

But, for me, I like your suggestion. +1

[Turini]

@lucascs and @garcia-jj, I can changed ParameterInstantiator from:

for (int i = 0; i < valuedParameters.length; i++) {
    valuedParameters[i].setValue(values[i]);
}

to something like that:

for (int i = 0; i < valuedParameters.length; i++) {
    Parameter parameter = valuedParameters[i].getParameter();
    boolean hasHeaderParam = parameter.isAnnotationPresent(HeaderParam.class);
    if (!hasHeaderParam && headerValues.contains(valuedParameters[i].getName())) {
        continue;
    }
    valuedParameters[i].setValue(values[i]);
}

and it will works (I've tested). But the code seems really ugly. Ideas?

[lucascs]

I guess the correct way would be something like:

for (int i = 0; i < valuedParameters.length; i++) {
    Parameter parameter = valuedParameters[i].getParameter();
    boolean hasHeaderParam = 
    if (parameter.isAnnotationPresent(HeaderParam.class)) {
        valuedParameters[i].setValue(request.getHeader(<...>));
    } else {
      valuedParameters[i].setValue(values[i]);
   }
}

maybe we need to use converters before setting the value, in this case.

[garcia-jj]

I don't have an opinion about this change. @lucascs idea sounds good for me. But if we need to call converters, so I think that we only need to override only if header is not null.

[lucascs]

If the header is null, we have to set null (or call the converter to maybe provide a good null object)

[garcia-jj]

I see your point. Fair enough.

[Turini]

I've updated the code with Lucas' suggestions. What do you think?
I don't think we should to use converters, this code is working as well

[lucascs]

This way you only support String headers...

but there are other types of headers, like "If-Modified-Since" (date-time), "Location" (URI), Content-Length (long), If-None-Match (list of e-tags, which could be a value object).

Converters would help with this.

[garcia-jj]

Ready to me supporting only strings by now.

[Turini]

I agree. What do you think @lucascs?
Can we merge and open an issue for other header types?

[lucascs]

It's kinda a breaking change if you only support Strings...

but I don't know how many people use this feature.

[Turini]

what about to merge it and open an issue?
if some user ask for it we can prioritize the feature in the next release.

[renanigt]

I think it's a good idea by now.

When is the next release of VRaptor final version ?

[Turini]

really soon :)

[renanigt]

Good. :)

[garcia-jj]

I agree with you. Merge and wait users ask for object support.

[Turini]

agreed, merging it.