OAK-9436 clean transient session storage after exceptions during import #293
Conversation
kwin
force-pushed
the
bugfix/OAK-9436-clean-transient-session-after-exceptions
branch
2 times, most recently
from
4dc6108 to
87a614e
Compare
kwin
changed the title
kwin
force-pushed
the
bugfix/OAK-9436-clean-transient-session-after-exceptions
branch
from
87a614e to
86b2f4a
Compare
There was a problem hiding this comment.
@kwin , the patch is quite hard to review. but i have a question: since {{Session.save()}} succeeds after the 'constraintviolationexception', does that mean that the import was incomplete? in other words: does the session contain any modifications after the exception? if that was the case, i would not be in favor of the change....
so, adding a {{assertFalse(session.hasPendingChanges()}} should be added to the tests.
also i would like you to invite more oak-committers for the review as i am not the owner of the importer code and might well miss some crucial pieces.
Potentially yes, but that was the case before this patch as well.
Not everything is reverted but just the conflicting node, so there might be changes in the session up to the point where the exception was thrown. This is IMHO expected behaviour.
I would love to but that isn't possible due to https://lists.apache.org/thread.html/rc57f0d58647113bad470d2632ec9cf3013ae3c6d41b759ada14c47be%40%3Cdev.jackrabbit.apache.org%3E. |
| @@ -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(); | |||
There was a problem hiding this comment.
The diff is confusing, but the block has not been removed but just wrapped in a try block to revert changes afterwards.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
| parentEnt.getNodeDefinition(tree.getName(), ent); | ||
| parents.push(tree); | ||
| } | ||
| } catch (ConstraintViolationException e) { |
There was a problem hiding this comment.
only this part is actually new
There was a problem hiding this comment.
would it be possible in this case to change to PR such that the changes are just limited to what really needs to change?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actually GitHub can. Just add ?w=1 to the URL or check the 'Hide whitespace' box in the Diff view settings.
|
@kwin , so the import will be incomplete and the next session save would persist the incomplete changes. that sounds pretty dangerous to me. IMHO the right way would rather be that the editing session reverts the incomplete changes from a failed import and will only then be able to call save again. so, having that clarified i am -1 for the proposed change. |
Actually you changed the behaviour in that direction already in http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/xml/ImporterImpl.java?r1=1836182&r2=1857242&pathrev=1857242. In case the property is violating constraints it is not added but the transient session storage contains all changes up to the property (excluding it). Also this is how Jackrabbit 2 does it, but I am also open to change it the other way around: Deferring all exceptions till |
|
@kwin , my preference would be to not change it all in this case. it's now a couple of years that jackrabbit2 is not used any more in Adobe AEM and i fear that changing the import behavior in the way(s) you suggest might lead to regressions. |
|
Sorry; not working on Oak currently. |
|
This PR is stale because it has been open 24 months with no activity. Remove stale label or comment or this will be closed in 30 days. |
No description provided.