[jitsi-dev] Message.addAttribute function replaces previous instance of attribute


#1

The Message.addAttribute() on addition of an attribute replaces the
prievious instance of the attrabute of same type.

Example:
When a client sends a createPermissionRequest with multiple
XOR-PEER-ADDRESS attributes the
*attributes.put(attribute.getAttributeType(), attribute)*
replaces the previous mapping of a attribute type.
This is important as when processing a message to be further processed for
creating permissions for TURN.
Is this a bug or maybe a wrong interpratation of code?

CreatePermissionRequest TURN rfc :
http://tools.ietf.org/html/rfc5766#section-9.2

···

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#2

Hey Aakash,

Sorry for the late response. See inline:

The Message.addAttribute() on addition of an attribute replaces the
prievious instance of the attrabute of same type.

Example:
When a client sends a createPermissionRequest with multiple
XOR-PEER-ADDRESS attributes the
*attributes.put(attribute.getAttributeType(), attribute)*
replaces the previous mapping of a attribute type.
This is important as when processing a message to be further processed
for creating permissions for TURN.
Is this a bug or maybe a wrong interpratation of code?

CreatePermissionRequest TURN rfc :
http://tools.ietf.org/html/rfc5766#section-9.2

That's a good point and I confirm we don't currently support this. We haven't needed it when acting as a TURN client but it would obviously be necessary now.

What I suggest is that you rename the current addAttribute() to putAttribute() (because that's what it really is), make sure you redirect all existing uses to the new method and then add a new addAttribute() method that actually adds attributes.

It would be best if you could send these in two separate patches.

Does this make sense?

Emil

···

On 01.05.14, 17:04, Aakash Garg wrote:

--
https://jitsi.org


#3

Hi Emil,

For *addAttribute*
The current specification says to only process the first occurrence of an
attribute type for STUN.
http://tools.ietf.org/html/rfc5389#section-15

I think that before putting the attribute we should check the previous
occurrence of the attribute object if it exists(checking by using equals
method) we don't insert if from addAttribute function to remove redundancy
in message.

I think that we should use the following declaration in Message class:
LinkedHashMap<Character, LinkedHashSet<Attribute>> attributes
                                = new
LinkedHashMap<Character, LinkedHashSet<Attribute>>()

We will be creating the linkedHashMap of linkedHashSet
In getAttribute we can return the first element of hashSet of attribute
type (if there are multiple elements) for STUN.

and add getAtttibuteList(attributeType) for TURN.

So for both STUN and TURN can be handled.

If it looks good to you I will implement it along with the put method
mentioned in previous mail.

Also the hashcode function is not overridden for each attribute type doing

···

On Tue, May 6, 2014 at 2:00 PM, Aakash Garg <aakashgargnsit@gmail.com>wrote:

this this would allow faster searching of attributes. Do you want them to
be overridden for each attribute type?

Regards,
Aakash Garg.

On Tue, May 6, 2014 at 11:07 AM, Emil Ivov <emcho@jitsi.org> wrote:

Hey Aakash,

Sorry for the late response. See inline:

On 01.05.14, 17:04, Aakash Garg wrote:

The Message.addAttribute() on addition of an attribute replaces the
prievious instance of the attrabute of same type.

Example:
When a client sends a createPermissionRequest with multiple
XOR-PEER-ADDRESS attributes the
*attributes.put(attribute.getAttributeType(), attribute)*

replaces the previous mapping of a attribute type.
This is important as when processing a message to be further processed
for creating permissions for TURN.
Is this a bug or maybe a wrong interpratation of code?

CreatePermissionRequest TURN rfc :
http://tools.ietf.org/html/rfc5766#section-9.2

That's a good point and I confirm we don't currently support this. We
haven't needed it when acting as a TURN client but it would obviously be
necessary now.

What I suggest is that you rename the current addAttribute() to
putAttribute() (because that's what it really is), make sure you redirect
all existing uses to the new method and then add a new addAttribute()
method that actually adds attributes.

It would be best if you could send these in two separate patches.

Does this make sense?

Emil

--
https://jitsi.org

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#4

Hey Aakash,

We just discussed the various options with Lyubomir. Please find our comments in line.

    Hi Emil,

    For _addAttribute_
    The current specification says to only process the first occurrence
    of an attribute type for STUN.
    http://tools.ietf.org/html/rfc5389#section-15

True. Note however that this is very specific to the STUN use case.

    I think that before putting the attribute we should check the
    previous occurrence of the attribute object if it exists(checking by
    using equals method) we don't insert if from addAttribute function
    to remove redundancy in message.

This would generalise a very specific case. If we want subsequent XOR Mapped Address attributes to be ignored in STUN responses then this should be implemented in the STUN response handling not in the attribute storage.

That said, STUN servers don't generally do this so we can keep this change for later.

    I think that we should use the following declaration in Message class:
    LinkedHashMap<Character, LinkedHashSet<Attribute>> attributes
                                     = new
    LinkedHashMap<Character, LinkedHashSet<Attribute>>()

Sounds good

    We will be creating the linkedHashMap of linkedHashSet
    In getAttribute we can return the first element of hashSet of
    attribute type (if there are multiple elements) for STUN.

and add getAtttibuteList(attributeType) for TURN.

Not for TURN only though. Right? The behaviour should be the same regardless of attribute type.

    So for both STUN and TURN can be handled.

    If it looks good to you I will implement it along with the put
    method mentioned in previous mail.

Speaking of put: make sure that it would always work as a replace, even when there are multiple existing elements.

    Also the hashcode function is not overridden for each attribute type
    doing this this would allow faster searching of attributes. Do you
    want them to be overridden for each attribute type?

Yes this can make sense but please only override hashcode() in places where equals() has been overridden. XorMappedAddressAttribute for example does not override it but AddressAttribute (its parent) does.

Also please try to keep these changes in separate patches.

Cheers,
Emil

···

On 06.05.14, 10:32, Aakash Garg wrote:

On Tue, May 6, 2014 at 2:00 PM, Aakash Garg <aakashgargnsit@gmail.com > <mailto:aakashgargnsit@gmail.com>> wrote:

    Regards,
    Aakash Garg.

    On Tue, May 6, 2014 at 11:07 AM, Emil Ivov <emcho@jitsi.org > <mailto:emcho@jitsi.org>> wrote:

        Hey Aakash,

        Sorry for the late response. See inline:

        On 01.05.14, 17:04, Aakash Garg wrote:

            The Message.addAttribute() on addition of an attribute
            replaces the
            prievious instance of the attrabute of same type.

            Example:
            When a client sends a createPermissionRequest with multiple
            XOR-PEER-ADDRESS attributes the
            *attributes.put(attribute.__getAttributeType(), attribute)*

            replaces the previous mapping of a attribute type.
            This is important as when processing a message to be further
            processed
            for creating permissions for TURN.
            Is this a bug or maybe a wrong interpratation of code?

            CreatePermissionRequest TURN rfc :
            http://tools.ietf.org/html/__rfc5766#section-9.2
            <http://tools.ietf.org/html/rfc5766#section-9.2>

        That's a good point and I confirm we don't currently support
        this. We haven't needed it when acting as a TURN client but it
        would obviously be necessary now.

        What I suggest is that you rename the current addAttribute() to
        putAttribute() (because that's what it really is), make sure you
        redirect all existing uses to the new method and then add a new
        addAttribute() method that actually adds attributes.

        It would be best if you could send these in two separate patches.

        Does this make sense?

        Emil

        --
        https://jitsi.org

    --
    */Aakash Garg/*
    /*Netaji Subhas Institute of Technology (NSIT), Dwarka,*/
    /New Delhi/

--
*/Aakash Garg/*
/*Netaji Subhas Institute of Technology (NSIT), Dwarka,*/
/New Delhi/

--
https://jitsi.org


#5

Sorry for the late reply, I am having practical exams.
Please find patches attached:

putAttribute.patch :

   - replaces the addAttribute to putAttribute occurrence wherever possible.

attributeSetLogic.patch :

   - added logic for multiple occurrence of attributes of a given type
   - add various additional functions for multiple occurrence of patches
   like:
      - removeAttributeFromSet(Attribute attribute)
      - getAttributeSet(char attributeType)
      - and more.
   - added various hashcodes for various attributes.

I tried to keep changes separate but the diff also includes the previous
changes also (I think it needs commit permissions for that or any other way
then please suggest). So the second patch also includes the first one.
Please suggest any changes or improvements.

Also, I always have problems with formatting, it would be nice if you can
give me a formatting template that I can add to my IDE for eclipse and/or
Intellij.
Thanks.

Regards,
Aakash Garg

attributeSetLogic.patch (62.5 KB)

putAttribute.patch (15.7 KB)

···

On Wed, May 7, 2014 at 2:49 PM, Emil Ivov <emcho@jitsi.org> wrote:

Hey Aakash,

We just discussed the various options with Lyubomir. Please find our
comments in line.

On 06.05.14, 10:32, Aakash Garg wrote:

On Tue, May 6, 2014 at 2:00 PM, Aakash Garg <aakashgargnsit@gmail.com >> <mailto:aakashgargnsit@gmail.com>> wrote:

    Hi Emil,

    For _addAttribute_

    The current specification says to only process the first occurrence
    of an attribute type for STUN.
    http://tools.ietf.org/html/rfc5389#section-15

True. Note however that this is very specific to the STUN use case.

     I think that before putting the attribute we should check the

    previous occurrence of the attribute object if it exists(checking by
    using equals method) we don't insert if from addAttribute function
    to remove redundancy in message.

This would generalise a very specific case. If we want subsequent XOR
Mapped Address attributes to be ignored in STUN responses then this should
be implemented in the STUN response handling not in the attribute storage.

That said, STUN servers don't generally do this so we can keep this change
for later.

     I think that we should use the following declaration in Message class:

    LinkedHashMap<Character, LinkedHashSet<Attribute>> attributes
                                     = new
    LinkedHashMap<Character, LinkedHashSet<Attribute>>()

Sounds good

     We will be creating the linkedHashMap of linkedHashSet

    In getAttribute we can return the first element of hashSet of
    attribute type (if there are multiple elements) for STUN.

and add getAtttibuteList(attributeType) for TURN.

Not for TURN only though. Right? The behaviour should be the same
regardless of attribute type.

     So for both STUN and TURN can be handled.

    If it looks good to you I will implement it along with the put
    method mentioned in previous mail.

Speaking of put: make sure that it would always work as a replace, even
when there are multiple existing elements.

     Also the hashcode function is not overridden for each attribute type

    doing this this would allow faster searching of attributes. Do you
    want them to be overridden for each attribute type?

Yes this can make sense but please only override hashcode() in places
where equals() has been overridden. XorMappedAddressAttribute for example
does not override it but AddressAttribute (its parent) does.

Also please try to keep these changes in separate patches.

Cheers,
Emil

    Regards,
    Aakash Garg.

    On Tue, May 6, 2014 at 11:07 AM, Emil Ivov <emcho@jitsi.org >> <mailto:emcho@jitsi.org>> wrote:

        Hey Aakash,

        Sorry for the late response. See inline:

        On 01.05.14, 17:04, Aakash Garg wrote:

            The Message.addAttribute() on addition of an attribute
            replaces the
            prievious instance of the attrabute of same type.

            Example:
            When a client sends a createPermissionRequest with multiple
            XOR-PEER-ADDRESS attributes the
            *attributes.put(attribute.__getAttributeType(), attribute)*

            replaces the previous mapping of a attribute type.
            This is important as when processing a message to be further
            processed
            for creating permissions for TURN.
            Is this a bug or maybe a wrong interpratation of code?

            CreatePermissionRequest TURN rfc :
            http://tools.ietf.org/html/__rfc5766#section-9.2

            <http://tools.ietf.org/html/rfc5766#section-9.2>

        That's a good point and I confirm we don't currently support
        this. We haven't needed it when acting as a TURN client but it
        would obviously be necessary now.

        What I suggest is that you rename the current addAttribute() to
        putAttribute() (because that's what it really is), make sure you
        redirect all existing uses to the new method and then add a new
        addAttribute() method that actually adds attributes.

        It would be best if you could send these in two separate patches.

        Does this make sense?

        Emil

        --
        https://jitsi.org

    --
    */Aakash Garg/*
    /*Netaji Subhas Institute of Technology (NSIT), Dwarka,*/
    /New Delhi/

--
*/Aakash Garg/*
/*Netaji Subhas Institute of Technology (NSIT), Dwarka,*/
/New Delhi/

--
https://jitsi.org

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#6

Updated 'attributeSetLogic' patch with formatting.
Other one is already formatted as I see it.

Please suggest any changes to be made.

Regards,
Aakash Garg

attributeSetLogic.patch (64.1 KB)

···

On Sat, May 10, 2014 at 4:19 PM, Aakash Garg <aakashgargnsit@gmail.com>wrote:

Sorry for the late reply, I am having practical exams.
Please find patches attached:

putAttribute.patch :

   - replaces the addAttribute to putAttribute occurrence wherever
   possible.

attributeSetLogic.patch :

   - added logic for multiple occurrence of attributes of a given type
   - add various additional functions for multiple occurrence of patches
   like:
      - removeAttributeFromSet(Attribute attribute)
      - getAttributeSet(char attributeType)
      - and more.
   - added various hashcodes for various attributes.

I tried to keep changes separate but the diff also includes the previous
changes also (I think it needs commit permissions for that or any other way
then please suggest). So the second patch also includes the first one.
Please suggest any changes or improvements.

Also, I always have problems with formatting, it would be nice if you can
give me a formatting template that I can add to my IDE for eclipse and/or
Intellij.
Thanks.

Regards,
Aakash Garg

On Wed, May 7, 2014 at 2:49 PM, Emil Ivov <emcho@jitsi.org> wrote:

Hey Aakash,

We just discussed the various options with Lyubomir. Please find our
comments in line.

On 06.05.14, 10:32, Aakash Garg wrote:

On Tue, May 6, 2014 at 2:00 PM, Aakash Garg <aakashgargnsit@gmail.com >>> <mailto:aakashgargnsit@gmail.com>> wrote:

    Hi Emil,

    For _addAttribute_

    The current specification says to only process the first occurrence
    of an attribute type for STUN.
    http://tools.ietf.org/html/rfc5389#section-15

True. Note however that this is very specific to the STUN use case.

     I think that before putting the attribute we should check the

    previous occurrence of the attribute object if it exists(checking by
    using equals method) we don't insert if from addAttribute function
    to remove redundancy in message.

This would generalise a very specific case. If we want subsequent XOR
Mapped Address attributes to be ignored in STUN responses then this should
be implemented in the STUN response handling not in the attribute storage.

That said, STUN servers don't generally do this so we can keep this
change for later.

     I think that we should use the following declaration in Message

class:
    LinkedHashMap<Character, LinkedHashSet<Attribute>> attributes
                                     = new
    LinkedHashMap<Character, LinkedHashSet<Attribute>>()

Sounds good

     We will be creating the linkedHashMap of linkedHashSet

    In getAttribute we can return the first element of hashSet of
    attribute type (if there are multiple elements) for STUN.

and add getAtttibuteList(attributeType) for TURN.

Not for TURN only though. Right? The behaviour should be the same
regardless of attribute type.

     So for both STUN and TURN can be handled.

    If it looks good to you I will implement it along with the put
    method mentioned in previous mail.

Speaking of put: make sure that it would always work as a replace, even
when there are multiple existing elements.

     Also the hashcode function is not overridden for each attribute type

    doing this this would allow faster searching of attributes. Do you
    want them to be overridden for each attribute type?

Yes this can make sense but please only override hashcode() in places
where equals() has been overridden. XorMappedAddressAttribute for example
does not override it but AddressAttribute (its parent) does.

Also please try to keep these changes in separate patches.

Cheers,
Emil

    Regards,
    Aakash Garg.

    On Tue, May 6, 2014 at 11:07 AM, Emil Ivov <emcho@jitsi.org >>> <mailto:emcho@jitsi.org>> wrote:

        Hey Aakash,

        Sorry for the late response. See inline:

        On 01.05.14, 17:04, Aakash Garg wrote:

            The Message.addAttribute() on addition of an attribute
            replaces the
            prievious instance of the attrabute of same type.

            Example:
            When a client sends a createPermissionRequest with multiple
            XOR-PEER-ADDRESS attributes the
            *attributes.put(attribute.__getAttributeType(), attribute)*

            replaces the previous mapping of a attribute type.
            This is important as when processing a message to be further
            processed
            for creating permissions for TURN.
            Is this a bug or maybe a wrong interpratation of code?

            CreatePermissionRequest TURN rfc :
            http://tools.ietf.org/html/__rfc5766#section-9.2

            <http://tools.ietf.org/html/rfc5766#section-9.2>

        That's a good point and I confirm we don't currently support
        this. We haven't needed it when acting as a TURN client but it
        would obviously be necessary now.

        What I suggest is that you rename the current addAttribute() to
        putAttribute() (because that's what it really is), make sure you
        redirect all existing uses to the new method and then add a new
        addAttribute() method that actually adds attributes.

        It would be best if you could send these in two separate patches.

        Does this make sense?

        Emil

        --
        https://jitsi.org

    --
    */Aakash Garg/*
    /*Netaji Subhas Institute of Technology (NSIT), Dwarka,*/
    /New Delhi/

--
*/Aakash Garg/*
/*Netaji Subhas Institute of Technology (NSIT), Dwarka,*/
/New Delhi/

--
https://jitsi.org

--
*Aakash Garg*

*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*

--
*Aakash Garg*
*Netaji Subhas Institute of Technology (NSIT), Dwarka,*
*New Delhi*


#7

Hi Aakash,

···

On Sat, May 10, 2014 at 12:49 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

attributeSetLogic.patch :

added logic for multiple occurrence of attributes of a given type
add various additional functions for multiple occurrence of patches like:

removeAttributeFromSet(Attribute attribute)
getAttributeSet(char attributeType)
and more.

I don't like the idea of having two collections of attributes and
trying to keep them in sync. Is it possible to use only
"attributesSetMap" ?

Regards,
Pawel


#8

Hi Aakash,

···

On Sat, May 10, 2014 at 9:15 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

On Sat, May 10, 2014 at 4:19 PM, Aakash Garg <aakashgargnsit@gmail.com> > wrote:

Sorry for the late reply, I am having practical exams.
Please find patches attached:

putAttribute.patch :

replaces the addAttribute to putAttribute occurrence wherever possible.

The patch does not rename all occurences of "addAttribute". Is it
intended ? Why not just rename this method with IDE refactor function
and then only use "add" where it's really needed ?

Regards,
Pawel


#9

Hi Aakash,

···

On Tue, May 27, 2014 at 10:13 PM, Paweł Domas <pawel.domas@jitsi.org> wrote:

Hi Aakash,

On Sat, May 10, 2014 at 12:49 PM, Aakash Garg <aakashgargnsit@gmail.com> wrote:

attributeSetLogic.patch :

added logic for multiple occurrence of attributes of a given type
add various additional functions for multiple occurrence of patches like:

removeAttributeFromSet(Attribute attribute)
getAttributeSet(char attributeType)
and more.

I don't like the idea of having two collections of attributes and
trying to keep them in sync. Is it possible to use only
"attributesSetMap" ?

Also getDataLength does not take into an account multiple attributes
of the same type.

Regards,
Pawel