Versioning does not work if a workspace is not in the same repository as mgnlSystem and mgnlVersion (MAGNOLIA-5975)

[MAGNOLIA-6093] Create mgnlSystem and mgnlVersion workspaces for each repository without registering in repositories.xml config Created: 20/Feb/15  Updated: 06/Nov/15  Resolved: 30/Mar/15

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:
Relates
causality
is causing MAGNOLIA-6427 WorkspaceMapping not correctly supported Closed
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.
Workspace physical names: mgnlSystem, mgnlVersion
Workspace logical names (used to obtain sessions for example): <repositoryId>-mgnlSystem, <repositoryId>-mgnlVersion



 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.
Since this is a non-trivial change, it needs to be well documented. At least, please describe the design of the change and their implication in the main task.

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 a unit test, typically, you'll instantiate the class-under-test yourself. e.g in FooBarTest, typically, you should find a new FooBar() somewhere.

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

  • avoid string concat and other voodoo to get version sessions, within classes that should not know about this (e.g DefaultContent)
  • avoid breaking all modules when we merge because it'd require yet another call to ComponentTestUtil to setup an only-vaguely related component (most tests that suffer from this are not testing anything about the repo, but they just so happen to use some content); ComponentTestUtil needs to go, so let's try to not add usages for new features.
Comment by Jan Haderka [ 08/Apr/15 ]

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.

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.

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.

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.

Generated at Mon Feb 12 04:11:14 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.