[MAGNOLIA-5353] VirtualURIMapping: fallback to path if uuid gets changed Created: 02/Oct/13  Updated: 20/Mar/15  Resolved: 25/Oct/13

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: None
Fix Version/s: 4.5.13, 5.2

Type: Bug Priority: Neutral
Reporter: Zdenek Skodik Assignee: Cheng Hu
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Cloners
is cloned by MAGNOLIA-5440 CLONE - VirtualURIMapping: fallback t... Closed
relation
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Epic Link: Support
Sprint: 4.5.13

 Description   

The module's virtualURIMapping node used to not be bootstrapped but autogenerated with random uuid by the module mechanism. VirtualURIManager caches the defined mappings and reloads them upon a change to any mapping's configuration. This is based on node uuids. If one does such change and doesn't activate the particular mapping but the whole virtualURIMapping node, the uuid at Public gets changed and the manager is not able to reload it as it expects the former uuid. This causes all the module's mappings to not be working and the only way to fix it is to restart the webapp so that the manager registers all the existing mappings from scratch. We should fallback to path in such case.



 Comments   
Comment by Jan Haderka [ 28/Oct/13 ]

Few Comments, not necessarily in order of importance:

  • would not it be better to use some existing Pair implementation than to come up w/ your own? Such as http://commons.apache.org/proper/commons-lang/javadocs/api-release/org/apache/commons/lang3/tuple/ImmutablePair.html or similar.
  • The new code should not try to catch all exceptions, but only ItemNotFoundException (or whatever is the name of the exception thrown when Identifier doesn't exist anymore.
    {{+ } catch (Exception e)
    Unknown macro: {+ node = session.getNode(uuidAndPath.getPath());}

    }

  • In line with JCR 2.0 API, UUID should not be used anymore. The new method would be better called getNodeByIdentifierOrPath rather than {{getNodeByUUIDOrPath}
  • and most importantly we are changing here signature of protected variable that was exposed and used by custom observers. They would all fail with this change for Set<String> to Set< UUIDAndPath>. In order to stay backward compatible I would personally prefer to keep original list intact and to have second one (Map) that contains just mapping between identifier and path. WDYT?
Comment by Cheng Hu [ 28/Oct/13 ]
  • I agree and have been wondering where there might be such a class. Thanks for pointing the Apache implementation out.
  • Although in the latest commit I've made the change to a more narrow catch for ItemNotFoundException, I have to say I am not sure. So the scenario is I do not know all the possible ways in which get by identifier can fail, but for whatever reason it fails, I would like to retry getting by path, which catching all exceptions allow me to do. For me to accept catching only ItemNotFoundException, I would have to accept that all other possible exceptions thrown by get by identifier imply that get by path will fail, which I cannot do without knowing every possible (including runtime) exception that get by identifier can throw.
  • Agree, changed.
  • Agree, changed. This was originally a doubt in mind and I actually wanted to ask someone why this variable is protected. I chose the previous solution because it allowed me to make what I think is the least change to the existing logic and I did check in magnolia-core that nothing used the protected instance variable, but I'm happy to go with what you think is safer.

These changes have been made in the new push on the issue branch.

Comment by Jozef Chocholacek [ 28/Oct/13 ]

My objections go more into the test:

  • fail(e.getMessage()); - try to add at least a short explanation in which phase of test (both path & uuid correct, path incorrect & uuid correct, …) the failure happened;
  • we do not use @author and @version tags in the JavaDoc anymore;
  • as it is new file, the copyright line should contain only the current year, not the 2003-2013 range;

And the class itself (line #145):

protected static Node getNodeByIdentifierOrPath(...) throws ItemNotFoundException, RepositoryException {

The ItemNotFoundException is a subclass of the RepositoryException, thus no need to mention it twice, IMO.

Comment by Cheng Hu [ 28/Oct/13 ]

@Jozef thanks for pointing these out.

  • More informative messages added for test failures.
  • Removed these from the class Javadoc comment.
  • Changed to just use 2013 instead of range.
  • Changed to only throw RepositoryException.

These changes have been made in the new push on the issue branch.

Comment by Jan Haderka [ 29/Oct/13 ]

Looks great, just one tiny thing:

+     * @param uuid
+     * @param path

If you are not providing any info for those params, there's no need to add them to javadoc as they will be added there automatically once javadoc is generated.

Comment by Tobias Mattsson [ 20/Mar/15 ]

A better solution would have been to fix MAGNOLIA-4090 by changing from ObservedManager to ModuleConfigurationObservingManager. It observes using paths instead and solves this and other problems, such as synchronization.

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