You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-dev@axis.apache.org by Thilina Gunarathne <cs...@gmail.com> on 2008/12/18 18:32:23 UTC
[axis2] Cleaning up the builders and documentation [was Re: Comments about the new UnknownContentBuilder (AXIS2-4153)]
Hi guys,
May it's because I'm not 100% in touch with the project nowadays, but there
seems to be lot of undocumented features/stuff all over the place :(...
After the recent conversations in the JIRA's and in the mailing list, I
discovered a one hidden feature (wild card support for builders) and many
builders who are doing more or less the same thing (unknownContentBuilder,
BinaryBuilder in commons tranport, DataSourceBuilder)... I should
personally take the blame for not documenting many Axis2 defualt builders
:(..
How about
1. We bring down the builders in commons/transport to Axis2
2. List down the requirements of these builders
eg: UnknownContentbuilder should be able to andle unknown MIME messsages
too...
3. Then consolidate the functionalities of the similar builders
4. Properly document each and every builder/formatters' purpose in java
doc.. May be we can also list the set of available builders/formatters in a
wiki or in the web site so that users will be aware of them.
IMHO there is good chance of ending up with a mess if we do not do this
now... Also let's discuss and see whether adding the getDataSource method
is useful for most of the builders... If so let's add it to the interface..
IMO it's OK to change API's in a major release, give that we properly
document it..
I think it's up to the release manager to decide whether to take this in or
not for the 1.5... But since this is not a make or break thing, but rather a
refactoring I guess it's fine even if we miss the 1.5...
thanks,
Thilina
On Thu, Dec 18, 2008 at 12:04 PM, Andreas Veithen <andreas.veithen@gmail.com
> wrote:
> See my comments inline.
>
> On Thu, Dec 18, 2008 at 04:55, Thilina Gunarathne <cs...@gmail.com>
> wrote:
> > Hi Andreas,
> > I'm sorry, I missed this mail.. Saw it only now...
> >
> > I agree with you regarding the 1. But I guess the solution will need to
> > address deferred building, which will make it bit complex... Something
> like
> > implementing a pushbackInputStream which will directly give the bytes
> from
> > the transport inputstream while buffering it to give it the next time...
>
> The existing BinaryBuilder uses IOUtils#toByteArray and then
> constructs a DataSource from the byte[]. This avoids the problem that
> UnknownContentBuilder has, but this solution is also clearly
> suboptimal. Your idea sounds very good. I'm sure that a DataSource
> that implements this behavior already exists somewhere in Axiom, Axis2
> or Synapse.
>
> > Regarding 2, I don't think we can call anything "the" right solution for
> > this. Normally Axis2 uses OMDataSources to carry native data as long as
> it
> > can, so that if an entity which knows how to process the native data can
> > take advantage of it.. Also using the OMSourcedElement, clearly
> distinguish
> > the usage of unknown content from other messages...
>
> In the case of binary data, this functionality as already provided by
> DataHandler/DataSource.
hmm..If you are talking about the deffered building, then it depends on the
underlying data source impl...
> UnknownContentOMDataSource only defers the
> construction of the wrapper OMElement and the OMText object that holds
> the reference to the DataHandler, which IMHO is not very useful.
> Actually your argument applies much more to PlainTextBuilder, which
> for the moment uses IOUtils.toString(inputStream, charSetEnc) and then
> creates an OMText node with the returned string. Here it would
> definitely make sense to defer building the tree as much as possible.
> Note that we have code in Synapse that does this (for text output
> generated by an XSL transformation). See TextFileDataSource and
> WrappedTextNodeStreamReader.
>
> > Regarding the 3, my apologies once again... I was not aware of such a
> thing
> > when I wrote the above. IMHO builder should live inside Axis2.. I did
> this
> > (and the mime support) as a solution to the issue raised in Synapse.
> Wonder
> > why they did not simply use the impl you mentioned.... May be I'm missing
> > something.. Let's see how we can combine these efforts...
>
> No need to apologize. With the large volume of code in Axis2 and its
> surrounding projects, probably nobody is able to say whether a given
> piece of functionality already exists somewhere or not. After all
> thats why we spam the dev lists with the Subversion commit logs, so
> that people can review and shout when code is duplicated...
>
> > thanks,
> > Thilina.
> >
> >> 1. The class InputStreamDataSource (the one in
> >> org.apache.axis2.builder.unknowncontent) violates the
> >> javax.activation.DataSource contract which says for the getInputStream
> >> method that "a new InputStream object must be returned each time this
> >> method is called, and [that] the stream must be positioned at the
> >> beginning of the data." The consequence will be that the message
> >> produced by UnknownContentBuilder can only be read once. This is a
> >> serious flaw.
> >>
> >> 2. The AXIOM tree produced by UnknownContentBuilder has only two
> >> nodes: an OMElement and an OMText (with a DataHandler). Using an
> >> OMSourcedElement/OMDataSource is not justified for this and would
> >> introduce unnecessary complexity and overhead.
> >>
> >> 3. The code in UnknownContentBuilder to a large extend duplicates the
> >> code in org.apache.axis2.format.BinaryBuilder (in
> >> axis2-transport-base), which doesn't have problems 1 and 2.
> >>
> >> Could you please make a proposal how to improve this?
> >>
> >> Regards,
> >>
> >> Andreas
> >
> >
> >
> > --
> > Thilina Gunarathne - http://thilinag.blogspot.com
> >
>
--
Thilina Gunarathne - http://thilinag.blogspot.com
Re: [axis2] Cleaning up the builders and documentation [was Re: Comments about the new UnknownContentBuilder (AXIS2-4153)]
Posted by Andreas Veithen <an...@gmail.com>.
I agree that we should not try to push this into the 1.5 release, in
particular because people who might provide some valuable input might
be off during the holiday period. If we start after the release, this
will also gives us ample time to work out a clean solution.
Andreas
On Thu, Dec 18, 2008 at 20:20, Sanjiva Weerawarana
<sa...@opensource.lk> wrote:
> +1 to the plan, but not for pushing this into 1.5.
>
> Sanjiva.
>
> Thilina Gunarathne wrote:
>>
>> Hi guys,
>> May it's because I'm not 100% in touch with the project nowadays, but
>> there seems to be lot of undocumented features/stuff all over the place
>> :(...
>>
>> After the recent conversations in the JIRA's and in the mailing list, I
>> discovered a one hidden feature (wild card support for builders) and many
>> builders who are doing more or less the same thing (unknownContentBuilder,
>> BinaryBuilder in commons tranport, DataSourceBuilder)... I should
>> personally take the blame for not documenting many Axis2 defualt builders
>> :(..
>>
>> How about
>> 1. We bring down the builders in commons/transport to Axis2
>> 2. List down the requirements of these builders
>> eg: UnknownContentbuilder should be able to andle unknown MIME messsages
>> too...
>> 3. Then consolidate the functionalities of the similar builders
>> 4. Properly document each and every builder/formatters' purpose in java
>> doc.. May be we can also list the set of available builders/formatters in a
>> wiki or in the web site so that users will be aware of them.
>>
>> IMHO there is good chance of ending up with a mess if we do not do this
>> now... Also let's discuss and see whether adding the getDataSource method
>> is useful for most of the builders... If so let's add it to the interface..
>> IMO it's OK to change API's in a major release, give that we properly
>> document it..
>>
>> I think it's up to the release manager to decide whether to take this in
>> or not for the 1.5... But since this is not a make or break thing, but
>> rather a refactoring I guess it's fine even if we miss the 1.5...
>>
>> thanks,
>> Thilina
>>
>> On Thu, Dec 18, 2008 at 12:04 PM, Andreas Veithen
>> <andreas.veithen@gmail.com <ma...@gmail.com>> wrote:
>>
>> See my comments inline.
>>
>> On Thu, Dec 18, 2008 at 04:55, Thilina Gunarathne <csethil@gmail.com
>> <ma...@gmail.com>> wrote:
>> > Hi Andreas,
>> > I'm sorry, I missed this mail.. Saw it only now...
>> >
>> > I agree with you regarding the 1. But I guess the solution will
>> need to
>> > address deferred building, which will make it bit complex...
>> Something like
>> > implementing a pushbackInputStream which will directly give the
>> bytes from
>> > the transport inputstream while buffering it to give it the next
>> time...
>>
>> The existing BinaryBuilder uses IOUtils#toByteArray and then
>> constructs a DataSource from the byte[]. This avoids the problem that
>> UnknownContentBuilder has, but this solution is also clearly
>> suboptimal. Your idea sounds very good. I'm sure that a DataSource
>> that implements this behavior already exists somewhere in Axiom, Axis2
>> or Synapse.
>>
>> > Regarding 2, I don't think we can call anything "the" right
>> solution for
>> > this. Normally Axis2 uses OMDataSources to carry native data as
>> long as it
>> > can, so that if an entity which knows how to process the native
>> data can
>> > take advantage of it.. Also using the OMSourcedElement, clearly
>> distinguish
>> > the usage of unknown content from other messages...
>>
>> In the case of binary data, this functionality as already provided by
>> DataHandler/DataSource.
>> hmm..If you are talking about the deffered building, then it depends on
>> the underlying data source impl...
>>
>> UnknownContentOMDataSource only defers the
>> construction of the wrapper OMElement and the OMText object that holds
>> the reference to the DataHandler, which IMHO is not very useful.
>> Actually your argument applies much more to PlainTextBuilder, which
>> for the moment uses IOUtils.toString(inputStream, charSetEnc) and then
>> creates an OMText node with the returned string. Here it would
>> definitely make sense to defer building the tree as much as possible.
>> Note that we have code in Synapse that does this (for text output
>> generated by an XSL transformation). See TextFileDataSource and
>> WrappedTextNodeStreamReader.
>>
>> > Regarding the 3, my apologies once again... I was not aware of
>> such a thing
>> > when I wrote the above. IMHO builder should live inside Axis2.. I
>> did this
>> > (and the mime support) as a solution to the issue raised in
>> Synapse. Wonder
>> > why they did not simply use the impl you mentioned.... May be I'm
>> missing
>> > something.. Let's see how we can combine these efforts...
>>
>> No need to apologize. With the large volume of code in Axis2 and its
>> surrounding projects, probably nobody is able to say whether a given
>> piece of functionality already exists somewhere or not. After all
>> thats why we spam the dev lists with the Subversion commit logs, so
>> that people can review and shout when code is duplicated...
>>
>> > thanks,
>> > Thilina.
>> >
>> >> 1. The class InputStreamDataSource (the one in
>> >> org.apache.axis2.builder.unknowncontent) violates the
>> >> javax.activation.DataSource contract which says for the
>> getInputStream
>> >> method that "a new InputStream object must be returned each time
>> this
>> >> method is called, and [that] the stream must be positioned at the
>> >> beginning of the data." The consequence will be that the message
>> >> produced by UnknownContentBuilder can only be read once. This is a
>> >> serious flaw.
>> >>
>> >> 2. The AXIOM tree produced by UnknownContentBuilder has only two
>> >> nodes: an OMElement and an OMText (with a DataHandler). Using an
>> >> OMSourcedElement/OMDataSource is not justified for this and would
>> >> introduce unnecessary complexity and overhead.
>> >>
>> >> 3. The code in UnknownContentBuilder to a large extend
>> duplicates the
>> >> code in org.apache.axis2.format.BinaryBuilder (in
>> >> axis2-transport-base), which doesn't have problems 1 and 2.
>> >>
>> >> Could you please make a proposal how to improve this?
>> >>
>> >> Regards,
>> >>
>> >> Andreas
>> >
>> >
>> >
>> > --
>> > Thilina Gunarathne - http://thilinag.blogspot.com
>> >
>>
>>
>>
>>
>> --
>> Thilina Gunarathne - http://thilinag.blogspot.com
>
>
> --
> Sanjiva Weerawarana, Ph.D.
> Founder & Director; Lanka Software Foundation; http://www.opensource.lk/
> Founder, Chairman & CEO; WSO2, Inc.; http://www.wso2.com/
> Member; Apache Software Foundation; http://www.apache.org/
> Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/
>
> Blog: http://sanjiva.weerawarana.org/
>
Re: [axis2] Cleaning up the builders and documentation [was Re: Comments
about the new UnknownContentBuilder (AXIS2-4153)]
Posted by Sanjiva Weerawarana <sa...@opensource.lk>.
+1 to the plan, but not for pushing this into 1.5.
Sanjiva.
Thilina Gunarathne wrote:
> Hi guys,
> May it's because I'm not 100% in touch with the project nowadays, but
> there seems to be lot of undocumented features/stuff all over the place
> :(...
>
> After the recent conversations in the JIRA's and in the mailing list, I
> discovered a one hidden feature (wild card support for builders) and
> many builders who are doing more or less the same thing
> (unknownContentBuilder, BinaryBuilder in commons tranport,
> DataSourceBuilder)... I should personally take the blame for not
> documenting many Axis2 defualt builders :(..
>
> How about
> 1. We bring down the builders in commons/transport to Axis2
> 2. List down the requirements of these builders
> eg: UnknownContentbuilder should be able to andle unknown MIME
> messsages too...
> 3. Then consolidate the functionalities of the similar builders
> 4. Properly document each and every builder/formatters' purpose in java
> doc.. May be we can also list the set of available builders/formatters
> in a wiki or in the web site so that users will be aware of them.
>
> IMHO there is good chance of ending up with a mess if we do not do this
> now... Also let's discuss and see whether adding the getDataSource
> method is useful for most of the builders... If so let's add it to the
> interface.. IMO it's OK to change API's in a major release, give that we
> properly document it..
>
> I think it's up to the release manager to decide whether to take this in
> or not for the 1.5... But since this is not a make or break thing, but
> rather a refactoring I guess it's fine even if we miss the 1.5...
>
> thanks,
> Thilina
>
> On Thu, Dec 18, 2008 at 12:04 PM, Andreas Veithen
> <andreas.veithen@gmail.com <ma...@gmail.com>> wrote:
>
> See my comments inline.
>
> On Thu, Dec 18, 2008 at 04:55, Thilina Gunarathne <csethil@gmail.com
> <ma...@gmail.com>> wrote:
> > Hi Andreas,
> > I'm sorry, I missed this mail.. Saw it only now...
> >
> > I agree with you regarding the 1. But I guess the solution will
> need to
> > address deferred building, which will make it bit complex...
> Something like
> > implementing a pushbackInputStream which will directly give the
> bytes from
> > the transport inputstream while buffering it to give it the next
> time...
>
> The existing BinaryBuilder uses IOUtils#toByteArray and then
> constructs a DataSource from the byte[]. This avoids the problem that
> UnknownContentBuilder has, but this solution is also clearly
> suboptimal. Your idea sounds very good. I'm sure that a DataSource
> that implements this behavior already exists somewhere in Axiom, Axis2
> or Synapse.
>
> > Regarding 2, I don't think we can call anything "the" right
> solution for
> > this. Normally Axis2 uses OMDataSources to carry native data as
> long as it
> > can, so that if an entity which knows how to process the native
> data can
> > take advantage of it.. Also using the OMSourcedElement, clearly
> distinguish
> > the usage of unknown content from other messages...
>
> In the case of binary data, this functionality as already provided by
> DataHandler/DataSource.
>
> hmm..If you are talking about the deffered building, then it depends on
> the underlying data source impl...
>
>
> UnknownContentOMDataSource only defers the
> construction of the wrapper OMElement and the OMText object that holds
> the reference to the DataHandler, which IMHO is not very useful.
> Actually your argument applies much more to PlainTextBuilder, which
> for the moment uses IOUtils.toString(inputStream, charSetEnc) and then
> creates an OMText node with the returned string. Here it would
> definitely make sense to defer building the tree as much as possible.
> Note that we have code in Synapse that does this (for text output
> generated by an XSL transformation). See TextFileDataSource and
> WrappedTextNodeStreamReader.
>
> > Regarding the 3, my apologies once again... I was not aware of
> such a thing
> > when I wrote the above. IMHO builder should live inside Axis2.. I
> did this
> > (and the mime support) as a solution to the issue raised in
> Synapse. Wonder
> > why they did not simply use the impl you mentioned.... May be I'm
> missing
> > something.. Let's see how we can combine these efforts...
>
> No need to apologize. With the large volume of code in Axis2 and its
> surrounding projects, probably nobody is able to say whether a given
> piece of functionality already exists somewhere or not. After all
> thats why we spam the dev lists with the Subversion commit logs, so
> that people can review and shout when code is duplicated...
>
> > thanks,
> > Thilina.
> >
> >> 1. The class InputStreamDataSource (the one in
> >> org.apache.axis2.builder.unknowncontent) violates the
> >> javax.activation.DataSource contract which says for the
> getInputStream
> >> method that "a new InputStream object must be returned each time
> this
> >> method is called, and [that] the stream must be positioned at the
> >> beginning of the data." The consequence will be that the message
> >> produced by UnknownContentBuilder can only be read once. This is a
> >> serious flaw.
> >>
> >> 2. The AXIOM tree produced by UnknownContentBuilder has only two
> >> nodes: an OMElement and an OMText (with a DataHandler). Using an
> >> OMSourcedElement/OMDataSource is not justified for this and would
> >> introduce unnecessary complexity and overhead.
> >>
> >> 3. The code in UnknownContentBuilder to a large extend
> duplicates the
> >> code in org.apache.axis2.format.BinaryBuilder (in
> >> axis2-transport-base), which doesn't have problems 1 and 2.
> >>
> >> Could you please make a proposal how to improve this?
> >>
> >> Regards,
> >>
> >> Andreas
> >
> >
> >
> > --
> > Thilina Gunarathne - http://thilinag.blogspot.com
> >
>
>
>
>
> --
> Thilina Gunarathne - http://thilinag.blogspot.com
--
Sanjiva Weerawarana, Ph.D.
Founder & Director; Lanka Software Foundation; http://www.opensource.lk/
Founder, Chairman & CEO; WSO2, Inc.; http://www.wso2.com/
Member; Apache Software Foundation; http://www.apache.org/
Visiting Lecturer; University of Moratuwa; http://www.cse.mrt.ac.lk/
Blog: http://sanjiva.weerawarana.org/