[jira] [Comment Edited] (SLING-4753) Commit the Resource Resolver before passing it to Tenant Customizers for setting up their own customizations

["Felix Meschberger (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SLING-4753?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15769366#comment-15769366 ] 

Felix Meschberger edited comment on SLING-4753 at 12/22/16 7:37 AM:
--------------------------------------------------------------------

Hmm, I am a bit concerned, yet, of course, late in the game.

IIRC we explicitly did *not* commit the changes to the Tenant before calling the customers to abort/roll-back the creation in case of problems. This change of course prevents that from happening.

I am also a bit concerned with the reasoning behind this change: AEM's PageManager.createPage method presumably clearing any changes before acting. I am not sure, we really want to support such questionable behaviour.

Also, while I agree that is not clearly spec-ed that changes are not persisted *before* calling the customizers, there is some indication in that:

* It is clearly stated in the TenantManager.setup method that changes are persisted *before* the method returns
* It is clearly stated with the TenantCustomer that ResourceResolver.commit must not be called

Particularly from the second statement we could deduce, that also ResourceResolver.revert should not be called, hence calling Session.refresh is equally unexpected and undesired.

Plus, even if the original creation is persisted, the behaviour could even revert any changes that a previous customizer applied. I really think, we should fix the problem not the symptom.

As such, the longer I think about it, the longer I have the impression, that this change should be reverted.

Last but not least: Do we really close issues before a release has been cut ?

/cc [~marett], [~amitgupt]


was (Author: fmeschbe):
Hmm, I am a bit concerned, yet, of course, late in the game.

IIRC we explicitly did *not* commit the changes to the Tenant before calling the customers to abort/roll-back the creation in case of problems. This change of course prevents that from happening.

I am also a bit concerned with the reasoning behind this change: AEM's PageManager.createPage method presumably clearing any changes before acting. I am not sure, we really want to support such questionable behaviour.

Also, while I agree that is not clearly spec-ed that changes are not persisted *before* calling the customizers, there is some indication in that:

* It is clearly stated in the TenantManager.setup method that changes are persisted *before* the method returns
* It is clearly stated with the TenantCustomer that ResourceResolver.commit must not be called

Particularly from the second statement we could deduce, that also ResourceResolver.revert should not be called, hence calling Session.refresh is equally unexpected and undesired.

As such, the longer I think about it, the longer I have the impression, that this change should be reverted.

Last but not least: Do we really close issues before a release has been cut ?

/cc [~marett], [~amitgupt]

> Commit the Resource Resolver before passing it to Tenant Customizers for setting up their own customizations
> ------------------------------------------------------------------------------------------------------------
>
>                 Key: SLING-4753
>                 URL: https://issues.apache.org/jira/browse/SLING-4753
>             Project: Sling
>          Issue Type: Bug
>          Components: Extensions
>    Affects Versions: Tenant 1.1.0
>            Reporter: Agraj Mangal
>            Assignee: Amit Gupta
>             Fix For: Tenant 1.1.0
>
>
> We should commit the Resource Resolver after creating the Tenant Resource and before passing it on to the Tenant Customizers. 
> One possible issue is that one of the Tenant Customizers calls some APIs like PageManager##createPage that does a session.refresh() and rollbacks all the un-committed changes on the resolver so far. That could also include the tenant resource itself. 
> Ideally the TenantCustomizers should not call commit on the resolver and let TenantProvider commit the changes, but it would be a good protection against all such cases where we could prevent the tenant resource from getting modified if the TenantCustomizer failed and tried to refresh the session.
> We are experiencing this issue in https://jira.corp.adobe.com/browse/MAC-25410 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)