Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method getMergedProperties in ReloadableResourceBundleMessageSource does not set fileTimestamp [SPR-14583] #19152

Closed
spring-projects-issues opened this issue Aug 12, 2016 · 4 comments

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 12, 2016

Igor Stepanov opened SPR-14583 and commented

Hello Spring team,

I'd like to use ReloadableResourceBundleMessageSource to implement REST service to access localized content. And to achieve best performance, I'd like to support caching with "if-modified-since". So the idea is to rely on PropertiesHolder.getFileTimestamp() method to understand if some of the underlying property files was updated or not.

I've extended ReloadableResourceBundleMessageSource and call next method:

protected PropertiesHolder getMergedProperties(Locale locale) {
     PropertiesHolder mergedHolder = this.cachedMergedProperties.get(locale);
     if (mergedHolder != null) {
          return mergedHolder;
     }
     Properties mergedProps = newProperties();
     mergedHolder = new PropertiesHolder(mergedProps, -1);
     String[] basenames = StringUtils.toStringArray(getBasenameSet());
     for (int i = basenames.length - 1; i >= 0; i--) {
          List<String> filenames = calculateAllFilenames(basenames[i], locale);
          for (int j = filenames.size() - 1; j >= 0; j--) {
               String filename = filenames.get(j);
               PropertiesHolder propHolder = getProperties(filename);
               if (propHolder.getProperties() != null) {
                        mergedProps.putAll(propHolder.getProperties());
               }
          }
     }
     PropertiesHolder existing = this.cachedMergedProperties.putIfAbsent(locale, mergedHolder);
     if (existing != null) {
          mergedHolder = existing;
     }
     return mergedHolder;
}

However in current implementation property fileTImestamp is initialized with -1 and never gets updated, so it's not really useful. What do you think about updating it with latest fileTimestamp of the PropertiesHolder instances which are merged into this one?

Moreover, what about adding some setPropertiesChangedCallback() method to make it possible to subscribe to the file updates? In my case this change will allow to push changes to consumer through some message bus and prevent repeated checking through REST interface.


Affects: 4.3.2

Issue Links:

  • #16791 Race condition in ReloadableResourceBundleMessageSource since 4.1.0
  • #15133 ReloadableResourceBundleMessageSource locks properties hashmap and fails under load.
  • #17265 ResourceBundleMessageSource should allow for custom PropertyResourceBundle subclass
  • #14948 Allow adding resources to ReloadableResourceBundleMessageSource
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 12, 2016

Igor Stepanov commented

As the first stage I'd like to know just your opinion - if this is really a good idea and worth implementing, or maybe you know a better approach for such use case.

Thanks!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 18, 2016

Igor Stepanov commented

Thanks for attending #JavaDayKyiv, @jhoeller! Hope you've also enjoyed the visit and was not too annoyed with all the attention from the grateful Spring users (including me, of course).

As discussed, this issue still matters for me, and actually it can be decomposed into two parts:

  1. Just return the correct timestamp in PropertiesHolder.getFileTimestamp().
  2. Add some callback to allow some "reacting" when it's detected that properties are modified.

First one is simple and looks more like a bug - the method does not return the data, which is promised according to the signature. Second one is a bit more complex, but actually fits well the reactive idea of Spring 5.

In my case I've implemented a separate micro-service as a server-side holder for i18n content. And it would be rather efficient to allow this service push the updated data to subscribers upon actual content update instead of periodic pulling.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 2, 2016

Juergen Hoeller commented

We're setting the file timestamp on the merged holder as of 4.3.4 now, simply to the latest timestamp of the original holders.

As for reacting to changed properties: Effectively, the existing protected loadProperties method is only ever called when a new or modified resource has been detected. So by overriding that method, calling super.loadProperties first, you could reliably react to property file updates already before letting the base class proceed...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 2, 2016

Igor Stepanov commented

Thanks, for the fix, will try the new release.

Also got the idea about loadProperties - yeah, this should do the job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants