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

        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 ?
        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.
        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
        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.
        Hide
        Pierre-Arnaud Marcelot added a comment -

        Closed.

        Show
        Pierre-Arnaud Marcelot added a comment - Closed.

          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