groovy
  1. groovy
  2. GROOVY-3187

ServletBinding lies about what variables it can handle

    Details

    • Number of attachments :
      5

      Description

      If you look at the current implementation of ServletBinding in trunk, method getVariables() is not an accurate depiction of what variables getVariable will to respond to.

      public Map getVariables() {
      return binding.getVariables();
      }

      This returns the list of variables that were specifically created through setVariable. But if you look at the implementation of getVariable it inspects the name of the property that is being requested and will handle an additional three variables out, sout and html. Giving the fact that getVariables is the only way to inspect if a binding is aware of a specific variable without dealing with exception handling (getVariable throws an exeception if it cant find said variable) this seems like a bug.

      The two possible solutions being:
      1.) Provide a method to determine the existence of a variable without having to catch exceptions
      or
      2.) Register out, sout and html via setVariable and not property name inspection in getVariable

      1. Clean_ServletBinding.java.patch
        5 kB
        Stephen Solka
      2. Groovy3187Wrapper.patch
        6 kB
        blackdrag blackdrag
      3. servlet3187_fix.patch
        3 kB
        Paul King
      4. ServletBinding.java.patch
        2 kB
        Stephen Solka
      5. ServletBindingTest.groovy.patch
        1 kB
        Stephen Solka

        Activity

        Hide
        Guillaume Laforge added a comment -

        Out of curiosity, why do you really need these variables to be listed in the binding?
        They are really special, and are not variables put by the user, so people only interested with the user variables will have to filter these ones out.

        Show
        Guillaume Laforge added a comment - Out of curiosity, why do you really need these variables to be listed in the binding? They are really special, and are not variables put by the user, so people only interested with the user variables will have to filter these ones out.
        Hide
        Stephen Solka added a comment -

        I am not sure I understand the point. Of the seven variables actually put into the binding (request, response, context, application, session, params, headers) none of these were put there by users but programatically injected by the binding itself. So these people who only want user variables would already be filtering these out.

        The actual code that lead me to this discovery was I implemented a binding delegate class. The class take two bindings one acting as a base binding and one acting as an extension to that base. When a script calls getVariable the delegate first checks if the extension can handle the variable if not it falls back to the base binding. Any calls to set variable are passed to the extension binding. This allows me to expose a set of variables to groovlets as top level variables like request response etc but not have this binding modified between groovlet calls. An example usage of this if I want to expose a special binding that maps my spring bean mappings as a variable called "beans" to all my groovlets.

        I also would like to note that its a bit frustrating that GroovyServlet does not methodize the creation of the binding that is used within groovlets. It just spawns off new ServletBinding in the middle of a large block of code. Ive found ways to get around that without having to rewirte the entire GroovyServlet but I had to go around my elbow to get to my face as the saying goes.

        I went ahead and just rewrote getVariable on my delegate to just catch the exception and call the bases get variable in the catch block. But I maintain that even if the out, sout and html have special meaning in a groovy script they are still located via the bindings getVariable and should be treated no differently than request, and response. None of the variables injected into the variables hashmap during construction of the ServletBinding are user created variables anyways.

        Show
        Stephen Solka added a comment - I am not sure I understand the point. Of the seven variables actually put into the binding (request, response, context, application, session, params, headers) none of these were put there by users but programatically injected by the binding itself. So these people who only want user variables would already be filtering these out. The actual code that lead me to this discovery was I implemented a binding delegate class. The class take two bindings one acting as a base binding and one acting as an extension to that base. When a script calls getVariable the delegate first checks if the extension can handle the variable if not it falls back to the base binding. Any calls to set variable are passed to the extension binding. This allows me to expose a set of variables to groovlets as top level variables like request response etc but not have this binding modified between groovlet calls. An example usage of this if I want to expose a special binding that maps my spring bean mappings as a variable called "beans" to all my groovlets. I also would like to note that its a bit frustrating that GroovyServlet does not methodize the creation of the binding that is used within groovlets. It just spawns off new ServletBinding in the middle of a large block of code. Ive found ways to get around that without having to rewirte the entire GroovyServlet but I had to go around my elbow to get to my face as the saying goes. I went ahead and just rewrote getVariable on my delegate to just catch the exception and call the bases get variable in the catch block. But I maintain that even if the out, sout and html have special meaning in a groovy script they are still located via the bindings getVariable and should be treated no differently than request, and response. None of the variables injected into the variables hashmap during construction of the ServletBinding are user created variables anyways.
        Hide
        Guillaume Laforge added a comment -

        Alright, makes sense as I had forgotten the other variables like request and so on were also returned

        Show
        Guillaume Laforge added a comment - Alright, makes sense as I had forgotten the other variables like request and so on were also returned
        Hide
        Guillaume Laforge added a comment -

        A patch and test case will be most welcome

        Show
        Guillaume Laforge added a comment - A patch and test case will be most welcome
        Hide
        Stephen Solka added a comment -

        Alright! Patch and test case incoming later this week

        Show
        Stephen Solka added a comment - Alright! Patch and test case incoming later this week
        Hide
        Stephen Solka added a comment -

        Accidently set patch root to selection instead of project.

        Show
        Stephen Solka added a comment - Accidently set patch root to selection instead of project.
        Hide
        Stephen Solka added a comment -

        Since this was my first patch I was not sure what you guys were looking for. ServletBinding.java.patch is a targeted patch that just fixes exactly what was described in this bug. But there are some left over internal variables that are no longer needed.

        Clean_ServletBinding.java.patch is the cleaned up version of this patch. So its up to you guys if you wanted one that changes just what we talked about or one that changes what we talked about and removes some dead code. If you could provide guidance on this so I may know how to handle this in the future that would be helpful.

        Show
        Stephen Solka added a comment - Since this was my first patch I was not sure what you guys were looking for. ServletBinding.java.patch is a targeted patch that just fixes exactly what was described in this bug. But there are some left over internal variables that are no longer needed. Clean_ServletBinding.java.patch is the cleaned up version of this patch. So its up to you guys if you wanted one that changes just what we talked about or one that changes what we talked about and removes some dead code. If you could provide guidance on this so I may know how to handle this in the future that would be helpful.
        Hide
        Paul King added a comment -

        Hi Stephen. Thanks for the patch. Your submission was fine. It was good to have both versions to look at. Often we will have to make changes anyway, so I guess there is no absolute guideline on the correct kind of patch to submit.

        The main thing I changed was to keep the lazy semantics of setting the out, sout and html variables as that behavior may be important for certain (perhaps legacy) web containers. I.e. the setting of those variables is deferred until the first attempted usage of the binding. Otherwise, there might be problems with certain web containers or with existing code that isn't expecting the constructor call to break. I think the solution we now have is a reasonable compromise.

        Show
        Paul King added a comment - Hi Stephen. Thanks for the patch. Your submission was fine. It was good to have both versions to look at. Often we will have to make changes anyway, so I guess there is no absolute guideline on the correct kind of patch to submit. The main thing I changed was to keep the lazy semantics of setting the out , sout and html variables as that behavior may be important for certain (perhaps legacy) web containers. I.e. the setting of those variables is deferred until the first attempted usage of the binding. Otherwise, there might be problems with certain web containers or with existing code that isn't expecting the constructor call to break. I think the solution we now have is a reasonable compromise.
        Hide
        Paul King added a comment -

        implementation needs a tweak as per recent email discussion

        Show
        Paul King added a comment - implementation needs a tweak as per recent email discussion
        Hide
        Paul King added a comment -

        potential fix - has its own issues

        Show
        Paul King added a comment - potential fix - has its own issues
        Hide
        Stephen Solka added a comment -

        You originally stated "The main thing I changed was to keep the lazy semantics of setting the out, sout and html variables as that behavior may be important for certain (perhaps legacy) web containers" for the retention of the lazy initialization. Is this actually an issue? Or may be an issue? Is this new patch related to the lazy initialization?

        Show
        Stephen Solka added a comment - You originally stated "The main thing I changed was to keep the lazy semantics of setting the out, sout and html variables as that behavior may be important for certain (perhaps legacy) web containers" for the retention of the lazy initialization. Is this actually an issue? Or may be an issue? Is this new patch related to the lazy initialization?
        Hide
        Stephen Solka added a comment -

        I tracked down the email thread. I now understand the issue much better. My previous question makes no sense now and I retract it.

        Show
        Stephen Solka added a comment - I tracked down the email thread. I now understand the issue much better. My previous question makes no sense now and I retract it.
        Hide
        Stephen Solka added a comment -

        This almost seems like a flaw in Binding interface. I originally filed this bug because I was writing a BindingDelegate that needed to inspect a binding to see if it knew how to handle a request for a variable name, if it did not would ask the alternative binding. Unfortunately, the only method available for such a request is getVariables() which return a hashmap. This means at the time of request all keys AND values must be known. If there was an alternative method getVariableNames it would alow both the lazy initialization this fix requires and the binding inspection that came from the initial request.

        Though changing an interface like Binding is probably more pain than its worth.

        Show
        Stephen Solka added a comment - This almost seems like a flaw in Binding interface. I originally filed this bug because I was writing a BindingDelegate that needed to inspect a binding to see if it knew how to handle a request for a variable name, if it did not would ask the alternative binding. Unfortunately, the only method available for such a request is getVariables() which return a hashmap. This means at the time of request all keys AND values must be known. If there was an alternative method getVariableNames it would alow both the lazy initialization this fix requires and the binding inspection that came from the initial request. Though changing an interface like Binding is probably more pain than its worth.
        Hide
        blackdrag blackdrag added a comment -

        Groovy3187Wrapper.patch tries to solve the issue by wrapping the output stream and the writer into a wrapper that allows out, sout and html to be get at the same time, with actually performing HttpResponse.getWriter() or HttpResponse.getOutputStream until the point the writer/stream is actually needed. If one of these methods is called calling the other one will cause a IllegalStateException. The exception happens only if a write, close, flash, format or checkError method is called. Simply getting sout and out itself will not cause any exception

        Show
        blackdrag blackdrag added a comment - Groovy3187Wrapper.patch tries to solve the issue by wrapping the output stream and the writer into a wrapper that allows out, sout and html to be get at the same time, with actually performing HttpResponse.getWriter() or HttpResponse.getOutputStream until the point the writer/stream is actually needed. If one of these methods is called calling the other one will cause a IllegalStateException. The exception happens only if a write, close, flash, format or checkError method is called. Simply getting sout and out itself will not cause any exception
        Hide
        Guillaume Laforge added a comment -

        Looking at the patch, why are there imports changes in MarkupBuilder?
        Also the error message "The variables 'out' or 'html' has been used already. Use either out/html or sout, not both.": either remove the s at variables or change has to have.
        Otherwise the wrapping approach looks okay to me, although I haven't tested the patch with a Groovlet yet.

        Show
        Guillaume Laforge added a comment - Looking at the patch, why are there imports changes in MarkupBuilder? Also the error message "The variables 'out' or 'html' has been used already. Use either out/html or sout, not both.": either remove the s at variables or change has to have. Otherwise the wrapping approach looks okay to me, although I haven't tested the patch with a Groovlet yet.
        Hide
        Paul King added a comment -

        Looks good, though I haven't tested it yet. One other typo: 'invlid' -> 'invalid' in InvalidOutputStream.

        Show
        Paul King added a comment - Looks good, though I haven't tested it yet. One other typo: 'invlid' -> 'invalid' in InvalidOutputStream.

          People

          • Assignee:
            Paul King
            Reporter:
            Stephen Solka
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: