[jira] [Comment Edited] (OAK-5163) ReadWriteVersionManager#getExistingBaseVersion throws an opaque exception if baseVersion doesn't exist

["Vikas Saurabh (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-5163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15701898#comment-15701898 ] 

Vikas Saurabh edited comment on OAK-5163 at 11/28/16 1:04 PM:
--------------------------------------------------------------

The reason I didn't want to add path to exception message because iirc we explicitly removed path information from OakMerge\* exception earlier as exception messages have potential to cause information leak (if it gets trickled back to end user who doesn't have access to look at that path).

May be, we can check for null at both places in {{checkin}} and log versionable at callee's end. [~mreutegg], would that be ok(somethign like \[0])?

\[0]:
{noformat}
diff --git a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/version/ReadWriteVersionManager.java b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/version/ReadWriteVersionManager.java
index c7322e7..23db5c7 100644
--- a/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/version/ReadWriteVersionManager.java
+++ b/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/version/ReadWriteVersionManager.java
@@ -35,6 +35,8 @@ import org.apache.jackrabbit.oak.plugins.nodetype.ReadOnlyNodeTypeManager;
 import org.apache.jackrabbit.oak.plugins.version.ReadOnlyVersionManager;
 import org.apache.jackrabbit.oak.util.TreeUtil;
 import org.apache.jackrabbit.util.ISO8601;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;

 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
@@ -48,6 +50,8 @@ import static org.apache.jackrabbit.JcrConstants.JCR_VERSIONLABELS;
  */
 public class ReadWriteVersionManager extends ReadOnlyVersionManager {

+    private static final Logger LOG = LoggerFactory.getLogger(ReadWriteVersionManager.class);
+
     private final SessionDelegate sessionDelegate;

     private final VersionStorage versionStorage;
@@ -114,8 +118,13 @@ public class ReadWriteVersionManager extends ReadOnlyVersionManager {
             throw new UnsupportedRepositoryOperationException(
                     versionable.getPath() + " is not versionable");
         }
+        Tree baseVersion;
         if (isCheckedOut(versionable)) {
-            Tree baseVersion = getExistingBaseVersion(versionable);
+            try {
+                baseVersion = getExistingBaseVersion(versionable);
+            } finally {
+                LOG.warn("Could not evaluate base version for {}", versionable.getPath());
+            }
             versionable.setProperty(JCR_ISCHECKEDOUT, Boolean.FALSE, Type.BOOLEAN);
             PropertyState created = baseVersion.getProperty(JCR_CREATED);
             if (created != null) {
@@ -133,7 +142,12 @@ public class ReadWriteVersionManager extends ReadOnlyVersionManager {
                 throw e.asRepositoryException();
             }
         }
-        return getExistingBaseVersion(getWorkspaceRoot().getTree(versionable.getPath()));
+        try {
+            baseVersion = getExistingBaseVersion(getWorkspaceRoot().getTree(versionable.getPath()));
+        } finally {
+            LOG.warn("Could not evaluate base version for {}", versionable.getPath());
+        }
+        return baseVersion;
     }
{noformat}


was (Author: catholicon):
The reason I didn't want to add path to exception message because iirc we explicitly removed path information from OakMerge\* exception earlier as exception messages have potential to cause information leak (if it gets trickled back to end user who doesn't have access to look at that path).

May be, we can check for null at both places in {{checkin}} and log versionable at callee's end. [~mreutegg], would that be ok?

> ReadWriteVersionManager#getExistingBaseVersion throws an opaque exception if baseVersion doesn't exist
> ------------------------------------------------------------------------------------------------------
>
>                 Key: OAK-5163
>                 URL: https://issues.apache.org/jira/browse/OAK-5163
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: jcr
>            Reporter: Vikas Saurabh
>            Assignee: Vikas Saurabh
>            Priority: Minor
>             Fix For: 1.6
>
>
> ReadWriteVersionManager#getExistingBaseVersion throws an opaque exception if baseVersion doesn't exist - this makes debugging/identifying the underlying cause very hard. It'd great if it at least logs the path for which this happens.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)