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
        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: