[
http://issues.apache.org/jira/browse/JCR-535?page=comments#action_12444107 ]
Jukka Zitting commented on JCR-535:
-----------------------------------
Some cosmetic comments on the patch.
First, the following are just changes to whitespace. Is there some reason for
making those changes (i.e. do they align the code better or make it more
checkstyle-conformant)? If not, please simplify the patch by not making those
changes.
>>>>>>>>>>
@@ -164,8 +165,8 @@
* @throws ConstraintViolationException if some constraints checks are not
OK
*/
private boolean checkNode(NodeState parent, NodeInfo nodeInfo, List
propInfo)
- throws ConstraintViolationException, AccessDeniedException,
VersionException,
- LockException, ItemNotFoundException, ItemExistsException,
RepositoryException {
+ throws ConstraintViolationException, AccessDeniedException,
VersionException,
+ LockException, ItemNotFoundException, ItemExistsException,
RepositoryException {
itemOps.checkAddNode(parent, nodeInfo.getName(),
nodeInfo.getNodeTypeName(),
BatchedItemOperations.CHECK_ACCESS
| BatchedItemOperations.CHECK_CONSTRAINTS
@@ -231,7 +232,6 @@
itemOps.destroy(conflicting);
}
}
-
return true;
}
@@ -246,8 +246,8 @@
* @throws RepositoryException if issue in the NodeState
*/
private void createProperties(NodeState myNode, List propInfos)
- throws ItemNotFoundException, ItemExistsException,
ConstraintViolationException,
- ValueFormatException,
RepositoryException {
+ throws ItemNotFoundException, ItemExistsException,
ConstraintViolationException,
+ ValueFormatException, RepositoryException {
// process properties
Iterator iter = propInfos.iterator();
while (iter.hasNext()) {
@@ -272,7 +272,6 @@
// potential uuid conflict
NodeState conflicting = null;
NodeState node;
-
try {
if (nodeInfo.getId() != null) {
conflicting = itemOps.getNodeState(nodeInfo.getId());
@@ -280,7 +279,7 @@
} catch (ItemNotFoundException infe) {
conflicting = null;
}
- if (conflicting != null) {
+ if (conflicting != null) {
// resolve uuid conflict
node = resolveUUIDConflict(parent, conflicting, nodeInfo);
}
@@ -287,7 +286,7 @@
else {
// do create new node
node = itemOps.createNodeState(parent, nodeInfo.getName(),
nodeInfo.getNodeTypeName(),
-
nodeInfo.getMixinNames(), null, def);
+ nodeInfo.getMixinNames(), null, def);
}
return node;
}
@@ -305,7 +304,7 @@
* @throws RepositoryException
*/
private NodeState resolveUUIDConflict(NodeState parent, NodeState
conflicting, NodeInfo nodeInfo)
- throws ItemExistsException, ConstraintViolationException,
IllegalStateException, RepositoryException {
+ throws ItemExistsException, ConstraintViolationException,
IllegalStateException, RepositoryException {
NodeState node = null;
switch (uuidBehavior) {
<<<<<<<<<<
Second, is there a reason for getting rid of the debug and exception message?
If the "parentId == null" should *really* never happen (i.e. it's part of an
internal API contract), then you could use assert for that, but otherwise I
don't see the value of dropping the message.
>>>>>>>>>>
@@ -312,35 +311,36 @@
case ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING:
NodeId parentId = conflicting.getParentId();
if (parentId == null) {
- String msg = "root node cannot be replaced";
- log.debug(msg);
- throw new RepositoryException(msg);
+ //Shoudn't happen
+ throw new RepositoryException();
}
<<<<<<<<<<
Third, the added "else" clause below adds indentation to the code, without any
functional difference since the "if" part always throws an exception. This
makes reviewing the patch more difficult since I need to doublecheck each line
on whether there was some other change than just the extra level of
indentation. In fact the entire patch snippet below makes no functional
difference, but verifying that takes extra effort.
>>>>>>>>>>
- // 'replace' current parent with parent of conflicting
- try {
- parent = itemOps.getNodeState(parentId);
- } catch (ItemNotFoundException infe) {
- // should never get here...
- String msg = "internal error: failed to retrieve parent state";
- log.error(msg, infe);
- throw new RepositoryException(msg, infe);
+ else {
+ // 'replace' current parent with parent of conflicting
+ try {
+ parent = itemOps.getNodeState(parentId);
+ } catch (ItemNotFoundException infe) {
+ // should never get here...
+ String msg = "internal error: failed to retrieve parent
state";
+ log.error(msg, infe);
+ throw new RepositoryException(msg, infe);
+ }
+ // remove conflicting:
+ // check if conflicting can be removed
+ // (access rights, node type constraints, locking & versioning
status)
+ itemOps.checkRemoveNode(conflicting,
+ BatchedItemOperations.CHECK_ACCESS
+ | BatchedItemOperations.CHECK_LOCK
+ | BatchedItemOperations.CHECK_VERSIONING
+ | BatchedItemOperations.CHECK_CONSTRAINTS);
+ // do remove conflicting (recursive)
+ itemOps.removeNodeState(conflicting);
+ // do create new node
+ node = itemOps.createNodeState(parent, nodeInfo.getName(),
+ nodeInfo.getNodeTypeName(), nodeInfo.getMixinNames(),
+ nodeInfo.getId());
}
- // remove conflicting:
- // check if conflicting can be removed
- // (access rights, node type constraints, locking & versioning
status)
- itemOps.checkRemoveNode(conflicting,
- BatchedItemOperations.CHECK_ACCESS
- | BatchedItemOperations.CHECK_LOCK
- | BatchedItemOperations.CHECK_VERSIONING
- | BatchedItemOperations.CHECK_CONSTRAINTS);
- // do remove conflicting (recursive)
- itemOps.removeNodeState(conflicting);
- // do create new node
- node = itemOps.createNodeState(parent, nodeInfo.getName(),
- nodeInfo.getNodeTypeName(), nodeInfo.getMixinNames(),
- nodeInfo.getId());
break;
-
+
case ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING:
// make sure conflicting node is not importTarget or an ancestor
thereof
Path p0 = hierMgr.getPath(importTarget.getNodeId());
@@ -394,7 +394,7 @@
refTracker.mappedUUID(nodeInfo.getId().getUUID(),
node.getNodeId().getUUID());
}
break;
- //No need for default case.
+ //No need for default case.
}
return node;
}
@@ -478,7 +478,6 @@
}
}
-
//-------------------------------------------------------------< Importer >
/**
* [EMAIL PROTECTED]
<<<<<<<<<<
That adds up to 150+ lines of changes, whose only functional effect is the
removal of a debug and an exception message. I know the rest 60+ lines contain
the meat of the patch, but the signal-to-noise ratio of the patch is really
quite low. Could you get rid of the extra stuff before I focus on the real
changes? Thanks. :-)
Also, a test case that illustrates the desired behaviour would be nice.
> Ignore root node when importing through sysView
> -----------------------------------------------
>
> Key: JCR-535
> URL: http://issues.apache.org/jira/browse/JCR-535
> Project: Jackrabbit
> Issue Type: Improvement
> Components: core
> Reporter: Nicolas Toper
> Priority: Minor
> Attachments: patch-1-Ignore-root-node.txt,
> patch-WorkspaceImporter-231006.txt
>
>
> When importing through a sysView, we should ignore the root node. It is in
> the sysView to provide a root XML node, but the importer is going to attach
> it to the repository"s root node... Which would create another root node and
> often raise exception. This is a know issue
> I needed this behavior to change for the backup tool, since I use the
> sysView. Therefore, I havce slightly updated the WorkspaceImporter. Maybe I
> should update too the SessionImporter so we have a consistant behavior. What
> do you think?
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
