jsedding commented on code in PR #107:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/107#discussion_r1403193013
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -1243,24 +1236,20 @@ private boolean loadAlias(final Resource resource,
Map<String, Map<String, Strin
if (aliasArray != null) {
log.debug("Found alias, total size {}", aliasArray.length);
+ // the order matters here, the first alias in the array must
come first
for (final String alias : aliasArray) {
if (isAliasValid(alias)) {
log.warn("Encountered invalid alias {} under parent
path {}. Refusing to use it.", alias, parentPath);
} else {
- Map<String, String> parentMap = map.get(parentPath);
-
- if (parentMap == null) {
- parentMap = new LinkedHashMap<>();
- map.put(parentPath, parentMap);
- }
-
- String current = parentMap.get(alias);
- if (current != null) {
+ ConcurrentMap<String, List<String>> parentMap =
map.computeIfAbsent(parentPath, key -> new ConcurrentHashMap<>());
+ Optional<String> currentResourceName =
parentMap.entrySet().stream().filter(entry ->
entry.getValue().contains(alias)).findFirst().map(Map.Entry::getKey);
+ if (currentResourceName.isPresent()) {
log.warn(
"Encountered duplicate alias {} under
parent path {}. Refusing to replace current target {} with {}.",
- alias, parentPath, current, resourceName);
+ alias, parentPath,
currentResourceName.get(), resourceName);
} else {
- parentMap.put(alias, resourceName);
+ List<String> existingAliases =
parentMap.computeIfAbsent(resourceName, name -> new CopyOnWriteArrayList<>());
+ existingAliases.add(alias);
Review Comment:
We're not accurately mirroring the ordering of the `aliasArray` into the
list. I really think it would be better to construct a `LinkedHashSet`, fill it
with the aliases, wrap it using `Collections#unmodifieableSet()` and at the end
replace it in the map.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -140,7 +142,10 @@ public class MapEntries implements
private Map <String,List <String>> vanityTargets;
- private Map<String, Map<String, String>> aliasMap;
+ /**
+ * The key of the map is the parent path, while the value is a map with
the the resource name as key and the actual aliases as values)
+ */
+ private ConcurrentMap<String, ConcurrentMap<String, List<String>>>
aliasMapsMap;
Review Comment:
> Yes, this is all correct. I consider the case to lookup an alias for a
given resource name (i.e. map) more frequent than looking for an alias below a
specific parent (i.e. resolve). The flipping of keys and values is necessary to
keep the order of the sling:alias values for a specific resource (see next
question) as there is no ConcurrentMap keeping the insertion order (similar to
LinkedHashMap).
Agreed. And I see the dilemma of not having a concurrent Map implementation
that maintains the insertion order. I also prefer your proposed datastructure,
because the alias ordering is localized to the individual resource and thus
easier to understand.
Regarding the "easier to understand" part, I believe that the current
implementation, as well as yours, suffer from an underspecification of the
ordering of aliases defined on a resource _and_ on its `jcr:content` child.
I.e. assume we have `/foo/sling:alias=bar` and
`/foo/jcr:content/sling:alias=baz`. Which one would be "first"? IIUC, currently
it depends on which one is inserted first.
> This was done deliberately to make users aware that this is a thread safe
implementation, i.e. no need to synchronize access on it. This is more explicit
than just mentioning it in the javadoc. What issues do you see we that?
ConcurrentMap and Map share exactly the same interface signature, the
differences are only in the default implementations.
I would much rather hide the implementation details, especially in the
signatures. This makes future refactoring easier. That's why I would really
like to see the functionality encapsulated in an object, rather than it being
plastered all over the `MapEntries` class. I agree with you, however, that we
would first need to re-write the tests to be agnostic of implementation
details. They are, unfortunately, quite horrible, IMHO :(
> The implementation used here CopyOnWriteArrayList uses a replacement
internally. [...]
I still believe we would be better of creating a new immutable list (or
preferably a set) each time, and replace it. Immutable objects are always
easier to reason about than mutable ones. And updating the list to reflect the
correct order of aliases is harder than creating a new one IMHO.
##########
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##########
@@ -1243,24 +1236,20 @@ private boolean loadAlias(final Resource resource,
Map<String, Map<String, Strin
if (aliasArray != null) {
log.debug("Found alias, total size {}", aliasArray.length);
+ // the order matters here, the first alias in the array must
come first
for (final String alias : aliasArray) {
if (isAliasValid(alias)) {
log.warn("Encountered invalid alias {} under parent
path {}. Refusing to use it.", alias, parentPath);
} else {
- Map<String, String> parentMap = map.get(parentPath);
-
- if (parentMap == null) {
- parentMap = new LinkedHashMap<>();
- map.put(parentPath, parentMap);
- }
-
- String current = parentMap.get(alias);
- if (current != null) {
+ ConcurrentMap<String, List<String>> parentMap =
map.computeIfAbsent(parentPath, key -> new ConcurrentHashMap<>());
+ Optional<String> currentResourceName =
parentMap.entrySet().stream().filter(entry ->
entry.getValue().contains(alias)).findFirst().map(Map.Entry::getKey);
+ if (currentResourceName.isPresent()) {
log.warn(
"Encountered duplicate alias {} under
parent path {}. Refusing to replace current target {} with {}.",
- alias, parentPath, current, resourceName);
+ alias, parentPath,
currentResourceName.get(), resourceName);
Review Comment:
Could we rename `currentResourceName` to `siblingWithDuplicateAlias` or
something that makes it easier to understand?
Also, we should filter the entry from the stream that matches
"resourceName". For that one, it would be ok to contain duplicates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
