OAK-9436 clean transient session storage after exceptions during import

[kwin]

[kwin]

does the session contain any modifications after the exception

Potentially yes, but that was the case before this patch as well.

adding a {{assertFalse(session.hasPendingChanges()}} should be added to the tests.

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.

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.

I would love to but that isn't possible due to https://lists.apache.org/thread.html/rc57f0d58647113bad470d2632ec9cf3013ae3c6d41b759ada14c47be%40%3Cdev.jackrabbit.apache.org%3E.

[anchela]

@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.

[kwin]

so the import will be incomplete and the next session save would persist the incomplete changes. that sounds pretty dangerous to me.

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 Session.save() and never throw ConstraintViolationException during import but as I said in https://issues.apache.org/jira/browse/OAK-9436?focusedCommentId=17346882&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17346882 this might break existing consumers....

[anchela]

@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. but i strongly feel that others in the oak team should also take a look.... i pinged marcel in JIRA but others familiar with JCR and the inner workings of oak might want to review it as well (thinking of tom mueller, julian reschke and others). if you cannot request a review from them in git please ping them in JIRA or post to oak-dev.

[reschke]

Sorry; not working on Oak currently.

[github-actions[bot]]

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.