[MGNLHARDLK-74] Locked pages using JCR native API cannot be edited by anyone Created: 23/Aug/18 Updated: 01/Oct/18 Resolved: 01/Oct/18 |
|
| Status: | Closed |
| Project: | Project - Telia - Hard Locking (closed) |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Viet Nguyen | Assignee: | Zdenek Skodik |
| Resolution: | Won't Do | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 0.5d | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||
| Issue Links: |
|
||||||||||||
| Template: |
|
||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||
| Date of First Response: | |||||||||||||
| Description |
|
After review and refactor hard-locking module, we decided to use JCR native lock support instead of hooking into MagnoliaAccessProvider and provide the custom implementation for locking. This helps end user not to configure their repository access provider anymore and simplify the implementation a lot. However this lead to an issue that:
JCR function APIs:
*This feature is conflicted with our declared function in https://wiki.magnolia-cms.com/display/SERVICES/Hard+Locking+submodule*
|
| Comments |
| Comment by Richard Gange [ 24/Aug/18 ] |
|
To me this does not seem like such a big deal. If the person wants to edit it then they must unlock it to do so. Perhaps there may even be use cases where this makes sense for everyone to be locked out. I see this as the same type of thing where a user in an application goes into "admin mode" or something like that. The system prompts the user that in order to make the change they must enter a different mode. Perhaps we could do something like that. |
| Comment by Viet Nguyen [ 28/Aug/18 ] |
|
After evaluated different approaches including
Final solution: Zip file here: org.zip This ticket is fixed under this commit: |
| Comment by Zdenek Skodik [ 30/Aug/18 ] |
|
Could you please elaborate on the rationale behind such a decision? I'd rather hesitate to patch JR impl directly, nor even rely on classloading order, which might vary between various app servers so as might be a subject of change at various deployments. We've came up with a custom mgnl AccessControlProvider right to target quirks in JR API and to me it sounds reasonable to expect those who wanna have even more strict provider to configure another one, extending the default one. |
| Comment by Viet Nguyen [ 30/Aug/18 ] |
|
Yes zdenekskodik, I'd rather not to patch JR if it is not necessary. If you had a chance to go over our custom AccessControlProvider, you'll see how much better if we can simply patch LockInfo. Allow lock owner to edit content Tomcat application server To enable lock owner to edit his locked content, we have to patch Jackrabbit core LockInfo class which is the core class to check for lock holder information. Users would like to use this function would have to apply this patch by copying 'org' folder below 'override' folder to 'WEB-INF/classes' folder under your deployed webapp. We also provided an 'org.zip' file so that you just have to copy it to your 'WEB-INF/classes' folder then unzip it. This will effectively made LockInfo.class (our patched one) available to webapp class-loader and allow it loading this class instead of Jackrabbit provided one. https://tomcat.apache.org/tomcat-8.5-doc/class-loader-howto.html Weblogic 12 Configuration - prefer-web-inf-classes Property 'prefer-web-inf-classes' has to be specified for the classes specified in Web-INF to take precendence over classes present in application library of weblogic server. https://docs.oracle.com/middleware/1212/wls/WLPRG/classloading.htm#WLPRG285 IBM Websphere 8.5 The sequence is similar to Tomcat 8, also it's configurable. https://www.ibm.com/support/knowledgecenter/en/SSAW57_8.5.5/com.ibm.websphere.nd.multiplatform.doc/ae/trun_classload_web.html |
| Comment by Zdenek Skodik [ 31/Aug/18 ] |
|
I dream about the world where it's up to the instance owner to drive their appservers, I won't be happy as well if something from outside demands me to configure classloaders this or that way, there might be internal policies for not doing it that way, or another intentions on using the appservers. It's quite fine if they deploy their local changes that way, but external solutions should ideally be delivered in a clean and standard format - jar, than having patches here or there. Maintenance wise there would also be an extra effort required any time a major will bring new JR into play. Also It's not an appservers agnostic solution as it used to be. We could have a hard locking parent webapp/pom providing the JR config files with preconfigured LockAwareMagnoliaAccessProvider customers can just use/extend as an alternative option, without having to follow some post-installation steps. IMHO having to reconfigure your appservers is a bit more demanding that updating few workspace.xml files. Sure there are some harder times on distributing the lock permissions at the provider, still workaround-able and better to me than maintaining a fork.
|
| Comment by Viet Nguyen [ 31/Aug/18 ] |
|
I agree with you zdenekskodik, however we spent quite a lot of time on this issue and also as rgange mentioned, to a certain extent, hard-locking module support JCR hard lock, user have to unlock to edit his content.
--> I'll create an improvement on Core and link this ticket into it. |
| Comment by Viet Nguyen [ 31/Aug/18 ] |
|
Link ticket created - MAGNOLIA-7365 |
| Comment by Zdenek Skodik [ 10/Sep/18 ] |
|
Please correct me if i'm wrong - hard locking aka i lock the page so that nobody else can edit it to concurrently override my writes i'm going to do. If JCR native lock doesn't meet this criteria I have hard time to understand why to follow this way. I believe that was considered in the early stages as well, but dropped. Ideally we want to have hard locking available so that ppl can use it. It doesn't have to necessarily reach the highest state of art with its very first release, keeping some room to properly implement improvements later on. From my point of view, inheriting the initial AccessProvider is simply better than forking JR with the first release. Later on the module can eventually introduce its own RepositoryManager allowing you to have a tailored ContentDecorator. rgange ? |
| Comment by Richard Gange [ 10/Sep/18 ] |
|
I'm in total agreement with Zdenek. As far as I know the Hard Locking was working for TS. I think what we should do is keep the code that you have created using the native lock. Perhaps down the road we can still use some of it. But let's move back to the original working design. I know that you (Viet) have put a lot of time into this moving to the new concept but it appears we have hit a wall here. It was a good attempt at a new solution but let's not get too crazy with changing lower level stuff to make the new concept work. This could be a really nice feature to be part of the product someday. |
| Comment by Viet Nguyen [ 10/Sep/18 ] |
|
So, we can revert back to our 1st merge PR here for our original version before making any API changes
|
| Comment by Viet Nguyen [ 11/Sep/18 ] |
|
zdenekskodik could you review changes, QA and close this issue, fixed as discussion. |
| Comment by Zdenek Skodik [ 11/Sep/18 ] |
|
Yup.. rgange is there any QA workflow already in place for extension modules or review&preintQA can be done by a single guy? |
| Comment by Zdenek Skodik [ 11/Sep/18 ] |
|
So after a review we'll try to treat the status quo by evaluating a dedicated repository manager wrapping the sessions hard-lock way, as if possible we'd prefer to not loose all the work done since than just because of those few lines at JR. |
| Comment by Richard Gange [ 11/Sep/18 ] |
|
Typically I will install the module and try it out. This way I can better create the required docs if I have firsthand usage of the module. viet.nguyen if you could put a rough draft of the docs together I will use that as my starting point. |
| Comment by Viet Nguyen [ 12/Sep/18 ] |
|
Yes rgange, please reference to README.md file in the project for the installation and usage draft, we'll try to keep it up to date as a practice. Also let's consider reuse Javadoc to alleviate Docs team, maybe somehow auto generate configuration parts and technical detail level implementation notes. |
| Comment by Viet Nguyen [ 25/Sep/18 ] |
|
Improvement mechanism for this in DEV-816. |
| Comment by Richard Gange [ 01/Oct/18 ] |
|
Reverted changes that led to this issue. Meaning, the native locking. |