Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAK-9436 clean transient session storage after exceptions during import #293

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

OAK-9436 clean transient session storage after exceptions during import #293

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38d7090
OAK-9436 add test which shows that the transient session is not clean
kwin May 17, 2021
86b2f4a
OAK-9436 clean up transient session storage after constraint violations
kwin May 17, 2021
33eeff6
fix comment
kwin Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
272 changes: 148 additions & 124 deletions oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/xml/ImporterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,14 +266,16 @@ private Tree resolveUUIDConflict(Tree parent,
private void importProperties(@NotNull Tree tree,
@NotNull List<PropInfo> propInfos,
boolean ignoreRegular) throws RepositoryException {
EffectiveNodeType ent = effectiveNodeTypeProvider.getEffectiveNodeType(tree);
// process properties
for (PropInfo pi : propInfos) {
// find applicable definition
//TODO find better heuristics?
EffectiveNodeType ent = effectiveNodeTypeProvider.getEffectiveNodeType(tree);
PropertyDefinition def = ent.getPropertyDefinition(pi.getName(), pi.getType(), pi.isUnknownMultiple());
if (def == null) {
throw new ConstraintViolationException("No matching property definition found for " + pi.getName());
} else {
ent.checkSetProperty(pi.asPropertyState(def));
}
if (def.isProtected()) {
// skip protected property
Expand All @@ -294,6 +296,7 @@ private void importProperties(@NotNull Tree tree,
for (ProtectedPropertyImporter ppi : getPropertyImporters()) {
ppi.propertiesCompleted(tree);
}
// check for mandatory properties in endNode(...)
}

private Iterable<ProtectedPropertyImporter> getPropertyImporters() {
Expand Down Expand Up @@ -336,138 +339,150 @@ public void startNode(@NotNull NodeInfo nodeInfo, @NotNull List<PropInfo> propIn
throws RepositoryException {
Tree parent = parents.peek();
Tree tree = null;
String id = nodeInfo.getUUID();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff is confusing, but the block has not been removed but just wrapped in a try block to revert changes afterwards.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I sometimes do in such cases (to simplify the diff) is create a new method, e.g. getPropertyImportersTry, and then call it in a try-catch.

But I don't know this area so I'm afraid I'm not the right person to review...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't know this area so I'm afraid I'm not the right person to review...

Who is the right person then?

String nodeName = nodeInfo.getName();
String ntName = nodeInfo.getPrimaryTypeName();

if (parent == null) {
log.debug("Skipping node: {}", nodeName);
// parent node was skipped, skip this child node too
parents.push(null); // push null onto stack for skipped node
// notify the p-i-importer
if (pnImporter != null) {
pnImporter.startChildInfo(nodeInfo, propInfos);
}
return;
}

NodeDefinition parentDef = getDefinition(parent);
if (parentDef.isProtected()) {
// skip protected node
parents.push(null);
log.debug("Skipping protected node: {}", nodeName);

if (pnImporter != null) {
// pnImporter was already started (current nodeInfo is a sibling)
// notify it about this child node.
pnImporter.startChildInfo(nodeInfo, propInfos);
} else {
// no importer defined yet:
// test if there is a ProtectedNodeImporter among the configured
// importers that can handle this.
// if there is one, notify the ProtectedNodeImporter about the
// start of a item tree that is protected by this parent. If it
// potentially is able to deal with it, notify it about the child node.
for (ProtectedNodeImporter pni : getNodeImporters()) {
if (pni.start(parent)) {
log.debug("Protected node -> delegated to ProtectedNodeImporter");
pnImporter = pni;
pnImporter.startChildInfo(nodeInfo, propInfos);
break;
} /* else: p-i-Importer isn't able to deal with the protected tree.
try next. and if none can handle the passed parent the
tree below will be skipped */
try {
String id = nodeInfo.getUUID();
String nodeName = nodeInfo.getName();
String ntName = nodeInfo.getPrimaryTypeName();

if (parent == null) {
log.debug("Skipping node: {}", nodeName);
// parent node was skipped, skip this child node too
parents.push(null); // push null onto stack for skipped node
// notify the p-i-importer
if (pnImporter != null) {
pnImporter.startChildInfo(nodeInfo, propInfos);
}
return;
}
return;
}

if (parent.hasChild(nodeName)) {
// a node with that name already exists...
Tree existing = parent.getChild(nodeName);
NodeDefinition def = getDefinition(existing);
if (!def.allowsSameNameSiblings()) {
// existing doesn't allow same-name siblings,
// check for potential conflicts
if (def.isProtected() && isNodeType(existing, ntName)) {
/*
use the existing node as parent for the possible subsequent
import of a protected tree, that the protected node importer
may or may not be able to deal with.
-> upon the next 'startNode' the check for the parent being
protected will notify the protected node importer.
-> if the importer is able to deal with that node it needs
to care of the complete subtree until it is notified
during the 'endNode' call.
-> if the import can't deal with that node or if that node
is the a leaf in the tree to be imported 'end' will
not have an effect on the importer, that was never started.
*/
log.debug("Skipping protected node: {}", existing);
parents.push(existing);
/**
* let ProtectedPropertyImporters handle the properties
* associated with the imported node. this may include overwriting,
* merging or just adding missing properties.
*/
importProperties(existing, propInfos, true);
return;
}
if (def.isAutoCreated() && isNodeType(existing, ntName)) {
// this node has already been auto-created, no need to create it
tree = existing;
NodeDefinition parentDef = getDefinition(parent);
if (parentDef.isProtected()) {
// skip protected node
parents.push(null);
log.debug("Skipping protected node: {}", nodeName);

if (pnImporter != null) {
// pnImporter was already started (current nodeInfo is a sibling)
// notify it about this child node.
pnImporter.startChildInfo(nodeInfo, propInfos);
} else {
// edge case: colliding node does have same uuid
// (see http://issues.apache.org/jira/browse/JCR-1128)
String existingIdentifier = IdentifierManager.getIdentifier(existing);
if (!(existingIdentifier.equals(id)
&& (uuidBehavior == ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING
|| uuidBehavior == ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING))) {
throw new ItemExistsException(
"Node with the same UUID exists:" + existing);
// no importer defined yet:
// test if there is a ProtectedNodeImporter among the configured
// importers that can handle this.
// if there is one, notify the ProtectedNodeImporter about the
// start of a item tree that is protected by this parent. If it
// potentially is able to deal with it, notify it about the child node.
for (ProtectedNodeImporter pni : getNodeImporters()) {
if (pni.start(parent)) {
log.debug("Protected node -> delegated to ProtectedNodeImporter");
pnImporter = pni;
pnImporter.startChildInfo(nodeInfo, propInfos);
break;
} /* else: p-i-Importer isn't able to deal with the protected tree.
try next. and if none can handle the passed parent the
tree below will be skipped */
}
// fall through
}
return;
}
}

if (tree == null) {
// create node
if (id == null) {
// no potential uuid conflict, always add new node
tree = createTree(parent, nodeInfo, null);
} else if (uuidBehavior == ImportUUIDBehavior.IMPORT_UUID_CREATE_NEW) {
// always create a new UUID even if no
// conflicting node exists. see OAK-1244
tree = createTree(parent, nodeInfo, UUID.randomUUID().toString());
// remember uuid mapping
if (isNodeType(tree, JcrConstants.MIX_REFERENCEABLE)) {
refTracker.put(nodeInfo.getUUID(), TreeUtil.getString(tree, JcrConstants.JCR_UUID));
if (parent.hasChild(nodeName)) {
// a node with that name already exists...
Tree existing = parent.getChild(nodeName);
NodeDefinition def = getDefinition(existing);
if (!def.allowsSameNameSiblings()) {
// existing doesn't allow same-name siblings,
// check for potential conflicts
if (def.isProtected() && isNodeType(existing, ntName)) {
/*
use the existing node as parent for the possible subsequent
import of a protected tree, that the protected node importer
may or may not be able to deal with.
-> upon the next 'startNode' the check for the parent being
protected will notify the protected node importer.
-> if the importer is able to deal with that node it needs
to care of the complete subtree until it is notified
during the 'endNode' call.
-> if the import can't deal with that node or if that node
is the a leaf in the tree to be imported 'end' will
not have an effect on the importer, that was never started.
*/
log.debug("Skipping protected node: {}", existing);
parents.push(existing);
/**
* let ProtectedPropertyImporters handle the properties
* associated with the imported node. this may include overwriting,
* merging or just adding missing properties.
*/
importProperties(existing, propInfos, true);
return;
}
if (def.isAutoCreated() && isNodeType(existing, ntName)) {
// this node has already been auto-created, no need to create it
tree = existing;
} else {
// edge case: colliding node does have same uuid
// (see http://issues.apache.org/jira/browse/JCR-1128)
String existingIdentifier = IdentifierManager.getIdentifier(existing);
if (!(existingIdentifier.equals(id)
&& (uuidBehavior == ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING
|| uuidBehavior == ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING))) {
throw new ItemExistsException(
"Node with the same UUID exists:" + existing);
}
// fall through
}
}
} else {
}

Tree conflicting = idLookup.getConflictingTree(id);
if (conflicting != null && conflicting.exists()) {
// resolve uuid conflict
tree = resolveUUIDConflict(parent, conflicting, id, nodeInfo);
if (tree == null) {
// no new node has been created, so skip this node
parents.push(null); // push null onto stack for skipped node
log.debug("Skipping existing node {}", nodeInfo.getName());
return;
if (tree == null) {
// create node
if (id == null) {
// no potential uuid conflict, always add new node
tree = createTree(parent, nodeInfo, null);
} else if (uuidBehavior == ImportUUIDBehavior.IMPORT_UUID_CREATE_NEW) {
// always create a new UUID even if no
// conflicting node exists. see OAK-1244
tree = createTree(parent, nodeInfo, UUID.randomUUID().toString());
// remember uuid mapping
if (isNodeType(tree, JcrConstants.MIX_REFERENCEABLE)) {
refTracker.put(nodeInfo.getUUID(), TreeUtil.getString(tree, JcrConstants.JCR_UUID));
}
} else {
// create new with given uuid
tree = createTree(parent, nodeInfo, id);

Tree conflicting = idLookup.getConflictingTree(id);
if (conflicting != null && conflicting.exists()) {
// resolve uuid conflict
tree = resolveUUIDConflict(parent, conflicting, id, nodeInfo);
if (tree == null) {
// no new node has been created, so skip this node
parents.push(null); // push null onto stack for skipped node
log.debug("Skipping existing node {}", nodeInfo.getName());
return;
}
} else {
// create new with given uuid
tree = createTree(parent, nodeInfo, id);
}
}
}
}

// process properties
importProperties(tree, propInfos, false);
// process properties
importProperties(tree, propInfos, false);

if (tree.exists()) {
parents.push(tree);
if (tree.exists()) {
EffectiveNodeType parentEnt = effectiveNodeTypeProvider.getEffectiveNodeType(tree.getParent());
EffectiveNodeType ent = effectiveNodeTypeProvider.getEffectiveNodeType(tree);
// this throws ConstraintViolationException
parentEnt.getNodeDefinition(tree.getName(), ent);
parents.push(tree);
}
} catch (ConstraintViolationException e) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only this part is actually new

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible in this case to change to PR such that the changes are just limited to what really needs to change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping with a try...catch requires changing indentation. Only the diff view from Github is not able to correctly visualize the changes in a clever way... As I said above, I have not changed anything apart from that try...catch.

Copy link
Contributor

@mreutegg mreutegg Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually GitHub can. Just add ?w=1 to the URL or check the 'Hide whitespace' box in the Diff view settings.

// clean up transient session storage by removing the violating node
if (tree != null) {
tree.remove();
}
throw e;
}
}

Expand All @@ -479,15 +494,24 @@ public void endNode(@NotNull NodeInfo nodeInfo) throws RepositoryException {
if (pnImporter != null) {
pnImporter.endChildInfo();
}
} else if (getDefinition(parent).isProtected()) {
if (pnImporter != null) {
pnImporter.end(parent);
// and reset the pnImporter field waiting for the next protected
// parent -> selecting again from available importers
pnImporter = null;
} else {
if (getDefinition(parent).isProtected()) {
if (pnImporter != null) {
pnImporter.end(parent);
// and reset the pnImporter field waiting for the next protected
// parent -> selecting again from available importers
pnImporter = null;
}
}
try {
EffectiveNodeType ent = effectiveNodeTypeProvider.getEffectiveNodeType(parent);
ent.checkMandatoryItems(parent);
} catch (ConstraintViolationException e) {
// clean up transient session storage by removing the violating node
parent.remove();
throw e;
}
}

idLookup.rememberImportedUUIDs(parent);
}

Expand Down
Loading