Fixing problem overriding extractControllerNameFrom method from RoutesParser.

[renanigt]

Related to #673 and #676.

The problem occurs because when a method has @Path annotation, the extractControllerNameFrom method from PathAnnotationRoutesParser class isn't called, so the extractControllerNameFrom method overridden by the subclass isn't called either.

I think I can did an ignorant solution, but maybe an idea.

I changed some asserts of the initial test from @dtelaroli based on PathAnnotationRoutesParserTest.

[renanigt]

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

[garcia-jj]

Hmm, thank you to try fix this case too soon :)

But I think that this is not the solution. If I'm not wrong, your code will always call extractControllerNameFrom. But as @lucascs suggested, the possible fix is something different: #673 (comment)

Because some reason, @dtelaroli needs a point to override this behavior. So we need to change in another place. Sorry because I could not see this code yet (too short time), so may @lucascs or @Turini can help us, or fix my think if I'm wrong.

[garcia-jj]

Please, don't close this pull request yet. Wait a more comments, because I can be wrong.

[renanigt]

Thanks @garcia-jj for yout feedback.

Really, I always call the extractControllerNameFrom. When I call the extractControllerNameFrom method is called the overridden method and returned the wanted behavior, so I add to the uris.

Thanks again.

[dtelaroli]

This issue is required to implement this issue caelum/vraptor-i18n#22

[renanigt]

As I said, is an ignorant solution, but maybe an way to think better about it... Or no... rsrs
😄

[dtelaroli]

I think that the original implementation workflow it's strange.
Returns empty, fix empty, returns paths, fix paths.

The method workflow:
Explicity: getURIsFor -> getUris -> fixURIs -> extractPrefix
Convention: getURIsFor -> getUris -> defaultUriFor -> extractControllerNameFrom -> extractPrefix

Maybe it's the moment to refactor with strategies for all cases.

A question: When used the relative method @path should return controller name without controller @path? I think it would be interesting, otherwise the relative path do nothing different of the absolute path.

[renanigt]

When you use @Path but the controller isn't annotated with @Path, the controller name is returned too, but using the extractPrefix method.

[dtelaroli]

@renanigt In my tests it just returned the path method value, as you said here or I understanded wrong?

[renanigt]

Really, I was wrong. It's just returned if method isn't annotated with @Path.

[dtelaroli]

And how about the relative path?

[renanigt]

What do you mean with relative path ?

[dtelaroli]

@path("myURI")

If is relative should resolved to: /controller/myURI
Actualy the component just add / : /myURI

[renanigt]

It's the default VRaptor behavior...

But if you override the extractControllerNameFrom, should work as you defined in your overridden extractControllerNameFrom method and not the default behavior.

[dtelaroli]

Ok. So it's does not relative, just fix wrong path.

[renanigt]

Yes, this PR just fix the wrong path when the extractControllerNameFrom method is overridden.

[dtelaroli]

👌

[renanigt]

But as discussed with @garcia-jj, probably isn't the correct solution...

I'll think in other way.

[dtelaroli]

I understood.

My question was to clarification.

[renanigt]

I added a verification if the class has PathAnnotationRoutesParser class as a superclass and has extractControllerNameFrom method as declared.

So, when a method has @Path annotation, just in case above, the extractControllerNameFrom method will be called.

[renanigt]

What do you think about something like this ? @garcia-jj @Turini @lucascs @dtelaroli
Or it's really very ugly ? rsrs 😄

[garcia-jj]

I think that this pull request is wrong. Let me explain the behavior regarding routes.

If you have only @Controller annotation, vraptor will use convention. So the URL will produced from classname.methodname.

But you can use annotations like @Path and @Get/Post. If you use any these annotation in the class or method, convention will be ignored. Remember that @Get("/foo") is only a shortcut for @Path("/foo") @Get. In the @dtelaroli scenario he have annotation in the method but not in the class. Since convention needs to be complete ignored, you can't call the method extractControllerNameFrom.

We can't append controller name if @Path or any other shortcut is used.

If I'm no wrong, the real problem is that @dtelaroli can't override this behaviour because our implementation have some bug. So we need to find this bug.

Please, let me know if I'm going nuts :)

[garcia-jj]

@dtelaroli Did you wrote a component that override this behavior, right? And you told to us that doesn't work. Can you share this component with us? Because I want to do some tests here. Thank you

[renanigt]

When has annotation in the method but not in the class, the convention is ignored, but we need call the extractControllerNameFrom from the subclass because this, I passed null as parameter and in the method if the parameter is null an empty String is returned calling the overridden method.

[garcia-jj]

Hmm, now I see your point, makes sense. Let me wait for Lucas and Turini
suggestions.

[renanigt]

I just call extractControllerNameFrom method if the class that is calling it's a subclass of PathAnnotationRoutesParser and if it has extractControllerNameFrom as declared method. 😃

[renanigt]

As you can see here: https://github.com/renanigt/vraptor4/blob/fixingOverridingRoutesParser/vraptor-core/src/main/java/br/com/caelum/vraptor/http/route/PathAnnotationRoutesParser.java#L259

Thanks for the feedback @garcia-jj

[dtelaroli]

As I said, I don't think that is a bug, but I need a way to change the final URI for all actions, in addition to the component is in production a long time, is unsafe change it.

Indeed, I agree that the extractControllerNameFrom is not the proper way. I found other way to solve my issue, overriding the DefaultRouter#builderFor adding the prefix, because I need only add a prefix.

Otherwise, I think that the any component of routes should provide easy customization, and the PathAnnotationRoutesParser design is not helping.
I believe that the component should be able to easily override the default controller name, the path, the parameters and the final url, even if each party returned empty or no value, so you could easily override the method returning another value, being the end of the internal component would return the concatenation of these parts. Other way would be a method that receive and return a class with this properties, making it easier, for example, to translate the uris.

Eg.:

class RouteParts {
   String prefix = "";
   String controller = "";
   String method = "";
   String parameters = "";
   String sufix = "";
   String finalUri() {
     return prefix + controller + method + parameters + sufix;
   }
}

These are just suggestions. Agree you that needs improve? Otherwise I think that we can close these issues.

[renanigt]

I think it's a bug. We just need to fix it. This PR solve the problem if accepted, but if not, we still need fix by another way. In my opinion of course.

[renanigt]

Sorry for the errors, I'm on cellphone.

[dtelaroli]

I'm waiting...

As I said, I need for it to solve other PR.

[renanigt]

Let's wait for @lucascs @Turini @garcia-jj suggestions. 😃

[lucascs]

This is the expected extension:

@Specializes @ApplicationScoped
public class PrefixedRoutesParser extends PathAnnotationRoutesParser {
    //delegate constructor

   @Override
   protected String[] getURIsFor(Method javaMethod, Class<?> type) {
        String[] uris = super.getURIsFor(javaMethod, type);
        for (int i = 0; i < uris.length; i++) {
             uris[i] = "/prefix" + uris[i];
        }
        return uris;
   }
}

no need for extra code on VRaptor. You can also make the prefix depend on the controller and the method the way you like.

This method works both for annotated and convention URIs.

This and #676 should be closed. Does it make sense?

Perhaps we should add a cookbook about it.

[renanigt]

Makes total sense @lucascs. Can I close ?

[lucascs]

Yes, please

[renanigt]

Closing.

[renanigt]

Thanks @lucascs