Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

linkTo support to overloaded methods #478

Merged
merged 2 commits into from Nov 14, 2012

Conversation

Projects
None yet
2 participants
Contributor

nykolaslima commented Nov 13, 2012

The problemn is related here: http://www.guj.com.br/java/281823-vraptor-linkto-com-dois-metodos-de-mesmo-nome-no-controller-bug

@lucascs, the code works but when we have a method as below it breaks:
Controller

@Post("/method/{entity.id}")
public void method(SomeModel someModel)

JSP

${linkTo[SubGenericController].method[entityId]}

This breaks because linkToHandler tries to find a method named "method" with an Integer parameter. The parameter's type verification is necessary to differentiate the overloaded methods.

How can we deal with it?
When parameter's type dont match can we return the method with same parameter's number but with no match types or the first matching method?

Maybe you could get most of those methods from Guava or Mirror, will you give a try? =)

Owner

nykolaslima replied Nov 14, 2012

I created this methods because I need to deal with primitive and wrappers types.
Does Guava or Mirror already deal with it?

There is any test that matches the method with 2 args?

Owner

nykolaslima replied Nov 14, 2012

Yes. The test "shouldReturnWantedUrlWithParamArgs".

Member

lucascs commented Nov 14, 2012

JSP


${linkTo[SubGenericController].method[entityId]}
This breaks because linkToHandler tries to find a method named "method" with an Integer parameter. The parameter's type verification is necessary to differentiate the overloaded methods.

How can we deal with it?

If we ignore the overloads, we could assume that entityId will be placed at the first {...} on the path. What do you think?

When parameter's type dont match can we return the method with same parameter's number but with no match types or the first matching method?

We could trigger this overload logic only when there are two or more methods with the same name. So when it is an overload, the parameter types must match or an error should be thrown.

What do you think?

Contributor

nykolaslima commented Nov 14, 2012

@lucascs, I didnt understand the first comment, if we ignore the overloads then we should ignore this pull request, shouldnt we?

I agree with the second comment, if there is overloaded methods then parameter's type must match. Otherwise, only method name must match.

Member

lucascs commented Nov 14, 2012

My point is, if the method is not overloaded, then we could assume that given parameter will go on the template part of the url.

ex:
${linkTo[Controller].method[entity]} should call entity.getId() to replace
${linkTo[Controller].method[id]} should only replace the first path param

Contributor

nykolaslima commented Nov 14, 2012

@lucascs I did the changes. Can you take a look?

Owner

nykolaslima replied Nov 14, 2012

No more.
The code below understands primitive and wrapper types.

new Mirror().on(controller).reflect().method(methodName).withArgs(getClasses(args));

you could change these for's with:

//Reflecting all methods of class that matches a Matcher<Method> (will return an empty list if none found):
Class clazz;
List<Method> l = new Mirror().on(clazz).reflectAll()
                               .methods().matching(new YourCustomMatcher());

so you create a matcher new NonBridgeNamed(methodName) and you can use it here and on the amount method

Owner

nykolaslima replied Nov 14, 2012

I did this.
But the matcher was returning the bridge method even when I tell to ignore it.
And when I see the returned methods, the isBridge() return false for bridge method.

even if you write a matchers filtering bridge methods? a bridge method have the exact name as the normal one... maybe its a bug on mirror also...

Owner

nykolaslima replied Nov 14, 2012

Yes, the problem occurs even when I filter bridge methods.

=(

I'm merging this pull request, then... Maybe we refactor this afterwards.

Owner

nykolaslima replied Nov 16, 2012

Ok.
Latter I will take a look at Matcher with bridge methods.

lucascs added a commit that referenced this pull request Nov 14, 2012

@lucascs lucascs merged commit e13f72a into caelum:master Nov 14, 2012

Member

lucascs commented Nov 14, 2012

Thanks!!

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