ServletBinding lies about what variables it can handle

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.

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.

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

A patch and test case will be most welcome

Alright! Patch and test case incoming later this week

Accidently set patch root to selection instead of project.

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.

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.

implementation needs a tweak as per recent email discussion

potential fix - has its own issues

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?

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

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.

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

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.

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