Directory ApacheDS
  1. Directory ApacheDS
  2. DIRSERVER-1150

No error thrown when removing a non existing value of an attribute

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.1
    • Fix Version/s: 1.5.4
    • Component/s: None
    • Labels:
      None

      Description

      It's possible with Apache DS 1.5.1 and the latest trunk to execute a request where you remove a non existing value of an attribute.
      This request return a SUCCES response.

      I think an Error Response should be returned instead, as OpenLDAP does, indicating that the value does not exist.

        Activity

        Pierre-Arnaud Marcelot created issue -
        Emmanuel Lecharny made changes -
        Field Original Value New Value
        Assignee Emmanuel Lecharny [ elecharny ]
        Emmanuel Lecharny made changes -
        Fix Version/s 1.5.2 [ 12310793 ]
        Hide
        Emmanuel Lecharny added a comment -

        Postponed, as we don't have a clear answer.

        I think that both returns are OK. OpenLDAP returns a noSuchAttribute error, and we consider that it's a Success, as the removed value is removed, all in all

        It's just a question of being consistent.

        i'm just curious about how the other servers deal with this ? Stefan Z ?

        Show
        Emmanuel Lecharny added a comment - Postponed, as we don't have a clear answer. I think that both returns are OK. OpenLDAP returns a noSuchAttribute error, and we consider that it's a Success, as the removed value is removed, all in all It's just a question of being consistent. i'm just curious about how the other servers deal with this ? Stefan Z ?
        Emmanuel Lecharny made changes -
        Fix Version/s 1.5.2 [ 12310793 ]
        Fix Version/s 1.5.3 [ 12312693 ]
        Hide
        Alex Karasulu added a comment -

        Any status on this. Did we agree on the behavior. If so I can get a quick fix out fast.

        Show
        Alex Karasulu added a comment - Any status on this. Did we agree on the behavior. If so I can get a quick fix out fast.
        Emmanuel Lecharny made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Emmanuel Lecharny made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Emmanuel Lecharny made changes -
        Comment [ The following test demonstrates that the userCertificate;binary attribute is working on the server, and that the certificate is not modified.

        There may be a bug in Studio, however.


            public void testAddNewBinaryAttributeValue() throws NamingException
            {
                // Add a binary attribute
                byte[] newValue = new byte[]{0x00, 0x01, 0x02, 0x03};
                Attributes attrs = new AttributesImpl( "userCertificate;binary", newValue );
                ctx.modifyAttributes( RDN_TORI_AMOS, DirContext.ADD_ATTRIBUTE, attrs );

                // Verify, that attribute value is added
                attrs = ctx.getAttributes( RDN_TORI_AMOS );
                Attribute attr = attrs.get( "userCertificate" );
                assertNotNull( attr );
                assertTrue( attr.contains( newValue ) );
                byte[] certificate = (byte[])attr.get();
                assertTrue( Arrays.equals( newValue, certificate ) );
                assertEquals( 1, attr.size() );
            } ]
        Emmanuel Lecharny made changes -
        Comment [ Oops, wrong issue :) ]
        Hide
        Emmanuel Lecharny added a comment -

        I have a fix... But it breaks other part of the server !

        For instance, the ChangeLog interceptor is not anymore happy when trying to revert an operation which failed because an attribute was not existing ( a modify operation where we try to remove a non existing value, for instance).

        I suggest that we postpone this issue to 1.5.4.

        Show
        Emmanuel Lecharny added a comment - I have a fix... But it breaks other part of the server ! For instance, the ChangeLog interceptor is not anymore happy when trying to revert an operation which failed because an attribute was not existing ( a modify operation where we try to remove a non existing value, for instance). I suggest that we postpone this issue to 1.5.4.
        Hide
        Emmanuel Lecharny added a comment -

        Not easy to fix.

        Here are some ideas :

        (01:54:38 AM) elecharny@gmail.com/Gaim: if I check the attribute before deleting it$
        (01:54:47 AM) elecharny@gmail.com/Gaim: and thrown a NoSuchAttribute exception
        (01:54:52 AM) elecharny@gmail.com/Gaim: it works well,
        (01:55:05 AM) elecharny@gmail.com/Gaim: but the changelog is splitting chunks
        (01:55:18 AM) elecharny@gmail.com/Gaim: because we already stored the reverted operation,
        (01:55:25 AM) elecharny@gmail.com/Gaim: assuming that the attribute exists
        (01:55:32 AM) elecharny@gmail.com/Gaim: which is obviously not the case
        (01:55:45 AM) elecharny@gmail.com/Gaim: so when we try to revert to pass to the next test,
        (01:55:48 AM) elecharny@gmail.com/Gaim: kaboom !
        (01:56:14 AM) Alex Karasulu: well then preload the entry in changelog service before making the decision to determine what the proper reverse operation will be
        (01:56:50 AM) elecharny@gmail.com/Gaim: I think we should revert the revert if we have an exception
        (01:56:59 AM) Alex Karasulu: yeah good point
        (01:57:04 AM) Alex Karasulu: we have to
        (01:57:12 AM) Alex Karasulu: that's another thing that we do not do
        (01:57:12 AM) elecharny@gmail.com/Gaim: or that we don't apply the revert if we get an exception
        (01:57:27 AM) Alex Karasulu: we need transactions here
        (01:57:39 AM) elecharny@gmail.com/Gaim: anyway, the modify/add/delete operations have to be reviewed from top to the bottom

        Show
        Emmanuel Lecharny added a comment - Not easy to fix. Here are some ideas : (01:54:38 AM) elecharny@gmail.com/Gaim: if I check the attribute before deleting it$ (01:54:47 AM) elecharny@gmail.com/Gaim: and thrown a NoSuchAttribute exception (01:54:52 AM) elecharny@gmail.com/Gaim: it works well, (01:55:05 AM) elecharny@gmail.com/Gaim: but the changelog is splitting chunks (01:55:18 AM) elecharny@gmail.com/Gaim: because we already stored the reverted operation, (01:55:25 AM) elecharny@gmail.com/Gaim: assuming that the attribute exists (01:55:32 AM) elecharny@gmail.com/Gaim: which is obviously not the case (01:55:45 AM) elecharny@gmail.com/Gaim: so when we try to revert to pass to the next test, (01:55:48 AM) elecharny@gmail.com/Gaim: kaboom ! (01:56:14 AM) Alex Karasulu: well then preload the entry in changelog service before making the decision to determine what the proper reverse operation will be (01:56:50 AM) elecharny@gmail.com/Gaim: I think we should revert the revert if we have an exception (01:56:59 AM) Alex Karasulu: yeah good point (01:57:04 AM) Alex Karasulu: we have to (01:57:12 AM) Alex Karasulu: that's another thing that we do not do (01:57:12 AM) elecharny@gmail.com/Gaim: or that we don't apply the revert if we get an exception (01:57:27 AM) Alex Karasulu: we need transactions here (01:57:39 AM) elecharny@gmail.com/Gaim: anyway, the modify/add/delete operations have to be reviewed from top to the bottom
        Emmanuel Lecharny made changes -
        Fix Version/s 1.5.4 [ 12313147 ]
        Fix Version/s 1.5.3 [ 12312693 ]
        Hide
        Emmanuel Lecharny added a comment -

        So what do we do ?

        IMHO, removing a non existing value from an existing attribute should just be accepted, as it's just a void operation.

        Should we mark this issue as closed ?

        Show
        Emmanuel Lecharny added a comment - So what do we do ? IMHO, removing a non existing value from an existing attribute should just be accepted, as it's just a void operation. Should we mark this issue as closed ?
        Hide
        Alex Karasulu added a comment -

        Sure I think it's fine especially if there are implementation conflicts. The RFCs don't mandate one way or the other so I don't see it as a protocol issue but one of those vague areas.

        Show
        Alex Karasulu added a comment - Sure I think it's fine especially if there are implementation conflicts. The RFCs don't mandate one way or the other so I don't see it as a protocol issue but one of those vague areas.
        Hide
        Emmanuel Lecharny added a comment -

        This does not make a lot of sense to throw an exception because we try to remove a value which does not exist.

        Closed the issue.

        Show
        Emmanuel Lecharny added a comment - This does not make a lot of sense to throw an exception because we try to remove a value which does not exist. Closed the issue.
        Emmanuel Lecharny made changes -
        Resolution Fixed [ 1 ]
        Status Reopened [ 4 ] Resolved [ 5 ]
        Hide
        Pierre-Arnaud Marcelot added a comment -

        Closed.

        Show
        Pierre-Arnaud Marcelot added a comment - Closed.
        Pierre-Arnaud Marcelot made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Emmanuel Lecharny
            Reporter:
            Pierre-Arnaud Marcelot
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development