jsedding commented on code in PR #26:
URL:
https://github.com/apache/sling-org-apache-sling-i18n/pull/26#discussion_r4324781853
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -277,55 +285,57 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
}
}
- // may be a completely new dictionary
if (status.resourceResolver == null) {
status.resourceResolver = createResourceResolver();
}
- if (isDictionaryResource(status.resourceResolver, change)) {
+ final Resource resource =
status.resourceResolver.getResource(change.getPath());
+ if (resource == null) {
+ log.trace("Could not get resource for '{}' for event {}",
change.getPath(), change.getType());
+ return;
+ }
Review Comment:
I expect that {{resource == null}} is always true for "delete" events. Is
that correct?
If that is the case, I think we need to invalidate the dictionary (or
ResourceBundle if necessary) that contains this resource.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -277,55 +285,57 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
}
}
- // may be a completely new dictionary
if (status.resourceResolver == null) {
status.resourceResolver = createResourceResolver();
}
- if (isDictionaryResource(status.resourceResolver, change)) {
+ final Resource resource =
status.resourceResolver.getResource(change.getPath());
+ if (resource == null) {
+ log.trace("Could not get resource for '{}' for event {}",
change.getPath(), change.getType());
+ return;
+ }
+
+ // A new language root can introduce another locale or basename,
even inside a known dictionary.
+ if (isDictionaryRoot(resource)) {
+ log.debug("Found dictionary root change at '{}', requiring a
full reload", change.getPath());
+ status.reloadAll = true;
+ return;
+ }
Review Comment:
Why do we need a full reload if we find a new dictionary? Could we not
reload just the ResourceBundle(s) affected by the locale of this dictionary?
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -261,8 +262,15 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
change.getPath());
status.reloadAll = true;
} else {
+ boolean withinKnownDictionary = false;
for (final String root : languageRootPaths) {
- if (change.getPath().startsWith(root)) {
+ if (change.getType() == ResourceChange.ChangeType.REMOVED &&
new Path(change.getPath()).matches(root)) {
+ // Removing an ancestor also removes its dictionaries.
+ status.reloadAll = true;
+ return;
+ }
Review Comment:
Could we just throw away the affected dictionary(s) (or ResourceBundles if
necessary) instead of reloading all?
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -261,8 +262,15 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
change.getPath());
status.reloadAll = true;
} else {
+ boolean withinKnownDictionary = false;
for (final String root : languageRootPaths) {
- if (change.getPath().startsWith(root)) {
+ if (change.getType() == ResourceChange.ChangeType.REMOVED &&
new Path(change.getPath()).matches(root)) {
+ // Removing an ancestor also removes its dictionaries.
+ status.reloadAll = true;
+ return;
+ }
+ if (new Path(root).matches(change.getPath())) {
+ withinKnownDictionary = true;
Review Comment:
IIUC this flag is equivalent to `!status.reloadBundles.isEmpty()`, right? If
so, we don't really need the flag.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -277,55 +285,57 @@ private void onChange(final ChangeStatus status, final
ResourceChange change) th
}
}
- // may be a completely new dictionary
if (status.resourceResolver == null) {
status.resourceResolver = createResourceResolver();
}
- if (isDictionaryResource(status.resourceResolver, change)) {
+ final Resource resource =
status.resourceResolver.getResource(change.getPath());
+ if (resource == null) {
+ log.trace("Could not get resource for '{}' for event {}",
change.getPath(), change.getType());
+ return;
+ }
+
+ // A new language root can introduce another locale or basename,
even inside a known dictionary.
+ if (isDictionaryRoot(resource)) {
+ log.debug("Found dictionary root change at '{}', requiring a
full reload", change.getPath());
+ status.reloadAll = true;
+ return;
+ }
+
+ // An entry outside known roots may reveal another dictionary.
Entries inside known dictionaries
+ // are already covered by the targeted refreshes collected above.
+ if (!withinKnownDictionary && isMessageEntry(resource)) {
status.reloadAll = true;
}
Review Comment:
> An entry outside known roots may reveal another dictionary.
True. But should we then not walk up the tree until we find that dictionary?
And ideally only load this new dictionary (or second best only the affected
ResourceBundle)?
--
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]
