You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by re...@apache.org on 2012/05/04 17:52:35 UTC
svn commit: r1334046 - in
/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr:
NodeDelegate.java NodeImpl.java SessionDelegate.java
Author: reschke
Date: Fri May 4 15:52:35 2012
New Revision: 1334046
URL: http://svn.apache.org/viewvc?rev=1334046&view=rev
Log:
avoid NPEs, add minimal checks in move operations
Modified:
jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeDelegate.java
jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeImpl.java
jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/SessionDelegate.java
Modified: jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeDelegate.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeDelegate.java?rev=1334046&r1=1334045&r2=1334046&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeDelegate.java (original)
+++ jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeDelegate.java Fri May 4 15:52:35 2012
@@ -65,8 +65,8 @@ public class NodeDelegate extends ItemDe
return getTree().getName();
}
- Status getNodeStatus() {
- return getTree().getParent().getChildStatus(getName());
+ Status getNodeStatus() throws InvalidItemStateException {
+ return check(getTree().getParent()).getChildStatus(getName());
}
NodeDelegate getNodeOrNull(String relOakPath) {
Modified: jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeImpl.java?rev=1334046&r1=1334045&r2=1334046&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeImpl.java (original)
+++ jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/NodeImpl.java Fri May 4 15:52:35 2012
@@ -31,6 +31,7 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import javax.jcr.Binary;
+import javax.jcr.InvalidItemStateException;
import javax.jcr.Item;
import javax.jcr.ItemNotFoundException;
import javax.jcr.ItemVisitor;
@@ -103,7 +104,11 @@ public class NodeImpl extends ItemImpl i
*/
@Override
public boolean isNew() {
- return dlg.getNodeStatus() == Status.NEW;
+ try {
+ return dlg.getNodeStatus() == Status.NEW;
+ } catch (InvalidItemStateException ex) {
+ return false;
+ }
}
/**
@@ -111,7 +116,11 @@ public class NodeImpl extends ItemImpl i
*/
@Override
public boolean isModified() {
- return dlg.getNodeStatus() == Status.MODIFIED;
+ try {
+ return dlg.getNodeStatus() == Status.MODIFIED;
+ } catch (InvalidItemStateException ex) {
+ return false;
+ }
}
/**
Modified: jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/SessionDelegate.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/SessionDelegate.java?rev=1334046&r1=1334045&r2=1334046&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/SessionDelegate.java (original)
+++ jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/SessionDelegate.java Fri May 4 15:52:35 2012
@@ -16,6 +16,19 @@
*/
package org.apache.jackrabbit.oak.jcr;
+import java.io.IOException;
+
+import javax.jcr.ItemExistsException;
+import javax.jcr.NamespaceRegistry;
+import javax.jcr.PathNotFoundException;
+import javax.jcr.Repository;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+import javax.jcr.Workspace;
+import javax.jcr.lock.LockManager;
+import javax.jcr.nodetype.NodeTypeManager;
+import javax.jcr.version.VersionManager;
+
import org.apache.jackrabbit.oak.api.AuthInfo;
import org.apache.jackrabbit.oak.api.CommitFailedException;
import org.apache.jackrabbit.oak.api.ContentSession;
@@ -32,16 +45,6 @@ import org.apache.jackrabbit.oak.namepat
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import javax.jcr.NamespaceRegistry;
-import javax.jcr.Repository;
-import javax.jcr.RepositoryException;
-import javax.jcr.Session;
-import javax.jcr.Workspace;
-import javax.jcr.lock.LockManager;
-import javax.jcr.nodetype.NodeTypeManager;
-import javax.jcr.version.VersionManager;
-import java.io.IOException;
-
public class SessionDelegate {
static final Logger log = LoggerFactory.getLogger(SessionDelegate.class);
@@ -158,6 +161,28 @@ public class SessionDelegate {
String srcPath = PathUtils.relativize("/", srcAbsPath); // TODO: is this needed?
String destPath = PathUtils.relativize("/", destAbsPath);
+
+ // TODO: do the checks below belong here?
+
+ // check destination
+ Tree dest = getTree(destPath);
+ if (dest != null) {
+ throw new ItemExistsException(destPath);
+ }
+
+ // check parent of destination
+ String destParentPath = PathUtils.getParentPath(destPath);
+ Tree destParent = getTree(destParentPath);
+ if (destParent == null) {
+ throw new PathNotFoundException(destParentPath);
+ }
+
+ // check source exists
+ Tree src = getTree(srcPath);
+ if (src == null) {
+ throw new PathNotFoundException(srcPath);
+ }
+
try {
if (transientOp) {
root.move(srcPath, destPath);
Re: svn commit: r1334046 - in /jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr:
NodeDelegate.java NodeImpl.java SessionDelegate.java
Posted by Michael Dürig <md...@apache.org>.
I'm not too much concerned about performance at this point but rather
about composability. The delegate classes are are going to be used by a
lot of internal clients. These shouldn't be forced to catch exceptions
on their normal paths of execution.
Michael
On 7.5.12 8:45, Thomas Mueller wrote:
> Hi,
>
> Never mind, Michael already fixed it.
>
> Regards,
> Thomas
>
> On 5/7/12 9:38 AM, "Thomas Mueller"<mu...@adobe.com> wrote:
>
>> Hi,
>>
>>> This was the quickest way to fill a gap;
>>
>> Sure no problem.
>>
>>> feel free to refactor so it's
>>> not needed. But keep in mind the old saying about premature
>>> optimizations :-).
>>
>> True. I will currently just add a TODO comment
>>
>> Regards,
>> Thomas
>>
>
Re: svn commit: r1334046 - in
/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr:
NodeDelegate.java NodeImpl.java SessionDelegate.java
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
Never mind, Michael already fixed it.
Regards,
Thomas
On 5/7/12 9:38 AM, "Thomas Mueller" <mu...@adobe.com> wrote:
>Hi,
>
>>This was the quickest way to fill a gap;
>
>Sure no problem.
>
>> feel free to refactor so it's
>>not needed. But keep in mind the old saying about premature
>>optimizations :-).
>
>True. I will currently just add a TODO comment
>
>Regards,
>Thomas
>
Re: svn commit: r1334046 - in
/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr:
NodeDelegate.java NodeImpl.java SessionDelegate.java
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>This was the quickest way to fill a gap;
Sure no problem.
> feel free to refactor so it's
>not needed. But keep in mind the old saying about premature
>optimizations :-).
True. I will currently just add a TODO comment
Regards,
Thomas
Re: svn commit: r1334046 - in /jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr:
NodeDelegate.java NodeImpl.java SessionDelegate.java
Posted by Julian Reschke <ju...@gmx.de>.
On 2012-05-07 09:08, Thomas Mueller wrote:
> Hi,
>
>> + try {
>> + return dlg.getNodeStatus() == Status.MODIFIED;
>> + } catch (InvalidItemStateException ex) {
>> + return false;
>> + }
>
> I would rather not use exception handling for flow control. One reason is
> that throwing exceptions is very slow (the fillStackTrace() method is
> slow).
>
> Is there an alternative to the try ... catch, or is the exception only
> thrown in very rare edge cases?
> ...
This was the quickest way to fill a gap; feel free to refactor so it's
not needed. But keep in mind the old saying about premature
optimizations :-).
Best regards, Julian
Re: svn commit: r1334046 - in
/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr:
NodeDelegate.java NodeImpl.java SessionDelegate.java
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
> + try {
> + return dlg.getNodeStatus() == Status.MODIFIED;
> + } catch (InvalidItemStateException ex) {
> + return false;
> + }
I would rather not use exception handling for flow control. One reason is
that throwing exceptions is very slow (the fillStackTrace() method is
slow).
Is there an alternative to the try ... catch, or is the exception only
thrown in very rare edge cases?
Regards,
Thomas
On 5/4/12 5:52 PM, "reschke@apache.org" <re...@apache.org> wrote:
>Author: reschke
>Date: Fri May 4 15:52:35 2012
>New Revision: 1334046
>
>URL: http://svn.apache.org/viewvc?rev=1334046&view=rev
>Log:
>avoid NPEs, add minimal checks in move operations
>
>Modified:
>
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeDelegate.java
>
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeImpl.java
>
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/S
>essionDelegate.java
>
>Modified:
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeDelegate.java
>URL:
>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/or
>g/apache/jackrabbit/oak/jcr/NodeDelegate.java?rev=1334046&r1=1334045&r2=13
>34046&view=diff
>==========================================================================
>====
>---
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeDelegate.java (original)
>+++
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeDelegate.java Fri May 4 15:52:35 2012
>@@ -65,8 +65,8 @@ public class NodeDelegate extends ItemDe
> return getTree().getName();
> }
>
>- Status getNodeStatus() {
>- return getTree().getParent().getChildStatus(getName());
>+ Status getNodeStatus() throws InvalidItemStateException {
>+ return check(getTree().getParent()).getChildStatus(getName());
> }
>
> NodeDelegate getNodeOrNull(String relOakPath) {
>
>Modified:
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeImpl.java
>URL:
>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/or
>g/apache/jackrabbit/oak/jcr/NodeImpl.java?rev=1334046&r1=1334045&r2=133404
>6&view=diff
>==========================================================================
>====
>---
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeImpl.java (original)
>+++
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/N
>odeImpl.java Fri May 4 15:52:35 2012
>@@ -31,6 +31,7 @@ import org.slf4j.Logger;
> import org.slf4j.LoggerFactory;
>
> import javax.jcr.Binary;
>+import javax.jcr.InvalidItemStateException;
> import javax.jcr.Item;
> import javax.jcr.ItemNotFoundException;
> import javax.jcr.ItemVisitor;
>@@ -103,7 +104,11 @@ public class NodeImpl extends ItemImpl i
> */
> @Override
> public boolean isNew() {
>- return dlg.getNodeStatus() == Status.NEW;
>+ try {
>+ return dlg.getNodeStatus() == Status.NEW;
>+ } catch (InvalidItemStateException ex) {
>+ return false;
>+ }
> }
>
> /**
>@@ -111,7 +116,11 @@ public class NodeImpl extends ItemImpl i
> */
> @Override
> public boolean isModified() {
>- return dlg.getNodeStatus() == Status.MODIFIED;
>+ try {
>+ return dlg.getNodeStatus() == Status.MODIFIED;
>+ } catch (InvalidItemStateException ex) {
>+ return false;
>+ }
> }
>
> /**
>
>Modified:
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/S
>essionDelegate.java
>URL:
>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/or
>g/apache/jackrabbit/oak/jcr/SessionDelegate.java?rev=1334046&r1=1334045&r2
>=1334046&view=diff
>==========================================================================
>====
>---
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/S
>essionDelegate.java (original)
>+++
>jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/S
>essionDelegate.java Fri May 4 15:52:35 2012
>@@ -16,6 +16,19 @@
> */
> package org.apache.jackrabbit.oak.jcr;
>
>+import java.io.IOException;
>+
>+import javax.jcr.ItemExistsException;
>+import javax.jcr.NamespaceRegistry;
>+import javax.jcr.PathNotFoundException;
>+import javax.jcr.Repository;
>+import javax.jcr.RepositoryException;
>+import javax.jcr.Session;
>+import javax.jcr.Workspace;
>+import javax.jcr.lock.LockManager;
>+import javax.jcr.nodetype.NodeTypeManager;
>+import javax.jcr.version.VersionManager;
>+
> import org.apache.jackrabbit.oak.api.AuthInfo;
> import org.apache.jackrabbit.oak.api.CommitFailedException;
> import org.apache.jackrabbit.oak.api.ContentSession;
>@@ -32,16 +45,6 @@ import org.apache.jackrabbit.oak.namepat
> import org.slf4j.Logger;
> import org.slf4j.LoggerFactory;
>
>-import javax.jcr.NamespaceRegistry;
>-import javax.jcr.Repository;
>-import javax.jcr.RepositoryException;
>-import javax.jcr.Session;
>-import javax.jcr.Workspace;
>-import javax.jcr.lock.LockManager;
>-import javax.jcr.nodetype.NodeTypeManager;
>-import javax.jcr.version.VersionManager;
>-import java.io.IOException;
>-
> public class SessionDelegate {
> static final Logger log =
>LoggerFactory.getLogger(SessionDelegate.class);
>
>@@ -158,6 +161,28 @@ public class SessionDelegate {
>
> String srcPath = PathUtils.relativize("/", srcAbsPath); //
>TODO: is this needed?
> String destPath = PathUtils.relativize("/", destAbsPath);
>+
>+ // TODO: do the checks below belong here?
>+
>+ // check destination
>+ Tree dest = getTree(destPath);
>+ if (dest != null) {
>+ throw new ItemExistsException(destPath);
>+ }
>+
>+ // check parent of destination
>+ String destParentPath = PathUtils.getParentPath(destPath);
>+ Tree destParent = getTree(destParentPath);
>+ if (destParent == null) {
>+ throw new PathNotFoundException(destParentPath);
>+ }
>+
>+ // check source exists
>+ Tree src = getTree(srcPath);
>+ if (src == null) {
>+ throw new PathNotFoundException(srcPath);
>+ }
>+
> try {
> if (transientOp) {
> root.move(srcPath, destPath);
>
>