You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuscany.apache.org by Jeremy Boynes <jb...@apache.org> on 2006/03/04 17:10:34 UTC
Re: svn commit: r383063 - /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
Please take Ant's comment to heart - it is very hard to review something
if the actual change is buried amongst a mass of code reformatting.
Patches like that are often rejected out of hand.
Digging through this one I have concerns about what it is doing. The
Axis servlet is now going to the root context, extracting all modules
loaded in the system and defining all entry points *from all modules*
into the Axis engine contained in *this* web application.
This exposes entry points from other modules under the context path for
this application. This will cause problems if they define ones with the
same name and opens a significant security hole.
I am not clear what problem this was trying to solve - please can you
describe that to the list so we can come up with a better solution.
Until then we should back this patch out.
--
Jeremy
jsdelfino@apache.org wrote:
> Author: jsdelfino
> Date: Fri Mar 3 22:53:09 2006
> New Revision: 383063
>
> URL: http://svn.apache.org/viewcvs?rev=383063&view=rev
> Log:
> applied Raymond's patch to register entry points from ALL modules
>
> Modified:
> incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
>
> Modified: incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
> URL: http://svn.apache.org/viewcvs/incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java?rev=383063&r1=383062&r2=383063&view=diff
> ==============================================================================
> --- incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java (original)
> +++ incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java Fri Mar 3 22:53:09 2006
> @@ -17,7 +17,6 @@
...
> + RuntimeContext tuscanyRuntime = getTuscanyWebAppRuntime( config );
> tuscanyRuntime.start();
> AggregateContext moduleContext = tuscanyRuntime.getRootContext();
> + Module rootModule = (Module) moduleContext.getAggregate();
> +
> + for ( Iterator m = rootModule.getComponents().iterator(); m.hasNext(); )
> + {
Re: svn commit: r383063 - /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
Posted by Jeremy Boynes <jb...@apache.org>.
Jean-Sebastien Delfino wrote:
>
> Jeremy, I think it would be very useful to add a page to our Wiki on
> best practices for contributing and handling patches? Would you want to
> help with that? Thanks...
>
That's a good idea - thanks for volunteering ...
--
Jeremy
Re: svn commit: r383063 - /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
Posted by Jean-Sebastien Delfino <js...@apache.org>.
Raymond Feng wrote:
> Hi,
>
> Thanks for the clarification. I updated the JIRA with a new patch that fix
> the scoping issue.
>
> Raymond
>
> ----- Original Message -----
> From: "Jeremy Boynes" <jb...@apache.org>
> To: <tu...@ws.apache.org>
> Sent: Saturday, March 04, 2006 9:39 AM
> Subject: Re: svn commit: r383063 -
> /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/b
> inding/axis2/handler/WebServiceEntryPointServlet.java
>
>
>
>> Raymond Feng wrote:
>>
>>> Hi,
>>>
>>> I meant to help instead of a trouble-maker.
>>>
>>>
>> Thanks - your help is appreciated. Perhaps the comments were a little
>> harsh but the mixture of reformatting and a functional change makes it
>> hard for people to review understand a change before applying it, which
>> will typically result in the patch being ignored.
>>
>>
>>> 1) My Eclipse does have the template "maven-eclipse-codestyle.xml"
>>>
> loaded
>
>>> and I formatted the code based on it. I'm not sure why it produces
>>>
> different
>
>>> a format.
>>>
>>>
>> I'm not sure - I always turn auto-formatting off on the IDE so that this
>> kind of thing does not happen by accident.
>>
>> It is also worth reviewing any patch before posting it to check that is
>> does not contain changes you did not intend. The clearer the change, the
>> easier for people to understand.
>>
>>
>>> 2) The acceptance testing was broken last night. After debugging, I
>>>
> found
>
>>> the service population logic has a problem. The patch was created to fix
>>>
> the
>
>>> issue and get at all the test case going. It's maybe not perfect. But in
>>> this case the logic to load all modules in the web application was there
>>>
> in
>
>>> the orginal code. I have found issue as well and plan to fix that later
>>>
> on.
>
>> It would be good to know what was broken and in what area. I saw you
>> opened a JIRA (and for that thanks) but the comment in it just contains
>> the stacktrace and that is not enough for someone to be able to
>> reproduce the problem. It could be that this is the right thing to do
>> but without knowing the real problem it is very hard to judge.
>>
>>
>>> I'm new to the open source community. I wish you can forgive any stupid
>>> mistakes I may make. The intension to contribute and help is from the
>>>
> bottom
>
>>> of my heart. I'll learn to be a good player.
>>>
>>>
>> Again thanks for the help and contribution, and I hope my comments have
>> not discouraged you from participating. My intention has been to try and
>> explain why things like clear patches and JIRA comments are so important
>> when the main communication channel is email.
>>
>> If you could update the JIRA with what you were originally doing that
>> triggered the problem and how to reproduce it, it would really help
>> understand what is going on which is the first step in getting this
>> resolved.
>>
>> --
>> Jeremy
>>
>
>
>
Raymond, I would like to thank you for helping diagnose and provide a
fix to this problem with the registration of Web Service entry points.
This bug was causing random errors (in particular different behavior
between Linux and Windows) and was very difficult to find. Your support
in debugging this and the patch you provided helped me a long way
bring-up our Web Services / Axis 2 integration yesterday. Your help is
greatly appreciated, Thanks again!
Jeremy, I think it would be very useful to add a page to our Wiki on
best practices for contributing and handling patches? Would you want to
help with that? Thanks...
--
Jean-Sebastien
Re: svn commit: r383063 - /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
Posted by Raymond Feng <en...@gmail.com>.
Hi,
Thanks for the clarification. I updated the JIRA with a new patch that fix
the scoping issue.
Raymond
----- Original Message -----
From: "Jeremy Boynes" <jb...@apache.org>
To: <tu...@ws.apache.org>
Sent: Saturday, March 04, 2006 9:39 AM
Subject: Re: svn commit: r383063 -
/incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/b
inding/axis2/handler/WebServiceEntryPointServlet.java
> Raymond Feng wrote:
> > Hi,
> >
> > I meant to help instead of a trouble-maker.
> >
>
> Thanks - your help is appreciated. Perhaps the comments were a little
> harsh but the mixture of reformatting and a functional change makes it
> hard for people to review understand a change before applying it, which
> will typically result in the patch being ignored.
>
> > 1) My Eclipse does have the template "maven-eclipse-codestyle.xml"
loaded
> > and I formatted the code based on it. I'm not sure why it produces
different
> > a format.
> >
>
> I'm not sure - I always turn auto-formatting off on the IDE so that this
> kind of thing does not happen by accident.
>
> It is also worth reviewing any patch before posting it to check that is
> does not contain changes you did not intend. The clearer the change, the
> easier for people to understand.
>
> > 2) The acceptance testing was broken last night. After debugging, I
found
> > the service population logic has a problem. The patch was created to fix
the
> > issue and get at all the test case going. It's maybe not perfect. But in
> > this case the logic to load all modules in the web application was there
in
> > the orginal code. I have found issue as well and plan to fix that later
on.
> >
>
> It would be good to know what was broken and in what area. I saw you
> opened a JIRA (and for that thanks) but the comment in it just contains
> the stacktrace and that is not enough for someone to be able to
> reproduce the problem. It could be that this is the right thing to do
> but without knowing the real problem it is very hard to judge.
>
> > I'm new to the open source community. I wish you can forgive any stupid
> > mistakes I may make. The intension to contribute and help is from the
bottom
> > of my heart. I'll learn to be a good player.
> >
>
> Again thanks for the help and contribution, and I hope my comments have
> not discouraged you from participating. My intention has been to try and
> explain why things like clear patches and JIRA comments are so important
> when the main communication channel is email.
>
> If you could update the JIRA with what you were originally doing that
> triggered the problem and how to reproduce it, it would really help
> understand what is going on which is the first step in getting this
> resolved.
>
> --
> Jeremy
Re: svn commit: r383063 - /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
Posted by Jeremy Boynes <jb...@apache.org>.
Raymond Feng wrote:
> Hi,
>
> I meant to help instead of a trouble-maker.
>
Thanks - your help is appreciated. Perhaps the comments were a little
harsh but the mixture of reformatting and a functional change makes it
hard for people to review understand a change before applying it, which
will typically result in the patch being ignored.
> 1) My Eclipse does have the template "maven-eclipse-codestyle.xml" loaded
> and I formatted the code based on it. I'm not sure why it produces different
> a format.
>
I'm not sure - I always turn auto-formatting off on the IDE so that this
kind of thing does not happen by accident.
It is also worth reviewing any patch before posting it to check that is
does not contain changes you did not intend. The clearer the change, the
easier for people to understand.
> 2) The acceptance testing was broken last night. After debugging, I found
> the service population logic has a problem. The patch was created to fix the
> issue and get at all the test case going. It's maybe not perfect. But in
> this case the logic to load all modules in the web application was there in
> the orginal code. I have found issue as well and plan to fix that later on.
>
It would be good to know what was broken and in what area. I saw you
opened a JIRA (and for that thanks) but the comment in it just contains
the stacktrace and that is not enough for someone to be able to
reproduce the problem. It could be that this is the right thing to do
but without knowing the real problem it is very hard to judge.
> I'm new to the open source community. I wish you can forgive any stupid
> mistakes I may make. The intension to contribute and help is from the bottom
> of my heart. I'll learn to be a good player.
>
Again thanks for the help and contribution, and I hope my comments have
not discouraged you from participating. My intention has been to try and
explain why things like clear patches and JIRA comments are so important
when the main communication channel is email.
If you could update the JIRA with what you were originally doing that
triggered the problem and how to reproduce it, it would really help
understand what is going on which is the first step in getting this
resolved.
--
Jeremy
Re: svn commit: r383063 - /incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java
Posted by Raymond Feng <en...@gmail.com>.
Hi,
I meant to help instead of a trouble-maker.
1) My Eclipse does have the template "maven-eclipse-codestyle.xml" loaded
and I formatted the code based on it. I'm not sure why it produces different
a format.
2) The acceptance testing was broken last night. After debugging, I found
the service population logic has a problem. The patch was created to fix the
issue and get at all the test case going. It's maybe not perfect. But in
this case the logic to load all modules in the web application was there in
the orginal code. I have found issue as well and plan to fix that later on.
I'm new to the open source community. I wish you can forgive any stupid
mistakes I may make. The intension to contribute and help is from the bottom
of my heart. I'll learn to be a good player.
Thanks,
Raymond
----- Original Message -----
From: "Jeremy Boynes" <jb...@apache.org>
To: <tu...@ws.apache.org>
Sent: Saturday, March 04, 2006 8:10 AM
Subject: Re: svn commit: r383063 -
/incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/b
inding/axis2/handler/WebServiceEntryPointServlet.java
> Please take Ant's comment to heart - it is very hard to review something
> if the actual change is buried amongst a mass of code reformatting.
> Patches like that are often rejected out of hand.
>
> Digging through this one I have concerns about what it is doing. The
> Axis servlet is now going to the root context, extracting all modules
> loaded in the system and defining all entry points *from all modules*
> into the Axis engine contained in *this* web application.
>
> This exposes entry points from other modules under the context path for
> this application. This will cause problems if they define ones with the
> same name and opens a significant security hole.
>
> I am not clear what problem this was trying to solve - please can you
> describe that to the list so we can come up with a better solution.
> Until then we should back this patch out.
>
> --
> Jeremy
>
> jsdelfino@apache.org wrote:
> > Author: jsdelfino
> > Date: Fri Mar 3 22:53:09 2006
> > New Revision: 383063
> >
> > URL: http://svn.apache.org/viewcvs?rev=383063&view=rev
> > Log:
> > applied Raymond's patch to register entry points from ALL modules
> >
> > Modified:
> >
incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/bi
nding/axis2/handler/WebServiceEntryPointServlet.java
> >
> > Modified:
incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/bi
nding/axis2/handler/WebServiceEntryPointServlet.java
> > URL:
http://svn.apache.org/viewcvs/incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/binding/axis2/handler/WebServiceEntryPointServlet.java?rev=383063&r1=383062&r2=383063&view=diff
> >
============================================================================
==
> > ---
incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/bi
nding/axis2/handler/WebServiceEntryPointServlet.java (original)
> > +++
incubator/tuscany/java/sca/binding.axis2/src/main/java/org/apache/tuscany/bi
nding/axis2/handler/WebServiceEntryPointServlet.java Fri Mar 3 22:53:09
2006
> > @@ -17,7 +17,6 @@
>
> ...
>
> > + RuntimeContext tuscanyRuntime = getTuscanyWebAppRuntime(
config );
>
> > tuscanyRuntime.start();
> > AggregateContext moduleContext =
tuscanyRuntime.getRootContext();
> > + Module rootModule = (Module) moduleContext.getAggregate();
> > +
> > + for ( Iterator m = rootModule.getComponents().iterator();
m.hasNext(); )
> > + {
>