groovy
  1. groovy
  2. GROOVY-5289

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

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.3, 1.8.5
    • Fix Version/s: 2.0.6, 1.8.9
    • Component/s: Groovlet / GSP
    • Labels:
      None
    • Environment:
      Ubuntu Linux, Tomcat 7
    • Number of attachments :
      3

      Description

      When deploying an application which use the GroovyServlet to Tomcat with a simple context, there are no problems. However, when deploying the same application to the same Tomcat instance using a multi-level context a 404 error is returned.

      Attaching a sample application which demonstrates this behavior. Simply run "mvn package" to create a WAR from the project. Take the "groovyweb.war" file from the "target" directory and deploy it to a Tomcat server. This will work just fine. Now take the same WAR file, rename it to "apps#groovyweb.war" and deploy it to Tomcat. Now a 404 is presented and in the log, a message indicates that the groovy script cannot be found.

      ----------------

      Looking into this, it appears that the "#" in the name is causing the problem. In the "loadScriptName" method of GroovyScriptEngine, it is calling "conn.getURL().getPath()" on the URLConnection to the resource. Because there is a "#" character in the URL, this only returns the part of the URL up to the "#" character.

      GroovyScriptEngine.java
      public Class loadScriptByName(String scriptName) throws ResourceException, ScriptException {
          URLConnection conn = rc.getResourceConnection(scriptName);
          String path = conn.getURL().getPath();
          ScriptCacheEntry entry = scriptCache.get(path);
          Class clazz = null;
          if (entry != null) clazz = entry.scriptClass;
          try {
              if (isSourceNewer(entry)) {
                  try {
                      String encoding = conn.getContentEncoding() != null ? conn.getContentEncoding() : "UTF-8";
                      clazz = groovyLoader.parseClass(DefaultGroovyMethods.getText(conn.getInputStream(), encoding), path);
                  } catch (IOException e) {
                      throw new ResourceException(e);
                  }
              }
          } finally {
              forceClose(conn);
          }
          return clazz;
      }
      
      1. AbstractHttpServlet.java.patch
        2 kB
        Daniel Mikusa
      2. AbstractHttpServletTest.groovy.patch
        3 kB
        Daniel Mikusa
      3. groovyweb.tar.gz
        2 kB
        Daniel Mikusa

        Activity

        Hide
        blackdrag blackdrag added a comment -

        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?

        Show
        blackdrag blackdrag added a comment - 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?
        Hide
        blackdrag blackdrag added a comment -

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

        Show
        blackdrag blackdrag added a comment - CARGO-734 seems to be a related issue from a different project
        Hide
        james added a comment -

        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!

        Show
        james added a comment - 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!
        Hide
        blackdrag blackdrag added a comment -

        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?

        Show
        blackdrag blackdrag added a comment - 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?
        Hide
        james added a comment -

        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 ?

        Show
        james added a comment - 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 ?
        Hide
        Daniel Mikusa added a comment -

        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);
        	}
            }
        
        Show
        Daniel Mikusa added a comment - 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); } }
        Hide
        blackdrag blackdrag added a comment -

        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?

        Show
        blackdrag blackdrag added a comment - 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?
        Hide
        Daniel Mikusa added a comment -

        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.

        Show
        Daniel Mikusa added a comment - 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.
        Hide
        Guillaume Laforge added a comment -

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

        Show
        Guillaume Laforge added a comment - A patch attached to the JIRA issue, or a GitHub pull request would be awesome We always love contributions!
        Hide
        james added a comment -

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

        Thanks

        Show
        james added a comment - Daniel, where you able to submit the patch then? or Guillaume were you going to do that then? Thanks
        Hide
        Daniel Mikusa added a comment -

        Working on it. Should have it posted shortly.

        Show
        Daniel Mikusa added a comment - Working on it. Should have it posted shortly.
        Hide
        Daniel Mikusa added a comment -

        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 "/").

        Show
        Daniel Mikusa added a comment - 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 "/").
        Hide
        blackdrag blackdrag added a comment - - edited

        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.

        Show
        blackdrag blackdrag added a comment - - edited 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.
        Hide
        james added a comment -

        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.

        Show
        james added a comment - 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.
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Daniel Mikusa
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: