GRAILS-11333: Handle already absolute URLs

[candrews]

When a "uri" is provided to the link method and the "absolute" parameter is true, check if the provided uri is already absolute before trying to make it absolute.

Note that this pull request is fixed and doesn't have any embarrassingly bad problems like /pull/488 had. I ran "gradlew test" in "grails-web" and all tests pass.

[jeffscottbrown]

Are there any tests for this change?

[candrews]

@jeffbrown I just updated this pull request with some tests (it had tests before, but not specifically covering the change I made). Thank you for taking a look.

[candrews]

@jeffbrown @graemerocher @lhotari can one of you nice folks please merge this? :-)

[graemerocher]

@candrews I'm not sure why this pull request is necessary, it adds overhead to the link tag as even if an exception is not thrown the URI has to be parsed which is expensive.

In addition, we already have a 'url' attribute, which can be used to specify an absolute link. i.e.

 <g:link url="http://grails.org">foo</g:link>

[candrews]

@graemerocher I think this is necessary as the current behavior is surprising. A bunch of business users and developers I work with didn't expect this behavior.
To address the parsing concern, would you consider merging this if instead of using Java's URI class, I used a regex?

[graemerocher]

@candrews It may be surprising, but the behaviour is very old and this is the first time we have heard of someone being confused by the current behaviour. Besides that your change has the potential to break existing applications.

I suggest if you wish to contribute something in this area, the documentation would be good place where you could clarify the behaviour for those business users and developers you refer to.

[candrews]

@graemerocher I don't see how this could break existing applications. If the grails application URL is "http://localhost/" and someone does <g:link uri="http://www.google.com"> then http://localhost/http://www.google.com was never okay, so nothing could be broken. Also, all the tests pass... so that's also some indication that chance of regression is low. If you can think of something I would be satisfied, but until then I'd really like to try pressing the issue.

This also impacts g:createLink, which has no "uri" string attribute. The user could write a custom taglib that decides if it should call g:createLink with "relativeUri" or "uri" and then set "absolute" appropriately... but that's really complicated and repetitive.

Wouldn't it be better to just fix the source of the problem (in DefaultLinkGenerator) and fix it everywhere?

I also don't buy the argument that since this is the first you've heard of the problem, there must not be a problem. Many users (including developers who are users) don't report problems, they just live with them. For every bug report, I'd bet there are countless more bugs that go unreported. I may just be the first to speak up... but I doubt I'm the first to notice.

[graemerocher]

@candrews We will have to benchmark the change in performance tests and do a performance comparison, adding parsing of the URI via the URI class could add a performance penalty. The link tag is a tag used many times on a given GSP so even a small degradation can have a big impact. Anyway this change will not be considered until post 2.4 GA.

[graemerocher]

@candrews Also don't get us wrong, we are not trying to be difficult we appreciate the contribution, but I have been doing this for years now and it is very different building a framework like Grails that will be used by thousands of developers worldwide. Any small change has the potential to impact existing applications, so we always err on the side of caution.

[candrews]

@graemerocher I totally get that and appreciate your position. Uri would create a bunch more objects which could be taxing in extreme cases, so I can see how that could be a problem.

Perhaps using a regex to catch just the http/https URIs would be better - that should perform well and not allocate a bunch of objects that have to be GC'ed immediately.

I'm not begging for this to be done now or anything... but do you think the regex approach would be more acceptable?