On 20 February 2015 at 03:57, Rajith Muditha Attapattu
<rajit...@gmail.com> wrote:
>> Most of the things I'd consider most awkward about about the current
>> codebase have little to do with the Message, but rather all the stuff
>> you need before you get that far.
>>
> Changing or improving Message or any of the public API's is not the focus
> for me.
> But while working on the type handling and codec stuff, I was also looking
> closely at how some of these "types" are used throughout the codebase and
> message handling was one such area.
> Given that I found the set method in the Message interface a bit of a
> nuisance to use, and I had to use it fairly often, I took the opportunity
> to start a discussion to see if an improvement could be made.
>

Which is entirely fair enough :)

A new mail thread might have been a good idea though, given its not
codec and is a breaking change to almost everyone.

>
>> Mostly, I find other areas being much more awkard, like many of the
>> types required to set up and operate connections, sessions, links etc
>> being in a bunch of different packages that arent as obvious as they
>> could be, and perhaps even interfaces and implementations with the
>> same name (e.g Source).
>
>
> Yep this has been noted and mentioned by several folks, and an attempt has
> been made to improve this area.
> I've got quite a large chuck on work sitting on my local that I will post
> to my branch soon.
>
>
>> I might have added a body specific interface
>> for set/getBody rather than just using the Section interface, since
>> that permits the other non-body sections.
>>
>>
> +1

It might even be worthwhile doing for 0.9. If im thinking it through
properly, no existing code that produces legal amqp messages would be
affected by such a change, since all 'Body' objects would continue to
be a Section, only the Body type Section objects should ever have been
used with the method, and all the other message sections have explicit
setters.

>
>>
>> > If I want to send a String or a Map, I have to wrap it with an AMQP type
>> > first. Which is awkward at best.
>> >
>>
>> As I said, I can definitely see there also being merit in having an
>> API that you set/get simple 'body objects' such as String with. I just
>> dont think it should be the only way as outlined, particularly since
>> it hasnt been from the start.
>>
>> I think setBody(Object) and Object getBody() is less restrictive than the
> current approach.
> It will certainly allow a user to pass an Object, Data or an AmqpSequence.
>

Its certainly less restrictive as a method signature, and could then
do almost anything as a result yes. It would obviously need better
documented to explain what it did do, which would become less obvious
in some ways and more intuitive in others.

It wasnt clear to me from your first mail that you atually intended to
keep it supporting setting the section types, but I mentioned that
possibility in my initial reply. Whilst I do still see some issues
with the setter the main things for me are probably around the getter
since as mentioned, an "Object getBody()" obviously cant permit the
existing behaviour and the new behaviour without e.g. a toggle to
control which behaviour it gives.

> The impl could simply return an Object if it's an amqp-value and Data or
> AmqpSequence if it's data or sequence.
> Even with the current approach you still need to use the instanceof
> operator to figure out the "type". So no difference there but slightly more
> user friendly.
>

Mixing the two behaviours in the same method as the same time doesnt
seem ideal to me. I'd probably prefer the toggle to do it all one way
or the other, or just separate methods that did one thing well would
probably be clearest for me.

> But the important thing to note here is that the current approach simply
> "restricts" to an argument of type Section, which I think is less user
> friendly.
>
> We don't need to remove the existing get/set methods. I have no problem
> leaving them there and it will save some compile errors.
> But if we add get/set Object it will certainly make them redundant. Just
> saying :)
>
> Would you agree?
>

I dont think we can add a second getObject method as our code wouldnt
compile, never mind the user code :)

I'll admit I wasnt thinking the change through fully either when
commenting in earlier mails, as even just changing to "Object getBody"
would obviously break compilation in many (but annoyingly, far from
all) cases for user code.

>>
>> >
>> >> Doing this would simply some use cases, but also further restrict the
>> >> ability to send the bodies you want or know what you really received,
>> >> e.g is a List an AmqpvValue+List or an AmqpSequence?
>> >
>> >
>> > From a typical application programmers perspective,  a List wrapped in an
>> > AmqpValue or an AmqpSequence makes little difference.
>>
>> It makes a fairly big difference if you e.g want to send sequences.
>>
>
> Looking at the spec. You can send only a single AmqpValue section but one
> or more AmqpSequence or Data sections.
> Unfortunately the current impl doesn't allow more than one sequence or data
> section to be sent which kills that advantage.
> We should implement that.
>

Fixing things for multiple bodies is also something I would like to
see and will need at some point. The setter would be easy, various
options for that, such as making it a vararg which that would keep
working for existing code as well. The getter would need thought,
possibly some sort of 'next body' increment or just a new method to
get them all in a list.


> I don't think I advocated anywhere about limiting what can be sent via the
> Message abstraction.
>
> I guess by getting into a discussion about why a user would care whether a
> single list is sent as a sequence or a value object, I gave the wrong
> impression that we should somehow not allow sending it as a sequence.
> I guess thats where you got the impression that I'm trying to limit what
> can or cannot be sent.
> Apologies for causing any confusion there.
>

In large part, yes. The other part is around what you would do to
return multiple bodies via the single "Object getBody" method, as
automagically combining them isnt necessarily something that I might
want as the receiver, or always as simple as it might sound.

> My point was that for most cases an average user would not care about the
> underlying AMQP type and wouldn't care about whether a single list or a
> byte[] is sent as an amqp-value vs sequence/data.
>
> For those that care they can simply wrap it with the AmqpSequence or Data.
>

As I have said from the start, I do see value in having such
simplicity available for use where it is appropriate. Just so long as
we arent artificially restricting everything to be done that way, and
personally I dont think we need to break existing code to add such
functionality.

>
>> > What the user cares about is that it's a List, not the underlying AMQP
>> type.
>> > Unless I have missed some subtlety, I don't see much of a benefit in
>> > figuring out what the underlying AMQP type is.
>> > If they care about that level of detail, then perhaps they are working at
>> > the wrong level of granularity.
>> > They should instead use the codec API for that.
>> >
>> > Robbie, can you please give us a use case to further explain your point?
>> >
>>
>> I'm mainly getting at that by making the proposed change, we would be
>> saying that the Message API probably wont actually support sending
>> lots of varieties of AMQP message content, or perhaps not receiving
>> them. Sequences allow you to do things that value+list do not, and
>> data allows you to do things that value+binary do not (I can only
>> presume thats part of the reason they both exist?).  The JMS mapping
>> for example uses sequences and data to allow for things that wouldnt
>> be possible using value+list or value+binary. Even if it didnt use
>> them as a matter of course for its basic message types though, it is
>> also a goal to let people send/recv fairly arbitrary AMQP message
>> through the JMS API, which could involve wanting to use them.
>>
>> Again, I'm all for adding a simpler way to do things which people can
>> use where those are sufficient, but it seems weird that it would be
>> made the only way, which would make Message be unable to do reasonable
>> things it can currently do, and will break most existing code over
>> something that isnt particularly hard to use as is, particularly when
>> compared to the rest of the codebase.
>>
>
> I'm fine about not removing the existing methods.
> But want to highlight again that what I proposed isn't restrictive compared
> to what we have today.
> Also what I proposed would make the existing methods redundant.
>
> Removing them will break existing code, but would also give an opportunity
> to simplify some of the code out there ;)
> Many API's do change over the years.
> But I don't care one way or the other. We could leave them as it is.
>
>
>> It also feels a little weird that something [originally] written
>> mostly by the key authors of the protocol, as a toolkit allowing
>> others to leverage the protocol without digging deep into a protocol
>> implementation, wouldnt actually let you use certain basic
>> functionality of the messages without doing so. Perhaps such people
>> should go and use the codec, but will it become similarly hobbled at
>> some point?
>>
>
> You raise a good point. Best answered by the folks who wrote the code, as I
> can only speculate at best ;).
>
>
>>
>> >
>> >> Is <some-object>
>> >> an AmqpValue+Binary, or is it a Data?
>> >
>> >
>> > Again the same question. For 99% of the use cases out there, the user
>> will
>> > only care about the underlying byte[].
>> > Could you pls let us know a use case where such a distinction is useful ?
>> >
>> >
>> >> There is then things like how to
>> >>
>> > support e.g multiple Data and AmqpSequence sections (something it
>> >> should already support but doesnt).
>> >>
>> >> Yea I noticed this was missing. More thoughts on that later.
>> >
>> >
>> >> Robbie
>> >>
>>

Reply via email to