Jackrabbit Content Repository
  1. Jackrabbit Content Repository
  2. JCR-2740

On missing child node, automatically remove the entry (when a session attribute is set)

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.2
    • Component/s: jackrabbit-core
    • Labels:
      None

      Description

      If a node points to a non-existing child node (which is a repository inconsistency), currently this child node is silently ignored for read operations (as far as I can tell). However, when trying to add another child node with the same name, an exception is thrown with a message saying a child node with this name already exists. Also, the parent node can't be removed.

      One solution is to remove the bad child node entry, but only if the session attribute "org.apache.jackrabbit.autoFixCorruptions" is set (so by default the repository is not changed 'secretly'):

      SimpleCredentials cred = new SimpleCredentials(...);
      cred.setAttribute("org.apache.jackrabbit.autoFixCorruptions", "true");
      rep.login(cred);

      It's not a perfect solution, but it might be better than throwing an exception and basically preventing changes.

      Another solution (not implemented) would be to rename the missing child node entry when trying to add a child node with the same name (for example add the current date/time, or a random digit until there is no conflict), and then continue with adding the new child node.

      1. jcr-2740.patch
        7 kB
        Thomas Mueller
      2. jcr-2740-b.patch
        7 kB
        Thomas Mueller
      3. jcr-2740-c.patch
        17 kB
        Thomas Mueller
      4. JCR-2740-d.patch
        18 kB
        Stefan Guggisberg

        Activity

        Hide
        Stefan Guggisberg added a comment -

        fixed some copy&paste errors,
        +1 for JCR-2740-d.patch

        thanks!

        Show
        Stefan Guggisberg added a comment - fixed some copy&paste errors, +1 for JCR-2740 -d.patch thanks!
        Hide
        Thomas Mueller added a comment -

        New patch (should now be correct).

        Show
        Thomas Mueller added a comment - New patch (should now be correct).
        Hide
        Stefan Guggisberg added a comment -

        +1 for jcr-2740-b.patch in general, thanks!

        some minor concerns:

        • i'd rather use getters instead of direct field access.
          we could avoid direct field access by passing a
          SessionContext instance instead of ItemManager
          in the LazyItemIterator constructor.
        • instead of printing a stacktrace directly to System.err
          i'd prefer to log the error
        Show
        Stefan Guggisberg added a comment - +1 for jcr-2740-b.patch in general, thanks! some minor concerns: i'd rather use getters instead of direct field access. we could avoid direct field access by passing a SessionContext instance instead of ItemManager in the LazyItemIterator constructor. instead of printing a stacktrace directly to System.err i'd prefer to log the error
        Hide
        Thomas Mueller added a comment -

        This new patch only fixes the problem when a session attribute is set.

        Instead of the ItemManager, it uses the ItemStateManager to check for node existence.

        The new patch fixes the problem when iterating over child nodes, and when trying to remove a node. That means, it's possible to fix all problems in a subtree by iterating over it with a session that has this session attribute set.

        P.S. I didn't fully test this new patch yet.

        Show
        Thomas Mueller added a comment - This new patch only fixes the problem when a session attribute is set. Instead of the ItemManager, it uses the ItemStateManager to check for node existence. The new patch fixes the problem when iterating over child nodes, and when trying to remove a node. That means, it's possible to fix all problems in a subtree by iterating over it with a session that has this session attribute set. P.S. I didn't fully test this new patch yet.
        Hide
        Stefan Guggisberg added a comment -

        thanks for the patch, a few comments:

        • i like the idea of automatically fixing inconsistencies when they're encountered;
          however, i am a bit concerned that the corresponding warnings in the log
          don't get noticed and the user might be unaware of a repository becoming gradually
          corrupt. i suggest to control the proposed behavior through a flag (e.g. a system property),
          and that this feature is disabled by default.
        • there's a potential problem in the patched addNode() method.

        + // loop until we either find an existing child node,
        + // or we are sure no such child node exists
        + ChildNodeEntry cne = thisState.getChildNodeEntry(nodeName, 1);
        + existing = null;
        + if (cne != null) {
        + try

        { + existing = itemMgr.getNode(cne.getId(), getNodeId()); + }

        catch (ItemNotFoundException e)

        { + // the item was removed in the persistence manager, + // which is a repository inconsistency - there is nothing + // we can do, so the child node entry is removed now + log.warn("Node " + getPath() + "/" + cne.getName() + " " + + "(index " + cne.getIndex() + ", id " + cne.getId() + + "parent id " + getId() + ") " + + "not found; removing", e); + removeChildNode(cne.getId()); + continue; + }

        + }

        the existence is checked through the ItemManager which
        depends on access control, i.e. an ItemNotFoundException might
        be thrown because the current session is not allowed to read
        that particular node...

        Show
        Stefan Guggisberg added a comment - thanks for the patch, a few comments: i like the idea of automatically fixing inconsistencies when they're encountered; however, i am a bit concerned that the corresponding warnings in the log don't get noticed and the user might be unaware of a repository becoming gradually corrupt. i suggest to control the proposed behavior through a flag (e.g. a system property), and that this feature is disabled by default. there's a potential problem in the patched addNode() method. + // loop until we either find an existing child node, + // or we are sure no such child node exists + ChildNodeEntry cne = thisState.getChildNodeEntry(nodeName, 1); + existing = null; + if (cne != null) { + try { + existing = itemMgr.getNode(cne.getId(), getNodeId()); + } catch (ItemNotFoundException e) { + // the item was removed in the persistence manager, + // which is a repository inconsistency - there is nothing + // we can do, so the child node entry is removed now + log.warn("Node " + getPath() + "/" + cne.getName() + " " + + "(index " + cne.getIndex() + ", id " + cne.getId() + + "parent id " + getId() + ") " + + "not found; removing", e); + removeChildNode(cne.getId()); + continue; + } + } the existence is checked through the ItemManager which depends on access control, i.e. an ItemNotFoundException might be thrown because the current session is not allowed to read that particular node...
        Hide
        Thomas Mueller added a comment -

        Could those that are familiar with NodeImpl please have a look at the changes and provide feedback? I know it's not a "real" unit test case yet (I will change that later on).

        The solution doesn't try to solve all possible edge cases. It's just a simple solution for the most common problems. For edge cases we can use the existing consistency check and fix. This is just for the most common cases, for old repositories where it was relatively easy to get corruptions. I believe with more recent versions of Jackrabbit it's very hard to get corruptions. At least, that's the plan - if it's not the case yet then I believe we should spend most of our time in hardening Jackrabbit, and not in automatically fixing problems

        Show
        Thomas Mueller added a comment - Could those that are familiar with NodeImpl please have a look at the changes and provide feedback? I know it's not a "real" unit test case yet (I will change that later on). The solution doesn't try to solve all possible edge cases. It's just a simple solution for the most common problems. For edge cases we can use the existing consistency check and fix. This is just for the most common cases, for old repositories where it was relatively easy to get corruptions. I believe with more recent versions of Jackrabbit it's very hard to get corruptions. At least, that's the plan - if it's not the case yet then I believe we should spend most of our time in hardening Jackrabbit, and not in automatically fixing problems
        Hide
        Thomas Mueller added a comment -

        I think it doesn't make much sense to move the broken child node reference, because it's not a node (the node itself doesn't exists). I guess we need to add all relevant test cases so existing features don't break.

        Show
        Thomas Mueller added a comment - I think it doesn't make much sense to move the broken child node reference , because it's not a node (the node itself doesn't exists). I guess we need to add all relevant test cases so existing features don't break.
        Hide
        Jukka Zitting added a comment -

        This could cause unexpected problems with stuff like node type validation. How about simply logging all relevant information at a high enough log level (WARN) or perhaps moving the broken child node reference to somewhere like the /jcr:system/lost+found tree that was proposed already earlier?

        Show
        Jukka Zitting added a comment - This could cause unexpected problems with stuff like node type validation. How about simply logging all relevant information at a high enough log level (WARN) or perhaps moving the broken child node reference to somewhere like the /jcr:system/lost+found tree that was proposed already earlier?

          People

          • Assignee:
            Thomas Mueller
            Reporter:
            Thomas Mueller
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development