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