Using GroovyServlet deployed to Tomcat with a multi-level context causes 404 errors

I have the tendency to close this issue as "Won't fix", since I think we cannot really do anything about the limitation of URL#getPath. But before doing that... what is does "multi-level context" mean here?

CARGO-734 seems to be a related issue from a different project

Example:

/groovyweb <- single level
/apps/groovyweb <- multi-level

Where the groovy application is a sub-url

This used to work in 1.6.5 version of Groovy until changes were made in 1.7 it looks like. Under Tomcat autodeploy naming rules / contexts both are allowed see:

http://tomcat.apache.org/tomcat-7.0-doc/config/context.html#Naming

To me this is actually a huge issue that I would like to get resolved with GroovyScript. Otherwise I have to define many different applications in our proxy for each application vs. just specifying a multi-level application aka : under /apps/myappname

Thanks!

Since 1.7 there is a new GroovyScriptEngine that uses the GroovyClassLoader and URLs instead of only path names, yes. But reenabling that is very difficult, the current design does not really allow for that and you would need a almost completely new GSE. You see the "bug" is actually the url logic in which # is used for other purposes. For the usage in an URL, you would normally have to "encode" it. My time with tomcat is quite a while back already, but wasn't there a way to rewrite the paths before the servlet gets those? If that exists, wouldn't that be an option for you?

I'm dealing with this from an Operations/Infrastructure perspective and there is not an easy way to rewrite the paths before the servlet gets it that I'm aware of.

However, wouldn't GSE need to conform to the TC Naming Context Rules ? In other words I don't think this is a Tomcat issue ? Why was it changed to include both URLs and path names in 1.7 vs. just the path names ?

I took a further look into this today and I think that my initial assessment was off a bit. After further review, I believe that the GroovyScriptEngine#loadScriptByName method is not causing this problem, but it is the AbstractHttpServlet#getResourceConnection method which could be improved.

    public URLConnection getResourceConnection(String name) throws ResourceException {
        String basePath = servletContext.getRealPath("/");
        if (name.startsWith(basePath)) name = name.substring(basePath.length());

        name = name.replaceAll("\\\\", "/");

        //remove the leading / as we are trying with a leading / now
        if (name.startsWith("/")) name = name.substring(1);

        /*
        * Try to locate the resource and return an opened connection to it.
        */
        try {
            String tryScriptName = "/" + name;
            URL url = servletContext.getResource(tryScriptName);
            if (url == null) {
                tryScriptName = "/WEB-INF/groovy/" + name;
                url = servletContext.getResource("/WEB-INF/groovy/" + name);
            }
            if (url == null) {
                throw new ResourceException("Resource \"" + name + "\" not found!");
            } else {
                url = new URL("file", "", servletContext.getRealPath(tryScriptName));
            }
            return url.openConnection();
        } catch (IOException e) {
            throw new ResourceException("Problems getting resource named \"" + name + "\"!", e);
        }
    }

Would it be possible to change this implementation to use a URI, which handles escaping invalid characters?

I successfully tested this in the sample application by subclassing GroovyServlet and overriding getResourceConnection with the following code.

    public URLConnection getResourceConnection(String name) throws ResourceException {
        String basePath = servletContext.getRealPath("/");
        
        try {
            URI nameUri = new URI(name);
            name = nameUri.getPath();
        } catch (URISyntaxException ex) {
            // ignore and use existing name
        }
        
        if (name.startsWith(basePath)) name = name.substring(basePath.length());

        name = name.replaceAll("\\\\", "/");

        //remove the leading / as we are trying with a leading / now
        if (name.startsWith("/")) name = name.substring(1);

        /*
        * Try to locate the resource and return an opened connection to it.
        */
        try {
            String tryScriptName = "/" + name;
            URL url = servletContext.getResource(tryScriptName);
            if (url == null) {
                tryScriptName = "/WEB-INF/groovy/" + name;
                url = servletContext.getResource("/WEB-INF/groovy/" + name);
            }
            if (url == null) {
                throw new ResourceException("Resource \"" + name + "\" not found!");
            } else {
            	URI realPath = new URI("file", "", servletContext.getRealPath(tryScriptName), "");
                url = realPath.toURL();
            }
            return url.openConnection();
        } catch (IOException e) {
            throw new ResourceException("Problems getting resource named \"" + name + "\"!", e);
        } catch (URISyntaxException e) {
            throw new ResourceException("Problems getting resource named \"" + name + "\"!", e);
	}
    }

ah, Daniel, I think this is a nice catch. Yes, this looks like it is possible. All you added is

        try {
            URI nameUri = new URI(name);
            name = nameUri.getPath();
        } catch (URISyntaxException ex) {
            // ignore and use existing name
        }

right?

Two changes...

1.) At the top

        try {
            URI nameUri = new URI(name);
            name = nameUri.getPath();
        } catch (URISyntaxException ex) {
            // ignore and use existing name
        }

2.) Prior to the return

     URI realPath = new URI("file", "", servletContext.getRealPath(tryScriptName), "");
     url = realPath.toURL();

Sorry, I should have submitted a patch.

A patch attached to the JIRA issue, or a GitHub pull request would be awesome
We always love contributions!

Daniel, where you able to submit the patch then? or Guillaume were you going to do that then?

Thanks

Working on it. Should have it posted shortly.

Attaching two patches against Groovy 1.8.6.

The first is a patch with the change that I proposed to the AbstractHttpServlet#getResourceConnection method.

The second is a patch to the test cases for AbstractHttpServlet. With the change applied to AbstractHttpServlet, there are a few tests which will fail. These seem to fail because the mock implementation of "getRealPath" is returning a relative path. I've simply changed the mock implementation to return a full path (i.e. one that starts with "/").

Daniel, first sorry for not coming back to this much earlier. It is nice to have a patch, but without the changes to the tests, the tests fail. This means it is not compatible with older versions, a breaking change. Such a change has no big chance to get into 1.8.x or 2.0.x.

So who normally creates the "changes to the tests" so that they do not fail? We need to get this done to make sure it will work with future versions. Sorry, I'm an end user not a developer but following this but because deploying applications with autodeploy using the standard tomcat naming was breaking my applications.

Thank you to whomever can add the new test cases to get this resolved for future versions.

I can change them, that in itself is not the problem. Changes to existing tests have to be justified. There are there to avoid regression and to avoid a breaking change is done accidentally.

In an ideal world the patch would not require those test to be changed.

In the meantime I made several changes to AbstractHttpServlet, see https://github.com/groovy/groovy-core/blob/master/subprojects/groovy-servlet/src/main/java/groovy/servlet/AbstractHttpServlet.java#L170 We are now using URI in the beginning, so I see a chance your changes may not be required anymore. Though your patch goes further then just using the servlet base path, and I assume that is what is required for you as well. I wonder if we can go with a "new File(servletContext.getRealPath(tryScriptName)).toURI().toURL().toExternalForm()" and not break the tests

James, I did a commit that for me works. I was able to use the multilevel context as well as not getting a 404 on the second try. I am marking the issue as resolved for now. I would like you to try out the change and confirm it is working for you as well. If it does not, we will have to open the issue again