jsedding commented on code in PR #26:
URL:
https://github.com/apache/sling-org-apache-sling-i18n/pull/26#discussion_r4314647290
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -281,23 +289,45 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
if (status.resourceResolver == null) {
status.resourceResolver = createResourceResolver();
}
- if (isDictionaryResource(status.resourceResolver, change)) {
+ if (requiresDictionaryRediscovery(status.resourceResolver, change,
withinKnownDictionary)) {
status.reloadAll = true;
}
}
}
- private boolean isDictionaryResource(final ResourceResolver resolver,
final ResourceChange change) {
- // language node changes happen quite frequently
(https://issues.apache.org/jira/browse/SLING-2881)
- // therefore only consider changes either for sling:MessageEntry's
- // or for JSON dictionaries
- // get valuemap
+ /**
+ * Checks whether the changed resource requires dictionary rediscovery and
a full reload,
+ * rather than a targeted entry refresh.
+ *
+ * <p>Resources with {@code mix:language} and {@code jcr:language} require
rediscovery,
+ * including new roots nested inside known dictionaries and JSON
dictionary roots.
+ * {@code sling:MessageEntry} resources and resources with the {@code
sling:Message}
+ * mixin require rediscovery only outside known dictionaries. Inside known
dictionaries,
+ * the caller already collects the affected bundles for targeted refresh.
+ *
+ * <p>The caller separately handles changes to known roots and removal of
their ancestors.
+ * Missing resources return {@code false}; targeted invalidations already
collected for
+ * entry removals are retained.
+ *
+ * @param resolver resolver used to inspect the current resource
+ * @param change the observed resource change
+ * @param withinKnownDictionary whether the changed path is below a known
dictionary root
+ * @return {@code true} if the resource indicates that dictionaries must
be rediscovered
+ */
+ private boolean requiresDictionaryRediscovery(
+ final ResourceResolver resolver, final ResourceChange change,
final boolean withinKnownDictionary) {
final Resource resource = resolver.getResource(change.getPath());
if (resource == null) {
log.trace("Could not get resource for '{}' for event {}",
change.getPath(), change.getType());
return false;
}
- if (resource.getResourceType() == null) {
+ final ValueMap valueMap = resource.getValueMap();
+ // A new language root can also be nested inside a known dictionary.
+ if (hasMixin(valueMap, JcrResourceBundle.MIXIN_LANGUAGE) &&
valueMap.containsKey(PROP_LANGUAGE)) {
+ log.debug("Found dictionary root change at '{}'",
change.getPath());
+ return true;
+ }
+ if (withinKnownDictionary || resource.getResourceType() == null) {
Review Comment:
`resource.getResourceType()` is annotated `@NotNull`, thus I don't
understand this condition.
I also don't understand why changes to
`resource.isResourceType(JcrResourceBundle.RT_MESSAGE_ENTRY)` or
`hasMixin(valueMap, JcrResourceBundle.MIXIN_MESSAGE)` (see below) require full
dictionary discovery.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -281,23 +289,45 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
if (status.resourceResolver == null) {
status.resourceResolver = createResourceResolver();
}
- if (isDictionaryResource(status.resourceResolver, change)) {
+ if (requiresDictionaryRediscovery(status.resourceResolver, change,
withinKnownDictionary)) {
status.reloadAll = true;
}
Review Comment:
Simplify:
```suggestion
status.reloadAll =
requiresDictionaryRediscovery(status.resourceResolver, change,
withinKnownDictionary);
```
--
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]
