Versioning does not work if a workspace is not in the same repository as mgnlSystem and mgnlVersion
(MAGNOLIA-5975)
|
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 5.4 |
| Type: | Sub-task | Priority: | Neutral |
| Reporter: | Jaroslav Simak | Assignee: | Jan Haderka |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Template: |
|
||||||||||||
| Date of First Response: | |||||||||||||
| Description |
|
Workspaces will be created for each repository registered in repositories.xml config file without need to specify and map them in the mentioned config file. |
| Comments |
| Comment by Jan Haderka [ 04/Mar/15 ] |
|
Check for workspace name in MgnlVersioningNodeWrapper should probably use endsWith("-" + workspaceName) rather then contains to minimize possibility of false positives. MgnlLogicalNamed*Wrapper would be less confusing with name like MgnlLogicalWorkspaceNameMapping*Wrapper or similar. As it is, those wrapper names mislead everyone to think that they might do some renaming of nodes or properties. Shouldn't there be also change in default repositories.xml in empty webapp to remove manual mgnlVersion and mgnlSystem mappings or were they never defined there? |
| Comment by Jaroslav Simak [ 05/Mar/15 ] |
|
https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=shortlog;h=refs/heads/MAGNOLIA-5975 https://git.magnolia-cms.com/gitweb/?p=ce-bundle.git;a=shortlog;h=refs/heads/MAGNOLIA-5975 |
| Comment by Magnolia International [ 20/Mar/15 ] |
|
Javadoc for wrappers. |
| Comment by Magnolia International [ 07/Apr/15 ] |
|
Ok seems good - I'd just like to take a moment to see if we can improve something test-wise (e.g I suspect we could inject RepositoryManager where something-somewhere probably calls Components.getInstance(); would simplify tests AND prod code) |
| Comment by Magnolia International [ 07/Apr/15 ] |
|
This was most likely already this way, but in good boy scout fashion, let's try to improve existing code and remove debt where we see it. In DefaultRepositoryManagerTest, we do this // GIVEN final DefaultRepositoryManager manager = (DefaultRepositoryManager) Components.getComponent(RepositoryManager.class); which makes way for a number of possible side effects and unwanted interactions with the test. Simplify ! |
| Comment by Magnolia International [ 07/Apr/15 ] |
|
Hmmm and one dubious change that escaped me at first sight are those 2, hidden in DefaultContent#delete() - if (!workspaceName.startsWith("mgnl")) {
+ if (!(workspaceName.endsWith("-" + RepositoryConstants.VERSION_STORE) || workspaceName.endsWith("-" + RepositoryConstants.SYSTEM))) {
- Session session = MgnlContext.getJCRSession("mgnlVersion");
+ Session session = MgnlContext.getJCRSession(repositoryManager.getRepositoryNameForWorkspace(workspaceName) + "-" + RepositoryConstants.VERSION_STORE);
These 2 lines were not ideal, but now they cary a whole bunch of implications that should be "hidden" somewhere in RepositoryManager or some other component. |
| Comment by Magnolia International [ 07/Apr/15 ] |
|
I found the above snippet in a couple of other locations; while it's very limited and seems to often be related to deletes, I'd really like if we could abstract that somewhere else; likewise, I'd like it if we could remove the MgnlContext.getHierarchyManager() calls this does, especially when they're done in a MgnlContext.doInSystemContext() block, which makes things are to debug, read and test. When that's done, it might become much clearer how to address the test problem: rather than add ComponentsTestUtil.setImplementation(RepositoryManager.class, DefaultRepositoryManager.class) we might be able to inject RepoMan in more appropriate places; MockNode and MockContent could use their own MockRepositoryManager instead of rely on Components being set for each and every test. The "funny" thing is tried to adjust MockContent to "force" it to use a MockRepositoryManager. My local MockRepositoryManager did nothing but throw exceptions, and yet, when running all core-tests, it wasn't invoked once. In short, there's 2 things I'd suggest we look into
|
| Comment by Jan Haderka [ 08/Apr/15 ] |
I would argue that DefaultContent is already deprecated and it's not worth of adding extra method to RepositoryManager just to hide those implications. We do not have same problems when working with Node API directly.
AFAIK for most if not all uses of MockContent/MockSession they "manage" content in memory w/o real repo behind so they indeed should not need to use RepositoryManager it just have to be available for injection. |