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

Versioning fixup leaves persistence in a state where the node can't be made versionable again

    Details

      Description

      Jackrabbit's version recovery mode (org.apache.jackrabbit.version.recovery system property) disconnects all version histories that expose problems that manifest in unexpected exceptions being thrown. "disconnects" means removing the properties defined for mix:versionable and removing the mixin type. The actual versioning related nodes remain in place.

      The problem: when re-adding mix:versionable, ItemSaveOperation.initVersionHistories tries to create the new version history in the same location (the path being derived from the versionable node's identifier), and consequently fails because of the broken underlying storage.

      (attaching a work-in-progress test case that illustrates the problem)

      1. JCR-3115.patch
        19 kB
        Julian Reschke
      2. JCR-3115.patch
        18 kB
        Julian Reschke
      3. JCR-3115.patch
        17 kB
        Julian Reschke
      4. JCR-3115.patch
        15 kB
        Julian Reschke
      5. AutoFixCorruptNode.java
        7 kB
        Julian Reschke

        Issue Links

          Activity

          Hide
          Julian Reschke added a comment -

          Modify checker to also inspect "candidate" version histories (trunk: 1187344, 2.2: 1187345). Test case (trunk only): 1187354.

          Show
          Julian Reschke added a comment - Modify checker to also inspect "candidate" version histories (trunk: 1187344, 2.2: 1187345). Test case (trunk only): 1187354.
          Hide
          Julian Reschke added a comment -

          Changed RepositoryChecker to obtain the VersionHistoryInfo first, in order to match more closely what other components do when enabling versioning (trunk: 1186802, 2.2: 1186294)

          Show
          Julian Reschke added a comment - Changed RepositoryChecker to obtain the VersionHistoryInfo first, in order to match more closely what other components do when enabling versioning (trunk: 1186802, 2.2: 1186294)
          Hide
          Julian Reschke added a comment -

          Additional fix in trunk (r1186285) and 2.2 (r1186294).

          Show
          Julian Reschke added a comment - Additional fix in trunk (r1186285) and 2.2 (r1186294).
          Hide
          Julian Reschke added a comment -

          trunk: 1185691 and 1185692 (test case)
          2.2: 1185711

          Show
          Julian Reschke added a comment - trunk: 1185691 and 1185692 (test case) 2.2: 1185711
          Hide
          Julian Reschke added a comment -

          > What happens if there are more than one children of a given version history parent need to be renamed? The current code seems to always generate a new modified parent node state.

          Now re-using state from ChangeLog when present (please have a look)

          > Can we make all InconsistentVersioningState instances carry the version history ID? I'd remove all the constructors without the ID argument.

          Not all of them; for instance we throw it when the VH node is missing in which case we do not have a ID .

          > Instead of the instanceof InternalVersionManagerImpl check, I'd rather simply change the RepositoryChecker constructor to always take a InternalVersionManagerImpl instance.

          Done.

          > Instead of the instanceof InconsistentVersioningState check, I'd have a separate catch block for such exceptions.

          Done.

          > IIRC Calendar.getInstance() should already reflect the current time. No need for the setTimeInMillis(System.currentTimeMillis()) call.

          Done.

          > The e.printStackTrace() call should be dropped.

          Was duplicated from existing code. Refactored to avoid code duplication.

          Also now uses the proper nodeId for renaming (the node's ID, not the VH's ID), and tests that.

          Show
          Julian Reschke added a comment - > What happens if there are more than one children of a given version history parent need to be renamed? The current code seems to always generate a new modified parent node state. Now re-using state from ChangeLog when present (please have a look) > Can we make all InconsistentVersioningState instances carry the version history ID? I'd remove all the constructors without the ID argument. Not all of them; for instance we throw it when the VH node is missing in which case we do not have a ID . > Instead of the instanceof InternalVersionManagerImpl check, I'd rather simply change the RepositoryChecker constructor to always take a InternalVersionManagerImpl instance. Done. > Instead of the instanceof InconsistentVersioningState check, I'd have a separate catch block for such exceptions. Done. > IIRC Calendar.getInstance() should already reflect the current time. No need for the setTimeInMillis(System.currentTimeMillis()) call. Done. > The e.printStackTrace() call should be dropped. Was duplicated from existing code. Refactored to avoid code duplication. Also now uses the proper nodeId for renaming (the node's ID, not the VH's ID), and tests that.
          Hide
          Jukka Zitting added a comment -

          Looks good in general. Some comments in decreasing order of importance:

          • What happens if there are more than one children of a given version history parent need to be renamed? The current code seems to always generate a new modified parent node state.
          • Can we make all InconsistentVersioningState instances carry the version history ID? I'd remove all the constructors without the ID argument.
          • Instead of the instanceof InternalVersionManagerImpl check, I'd rather simply change the RepositoryChecker constructor to always take a InternalVersionManagerImpl instance.
          • Instead of the instanceof InconsistentVersioningState check, I'd have a separate catch block for such exceptions.
          • IIRC Calendar.getInstance() should already reflect the current time. No need for the setTimeInMillis(System.currentTimeMillis()) call.
          • The e.printStackTrace() call should be dropped.
          Show
          Jukka Zitting added a comment - Looks good in general. Some comments in decreasing order of importance: What happens if there are more than one children of a given version history parent need to be renamed? The current code seems to always generate a new modified parent node state. Can we make all InconsistentVersioningState instances carry the version history ID? I'd remove all the constructors without the ID argument. Instead of the instanceof InternalVersionManagerImpl check, I'd rather simply change the RepositoryChecker constructor to always take a InternalVersionManagerImpl instance. Instead of the instanceof InconsistentVersioningState check, I'd have a separate catch block for such exceptions. IIRC Calendar.getInstance() should already reflect the current time. No need for the setTimeInMillis(System.currentTimeMillis()) call. The e.printStackTrace() call should be dropped.
          Hide
          Julian Reschke added a comment -

          Updated patch: exception cleaned up, new constructor used in more places, RepositoryChecker extended so the VHR renaming occurs in more situations

          Show
          Julian Reschke added a comment - Updated patch: exception cleaned up, new constructor used in more places, RepositoryChecker extended so the VHR renaming occurs in more situations
          Hide
          Julian Reschke added a comment -

          1) adds test case
          2) augments InconsistentVersioningState with information about the VHR's nodeId
          3) enables RepositoryChecker to rename the VHR in place

          Show
          Julian Reschke added a comment - 1) adds test case 2) augments InconsistentVersioningState with information about the VHR's nodeId 3) enables RepositoryChecker to rename the VHR in place
          Hide
          Julian Reschke added a comment -

          r1185228: added test case for breaking and fixing version storage (also add hook for running consistency check in "fix" mode)

          So at least some types of broken versioning PM can be fixed by the bundle consistency check sufficiently to reenable versioning.

          Show
          Julian Reschke added a comment - r1185228: added test case for breaking and fixing version storage (also add hook for running consistency check in "fix" mode) So at least some types of broken versioning PM can be fixed by the bundle consistency check sufficiently to reenable versioning.
          Hide
          Julian Reschke added a comment -

          A possible approach to work-around this issue would be to change the version cleanup to move the broken history to a different path.

          Show
          Julian Reschke added a comment - A possible approach to work-around this issue would be to change the version cleanup to move the broken history to a different path.
          Hide
          Julian Reschke added a comment -

          w-i-p test case, based on Alex' earlier work.

          Show
          Julian Reschke added a comment - w-i-p test case, based on Alex' earlier work.

            People

            • Assignee:
              Julian Reschke
              Reporter:
              Julian Reschke
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development