You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by Wes Wannemacher <we...@wantii.com> on 2008/02/14 17:48:03 UTC
feedback on patches
Guys,
In an effort to get that money ;-), I'd like to close a few more
JIRAs. This one looks pretty easy, but my unfamiliarity with
Jetty/REST make me a bit apprehensive.
https://issues.apache.org/struts/browse/WW-2350
Can someone take a look at the patch and let me know if if is
accceptable? Also, it looks to be assigned to Don, but I'm thinking
he's been busy lately (writing a book, working a full-time job,
nothing big ;-) ). Is it acceptable to resolve someone else's JIRA?
--
Wesley Wannemacher
President, Head Engineer/Consultant
WanTii, Inc.
http://www.wantii.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: feedback on patches
Posted by Wes Wannemacher <we...@wantii.com>.
The reader that causes the problem is only accessed (currently) when
JSON or XStream data is posted. I would imagine that
buffering/no-buffering isn't going to make that much difference. Plus,
I tested my fix with the struts2-rest-showcase (or whatever it's
called), which isn't a big app, but I verified the bug and verified
the fix with Jetty 6.1.6 and tested in Tomcat and using whichever
version mvn jetty:run pulls out of cyberspace. I would like to apply
it to help push 2.1.1 a little closer to release, but if Don or
Anthony speak up, I'll hold off.
-Wes
On 2/14/08, Matt Raible <ma...@raibledesigns.com> wrote:
> <quote from="WW-2350">
> Although I am not sure about how this will affect performance (since
> this reader may not be buffered), this patch resolves the issue. It
> seems that you get the IllegalStateException if getReader() is called
> after a call to getInputStream(). So, I call getInputStream() again
> and create an InputStreamReader with it. The
> ContentTypeHandler.toObject( method only asks for a Reader, so the
> InputStreamReader gladly accepts it. I verified the issue with the
> struts2-rest-showcase and jetty 6.1.6, and I verified that this patch
> fixes the IllegalStateException. I could commit it myself, but I am
> not very familiar with REST or Jetty. Take a look and commit if it is
> a legitimate resolution.
> </quote>
>
> If it fixes the problem, I see no issues with the suggested fix. Of
> course, I'm not that familiar with the REST plugin, so it's probably
> best that someone like Don verifies this is the best solution.
>
> Matt
>
> On Thu, Feb 14, 2008 at 9:48 AM, Wes Wannemacher <we...@wantii.com> wrote:
> > Guys,
> >
> > In an effort to get that money ;-), I'd like to close a few more
> > JIRAs. This one looks pretty easy, but my unfamiliarity with
> > Jetty/REST make me a bit apprehensive.
> >
> > https://issues.apache.org/struts/browse/WW-2350
> >
> > Can someone take a look at the patch and let me know if if is
> > accceptable? Also, it looks to be assigned to Don, but I'm thinking
> > he's been busy lately (writing a book, working a full-time job,
> > nothing big ;-) ). Is it acceptable to resolve someone else's JIRA?
> >
> > --
> > Wesley Wannemacher
> > President, Head Engineer/Consultant
> > WanTii, Inc.
> > http://www.wantii.com
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> > For additional commands, e-mail: dev-help@struts.apache.org
> >
> >
>
>
>
> --
> http://raibledesigns.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
--
Wesley Wannemacher
President, Head Engineer/Consultant
WanTii, Inc.
http://www.wantii.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: feedback on patches
Posted by Blake Byrnes <bl...@gmail.com>.
BTW - the Json Plugin Result has some of this built in without a dependency
to beanlib.
On Fri, Feb 15, 2008 at 10:21 AM, Blake Byrnes <bl...@gmail.com>
wrote:
> Ok. I think the other nice thing in those stream handlers would be to
> handle hibernate objects on the way out. Right now, they just send back the
> uninitialized junk that hibernate creates. In my project, I used Beanlib to
> run them through a cleaner, but for a slightly different purpose. To really
> integrate it with the content handlers though, I think you'd need to be able
> to specify properties you want to exclude somewhere. Doing that for a whole
> object graph (uses a visitor pattern) is kind of messy in xml though. Which
> means you really need something in your models or in some sort of java
> package with a config per model object.
>
>
> On Fri, Feb 15, 2008 at 10:11 AM, Wes Wannemacher <we...@wantii.com> wrote:
>
> > On 2/15/08, Blake Byrnes <bl...@gmail.com> wrote:
> > [snip]
> > >
> > > Just another potential approach. I don't understand the reasoning
> > behind
> > > the request specs that got posted in the JIRA ticket.
> > >
> >
> > I am not sure about the lengthy discussion either (which was the main
> > reason behind not applying my patch earlier). Basically, what I took
> > from it is that an IllegalStateException is thrown by the app server
> > (Jetty 6.1.6) when the Java/JSP code breaks "the rules" (the spec).
> > I've gotten them before when trying to do silly things like redirect
> > via HTTP 302 after I've already written to the output stream. In this
> > case, I believe that it was tracked down a
> > getParameterMap/getParameter call happening before the call to
> > getReader. Further digging on google led me to think that if the root
> > cause is a call to getInputStream, that a subsequent call shouldn't be
> > illegal (or if it is, it does work w/o exception).
> >
> > Anyhow, I agree that the purpose of the affected code is to
> > deserialize JSON/XStream data, so how it happens is probably
> > irrelevant. If you want to tweak the code further, patches are always
> > welcome. In this case though, start a new JIRA, since the original
> > dealt with the exception in Jetty 6.1.6.
> >
> >
> > --
> > Wesley Wannemacher
> > President, Head Engineer/Consultant
> > WanTii, Inc.
> > http://www.wantii.com
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> > For additional commands, e-mail: dev-help@struts.apache.org
> >
> >
>
Re: feedback on patches
Posted by Blake Byrnes <bl...@gmail.com>.
Ok. I think the other nice thing in those stream handlers would be to
handle hibernate objects on the way out. Right now, they just send back the
uninitialized junk that hibernate creates. In my project, I used Beanlib to
run them through a cleaner, but for a slightly different purpose. To really
integrate it with the content handlers though, I think you'd need to be able
to specify properties you want to exclude somewhere. Doing that for a whole
object graph (uses a visitor pattern) is kind of messy in xml though. Which
means you really need something in your models or in some sort of java
package with a config per model object.
On Fri, Feb 15, 2008 at 10:11 AM, Wes Wannemacher <we...@wantii.com> wrote:
> On 2/15/08, Blake Byrnes <bl...@gmail.com> wrote:
> [snip]
> >
> > Just another potential approach. I don't understand the reasoning
> behind
> > the request specs that got posted in the JIRA ticket.
> >
>
> I am not sure about the lengthy discussion either (which was the main
> reason behind not applying my patch earlier). Basically, what I took
> from it is that an IllegalStateException is thrown by the app server
> (Jetty 6.1.6) when the Java/JSP code breaks "the rules" (the spec).
> I've gotten them before when trying to do silly things like redirect
> via HTTP 302 after I've already written to the output stream. In this
> case, I believe that it was tracked down a
> getParameterMap/getParameter call happening before the call to
> getReader. Further digging on google led me to think that if the root
> cause is a call to getInputStream, that a subsequent call shouldn't be
> illegal (or if it is, it does work w/o exception).
>
> Anyhow, I agree that the purpose of the affected code is to
> deserialize JSON/XStream data, so how it happens is probably
> irrelevant. If you want to tweak the code further, patches are always
> welcome. In this case though, start a new JIRA, since the original
> dealt with the exception in Jetty 6.1.6.
>
>
> --
> Wesley Wannemacher
> President, Head Engineer/Consultant
> WanTii, Inc.
> http://www.wantii.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
Re: feedback on patches
Posted by Wes Wannemacher <we...@wantii.com>.
On 2/15/08, Blake Byrnes <bl...@gmail.com> wrote:
[snip]
>
> Just another potential approach. I don't understand the reasoning behind
> the request specs that got posted in the JIRA ticket.
>
I am not sure about the lengthy discussion either (which was the main
reason behind not applying my patch earlier). Basically, what I took
from it is that an IllegalStateException is thrown by the app server
(Jetty 6.1.6) when the Java/JSP code breaks "the rules" (the spec).
I've gotten them before when trying to do silly things like redirect
via HTTP 302 after I've already written to the output stream. In this
case, I believe that it was tracked down a
getParameterMap/getParameter call happening before the call to
getReader. Further digging on google led me to think that if the root
cause is a call to getInputStream, that a subsequent call shouldn't be
illegal (or if it is, it does work w/o exception).
Anyhow, I agree that the purpose of the affected code is to
deserialize JSON/XStream data, so how it happens is probably
irrelevant. If you want to tweak the code further, patches are always
welcome. In this case though, start a new JIRA, since the original
dealt with the exception in Jetty 6.1.6.
--
Wesley Wannemacher
President, Head Engineer/Consultant
WanTii, Inc.
http://www.wantii.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: feedback on patches
Posted by Blake Byrnes <bl...@gmail.com>.
To me, this doesn't seem to have anything to do with REST. It seems like
the same problem as anytime you are reading parameters out of a request. In
this case, the streamreader is passed to XStream and JSON-libe so that they
can populate Java objects inline. If a Map has already been created from
the parameters (which I think it has), I believe you could also pass that to
both libraries to translate into target objects.
Just another potential approach. I don't understand the reasoning behind
the request specs that got posted in the JIRA ticket.
On Thu, Feb 14, 2008 at 3:07 PM, Al Sutton <al...@alsutton.com> wrote:
> I've always been a fan of a working solution over waiting for the perfect
> solution.
>
> If it fixes the problem, and doesn't cause any other issues then it's one
> step on from having a problem. If a REST expert wants to optimize the
> solution then that can be done as another step.
>
> Al.
>
> ----- Original Message -----
> From: "Matt Raible" <ma...@raibledesigns.com>
> To: "Struts Developers List" <de...@struts.apache.org>
> Sent: Thursday, February 14, 2008 4:54 PM
> Subject: Re: feedback on patches
>
>
> > <quote from="WW-2350">
> > Although I am not sure about how this will affect performance (since
> > this reader may not be buffered), this patch resolves the issue. It
> > seems that you get the IllegalStateException if getReader() is called
> > after a call to getInputStream(). So, I call getInputStream() again
> > and create an InputStreamReader with it. The
> > ContentTypeHandler.toObject( method only asks for a Reader, so the
> > InputStreamReader gladly accepts it. I verified the issue with the
> > struts2-rest-showcase and jetty 6.1.6, and I verified that this patch
> > fixes the IllegalStateException. I could commit it myself, but I am
> > not very familiar with REST or Jetty. Take a look and commit if it is
> > a legitimate resolution.
> > </quote>
> >
> > If it fixes the problem, I see no issues with the suggested fix. Of
> > course, I'm not that familiar with the REST plugin, so it's probably
> > best that someone like Don verifies this is the best solution.
> >
> > Matt
> >
> > On Thu, Feb 14, 2008 at 9:48 AM, Wes Wannemacher <we...@wantii.com>
> wrote:
> >> Guys,
> >>
> >> In an effort to get that money ;-), I'd like to close a few more
> >> JIRAs. This one looks pretty easy, but my unfamiliarity with
> >> Jetty/REST make me a bit apprehensive.
> >>
> >> https://issues.apache.org/struts/browse/WW-2350
> >>
> >> Can someone take a look at the patch and let me know if if is
> >> accceptable? Also, it looks to be assigned to Don, but I'm thinking
> >> he's been busy lately (writing a book, working a full-time job,
> >> nothing big ;-) ). Is it acceptable to resolve someone else's JIRA?
> >>
> >> --
> >> Wesley Wannemacher
> >> President, Head Engineer/Consultant
> >> WanTii, Inc.
> >> http://www.wantii.com
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> >> For additional commands, e-mail: dev-help@struts.apache.org
> >>
> >>
> >
> >
> >
> > --
> > http://raibledesigns.com
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> > For additional commands, e-mail: dev-help@struts.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
Re: feedback on patches
Posted by Al Sutton <al...@alsutton.com>.
I've always been a fan of a working solution over waiting for the perfect
solution.
If it fixes the problem, and doesn't cause any other issues then it's one
step on from having a problem. If a REST expert wants to optimize the
solution then that can be done as another step.
Al.
----- Original Message -----
From: "Matt Raible" <ma...@raibledesigns.com>
To: "Struts Developers List" <de...@struts.apache.org>
Sent: Thursday, February 14, 2008 4:54 PM
Subject: Re: feedback on patches
> <quote from="WW-2350">
> Although I am not sure about how this will affect performance (since
> this reader may not be buffered), this patch resolves the issue. It
> seems that you get the IllegalStateException if getReader() is called
> after a call to getInputStream(). So, I call getInputStream() again
> and create an InputStreamReader with it. The
> ContentTypeHandler.toObject( method only asks for a Reader, so the
> InputStreamReader gladly accepts it. I verified the issue with the
> struts2-rest-showcase and jetty 6.1.6, and I verified that this patch
> fixes the IllegalStateException. I could commit it myself, but I am
> not very familiar with REST or Jetty. Take a look and commit if it is
> a legitimate resolution.
> </quote>
>
> If it fixes the problem, I see no issues with the suggested fix. Of
> course, I'm not that familiar with the REST plugin, so it's probably
> best that someone like Don verifies this is the best solution.
>
> Matt
>
> On Thu, Feb 14, 2008 at 9:48 AM, Wes Wannemacher <we...@wantii.com> wrote:
>> Guys,
>>
>> In an effort to get that money ;-), I'd like to close a few more
>> JIRAs. This one looks pretty easy, but my unfamiliarity with
>> Jetty/REST make me a bit apprehensive.
>>
>> https://issues.apache.org/struts/browse/WW-2350
>>
>> Can someone take a look at the patch and let me know if if is
>> accceptable? Also, it looks to be assigned to Don, but I'm thinking
>> he's been busy lately (writing a book, working a full-time job,
>> nothing big ;-) ). Is it acceptable to resolve someone else's JIRA?
>>
>> --
>> Wesley Wannemacher
>> President, Head Engineer/Consultant
>> WanTii, Inc.
>> http://www.wantii.com
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
>> For additional commands, e-mail: dev-help@struts.apache.org
>>
>>
>
>
>
> --
> http://raibledesigns.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: feedback on patches
Posted by Matt Raible <ma...@raibledesigns.com>.
<quote from="WW-2350">
Although I am not sure about how this will affect performance (since
this reader may not be buffered), this patch resolves the issue. It
seems that you get the IllegalStateException if getReader() is called
after a call to getInputStream(). So, I call getInputStream() again
and create an InputStreamReader with it. The
ContentTypeHandler.toObject( method only asks for a Reader, so the
InputStreamReader gladly accepts it. I verified the issue with the
struts2-rest-showcase and jetty 6.1.6, and I verified that this patch
fixes the IllegalStateException. I could commit it myself, but I am
not very familiar with REST or Jetty. Take a look and commit if it is
a legitimate resolution.
</quote>
If it fixes the problem, I see no issues with the suggested fix. Of
course, I'm not that familiar with the REST plugin, so it's probably
best that someone like Don verifies this is the best solution.
Matt
On Thu, Feb 14, 2008 at 9:48 AM, Wes Wannemacher <we...@wantii.com> wrote:
> Guys,
>
> In an effort to get that money ;-), I'd like to close a few more
> JIRAs. This one looks pretty easy, but my unfamiliarity with
> Jetty/REST make me a bit apprehensive.
>
> https://issues.apache.org/struts/browse/WW-2350
>
> Can someone take a look at the patch and let me know if if is
> accceptable? Also, it looks to be assigned to Don, but I'm thinking
> he's been busy lately (writing a book, working a full-time job,
> nothing big ;-) ). Is it acceptable to resolve someone else's JIRA?
>
> --
> Wesley Wannemacher
> President, Head Engineer/Consultant
> WanTii, Inc.
> http://www.wantii.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
--
http://raibledesigns.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org