Spring i18n needs TimeZone resolution as well as Locale resolution. [SPR-1528]

[spring-projects-issues]

Tom Czarniecki opened SPR-1528 and commented

Spring Web framework has really good and extensible support for Locale resolution. However, in many internationalised applications one also requires to be able to resolve TimeZones, so that a remote user can be shown, for example, transaction logs in their local TimeZone, and not in the TimeZone of the server. You can always retrieve the user's timezone from the database at login and store it using a known key in the session or look it up whenever you need it. What I propose is really a set of extensible interfaces and strategy classes, based on Spring's current locale resolution mechanism, to allow a uniform approach to TimeZone resolution. Guess I have done this too many times for clients :-)

---

Attachments:

• client_timezone.zip (7.44 kB)
• FixedTimeZoneResolver.java (2.68 kB)
• TimeZoneInterceptor.java (3.14 kB)
• TimeZoneResolver.java (3.41 kB)
• TimeZoneUtils.java (2.90 kB)

Issue Links:

• JSP tags doesn't pick up JSTL-defined time zone at page level [SPR-15746] #20302 JSP tags doesn't pick up JSTL-defined time zone at page level
• CookieLocaleResolver is not RFC6265 compliant when setting a locale and time zone [SPR-16700] #21241 CookieLocaleResolver is not RFC6265 compliant when setting a locale and time zone

Referenced from: commits 44dee37, 7ed108e, 7b2ac6d, 4574528, c664010

5 votes, 7 watchers

[spring-projects-issues]

Tom Czarniecki commented

The above classes made it trivial to extend org.apache.velocity.tools.generic.DateTool to provide internationalised date handling.
Check out the http://xconf.sourceforge.net/toolbox/net/sourceforge/xconf/toolbox/spring/SpringDateTool.html.

[spring-projects-issues]

Nickolay Mazurkin commented

I've implemented the timezone client definition as a full analogue of a locale engine in Spring. It works absolutely fine so I would be glad if such functionality became a standard in Spring Web

(I've deleted all headers/package names in files - don't think it would be a problem for anyone)

[spring-projects-issues]

Xavier Dury commented

@Nickolay, JstlUtils.exposeLocalizationContext() should be adapted too in order to expose the TimeZone to jstl formatDate tag.

[spring-projects-issues]

Nickolay Mazurkin commented

I use my own DispatcherServlet subclass with a such method

@Override
protected void render(ModelAndView mv, HttpServletRequest request, HttpServletResponse response) throws Exception {
Config.set(request, Config.FMT_TIME_ZONE, timezoneResolver.resolveTimezone(request));
super.render(mv, request, response);
}

[spring-projects-issues]

Rossen Stoyanchev commented

This issue has been resolved through a selective bulk update, as part of a larger effort to better manage unresolved issues. To qualify for the update, the issue was either created before Spring 3.0 or affects a version older than Spring 3.0 and is not a bug.

There is a good chance the request was made obsolete, or at least partly outdated, by changes in later versions of Spring including deprecations. It is also possible it didn't get enough traction or we didn't have enough time to address it. One way or another, we didn't get to it.

If you believe the issue, or some aspects of it, are still relevant and worth pursuing at present you may re-open this issue or create a new one with a more up-to-date description.

We thank you for your contributions and encourage you to become familiar with the current process of managing Spring Framework JIRA issues that has been in use for over a year.

[spring-projects-issues]

Nick Williams commented

A worthy suggestion was made here with example implementation and never adopted. I think this is a much-needed addition to Spring Framework 4.0. I am re-opening this, and will attempt to implement it and submit a pull request.

[spring-projects-issues]

Phil Webb commented

#298

[spring-projects-issues]

Nick Williams commented

I have submitted pull request 298 with thorough time zone support implement. I had no idea how big of a task this was going to be. Tom's attachments just began to scratch the surface as to what was needed to make this work. TimeZone support closely mirrors Locale support and appears to work very well. Thorough JavaDoc documentation and 69 unit tests back up the changes.

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

[spring-projects-issues]

Phil Webb commented

Wow Nick, that is quite some work! Cheers.

I have updated the pull request with a few comments, the main one is if we can use a Converter rather than a PropertyEditor. I think Juergen Hoeller has plans for full JSR-310 support so merging this pull request might make most sense when that work is addressed.

Rossen Stoyanchev will probably also be interested in what you have done.

Thanks for the great work, it really does look like a very complete solution.

[spring-projects-issues]

Nick Williams commented

Thanks, Phil. I had a couple quick questions that I added to the pull request after you commented. I'll summarize them here. I'd appreciate some guidance so that I can take this to phase two.

1. Could you elaborate a bit on the difference between Converter and PropertyEditor and why you would use one over another? I'm not clear on the distinction, and I feel like I've seen some types that have both a converter and an editor. Isn't PropertyEditor a Java SE standard, and so doesn't that make it more portable?
2. What steps do I need to take to make this a converter? I looked around and didn't immediately see an equivalent of PropertyEditorRegistrySupport for converters.
3. Since I was trying to mirror the Locale/LocaleContext/LocaleContextHolder implementation as closely as possible, I made the code and patterns nearly identical, including the tests. Is it crucial that I convert all my unit tests to use @Test instead of TestCase, or was that guidance for the future? I took the TestCase route because that's how the Locale-related tests were written.

Also:

Do I really need to quash commits with my next commit? I know the dev guidelines said to do so, but I tried that on a different repo the other day and it did not go well. If that's a must, some instructions would sure be helpful.

[spring-projects-issues]

Phil Webb commented

The ConverterService has a more flexible API and is designed specifically with conversion in mind. PropertyEditors are really intended more for GUIs to render bean properties. Some of the advantages are:

• Can convert from different source/target types (not just to/from Strings)
• Can access additional information via TypeDescriptor (such as method annotations)
• Can be conditional (for example based on annotations, see ConditionalConverter)
• Can convert to ranged subclasses of types (see ConverterFactory)
• Can convert elements within Collections and Arrays
• Can be customized easily by the user (eg. WebMvcConfigurationSupport#addFormatters
• Can support one-way only conversion

Recently, most core code has been developed using Converters rather than PropertyEditors but I take your point about portability.

It might be worth waiting for Juergen Hoeller to review this before attempting that change, he might have different ideas here; but to answer the question:

The DefaultFormattingConversionService is the default ConversionService used by MVC. You can see that it calls some Registrars for things like JodaTime and also calls DefaultConversionService.addDefaultConverters for all basic conversion. The closest existing match is to ZoneIdEditor is probably StringToLocaleConverter.

No worries with that, it was more guidance. Certain JUnit features (such as @Rule) only work with the newer style. You can certainly leave those tests as they are.

We can easily squash commits during a merge, it is fine to leave them and in this case it may even help to show how the pull request evolved.

[spring-projects-issues]

Nick Williams commented

Sounds good, Phil. I'll wait until Juergen Hoeller weighs in before I make any changes.

Meanwhile, think you'll have a chance to look at my other pull request soon? It's much less extensive. :-)

[spring-projects-issues]

Nick Williams commented

Phil, I really wanted to get this in Milestone 2, which is suppose to release this Monday. Could you poke Juergen and see if he's had a chance to look at this? I really want to commit the necessary changes so that it can be reviewed again and, hopefully, merged.

[spring-projects-issues]

Phil Webb commented

I think the M2 date may slip by a week. Things are fairly hectic at the moment but I would like to see this make M2.

[spring-projects-issues]

Juergen Hoeller commented

Indeed, 4.0 M2 has been postponed for at least a week - the deadline is July 15th now. The TimeZone stuff (a few related issues) is high up on my list there.

Juergen

[spring-projects-issues]

Nick Williams commented

Juergen and Phil,

I haven't seen any movement on this 3 weeks. What can I do to make this happen?

[spring-projects-issues]

Nick Williams commented

I'm starting to get concerned about my pull request's mergability. :-(

[spring-projects-issues]

Nick Williams commented

According to JIRA, 4.0 M2 releases today. Phil Webb said he wanted to get this into Milestone 2, and I share that desire. If there are simple revisions that need to be made, I can turn that around in 2-3 hours this afternoon. If there are more major revisions that need to be made, it's going to take me several days. Juergen Hoeller, has this reached the top of your list yet?

[spring-projects-issues]

Nick Williams commented

Juergen Hoeller, it has been almost four months since I submitted pull request 298. I really want to see this get merged into RC1. By this point, I'm concerned about whether it is even mergable anymore. There have been a lot of changes to master in the last four months. I haven't even received any guidance yet on what should be changed to make it acceptable (and I would need at least a week to make those changes).

Can you review and provide some feedback, please?

Thanks.

[spring-projects-issues]

Juergen Hoeller commented

Nick,

FYI, I'm trying a slightly different approach with a TimeZoneAwareLocaleResolver extending LocaleResolver. This feels quite straightforward and seems to lead to less impact overall, with the existing LocaleResolver strategies simply implementing the same strategy for TimeZone handing as well if they can (i.e. for cookies and session). Some mixing and matching is always possible in subclasses, or combining dynamic locale resolution with a fixed time zone (through setting setDefaultTimeZone - now on AbstractLocaleResolver). I've also added TimeZone access to LocaleContextHolder, backed by a TimeZoneAwareLocaleContext, with our JodaTimeContext and the JSR-310 DateTimeContext checking the TimeZone there if none has been set for Joda/JSR-310 specifically.

I'm wrapping this up now and will commit it tonight. I hope you're ok with the combined LocaleResolver/LocaleContext approach: It's less spread out that way, with the existing points of configuration simply additionally dealing with a time zone. There's also less passing through, since it's all just based on existing LocaleResolver references now.

Juergen

[spring-projects-issues]

Nick Williams commented

That approach works fine with me. Sounds like it makes a lot of sense. So you won't need me to make revisions to my pull request? You're making them instead?

[spring-projects-issues]

Juergen Hoeller commented

After a further iteration, I went with with a LocaleContextResolver (extends LocaleResolver) now, returning and accepting an org.springframework.context.i18n.LocaleContext. This allows for direct exposure to LocaleContextHolder there, and keeps the SPI flexible enough to accommodate further locale-related information in the future (through LocaleContext extensions). For the time being, we have (Simple)TimeZoneAwareLocaleContext and a corresponding getTimeZone() convenience accessor on LocaleContextHolder.

Nick, the version committed now is conceptually based on your pull request but didn't actually pull it. It turned out to be easier to implement the revised approach in a fresh cut. There are a few further intentional differences beyond the LocaleResolver extension approach: e.g. there's no TimeZoneChangeInterceptor since I have yet to see somebody using an on-the-fly time zone switcher on every page in their website, based on a plain parameter convention. This is common for language switches but not for time zones, I feel. Instead, I've made it more convenient to programmatically change the locale and time zone in specific controllers, through new "changeLocale" methods on RequestContext. Alternatively, people may of course inject their LocaleResolver and call the setLocale(Context) methods directly.

Let me know whether I've been missing something. There might be a few further tests that we could add, for example. Other than that, I'm pretty happy with the current cut - and grateful for your pull request which served as a great guideline! We hardly ever get pull requests of that extent, that's for sure.

Juergen

[spring-projects-issues]

Nick Williams commented

Juergen Hoeller,

Sorry about the delay in responding. I've been pretty busy lately.

First, I'd like to say thanks for making this happen. I am thrilled that Spring Framework 4 will finally have time zone support, and I'm glad I could be part of making it happen. For the most part, your commit looks good. I did have a few comments though:

• The LocaleContextHolder's new getTimeZone method does, indeed, make getting the time zone easier. But the absence of a setTimeZone method seems to make things more difficult, IMO.
• If I set the time zone (currently by calling LocaleContextHolder#setLocaleContext(new TimeZoneAwareContextHolder(...))), and then some other code sets the locale by calling LocaleContextHolder#setLocale(...), the set time zone will be erased. The setLocale methods need to check if there is an existing TimeZoneAwareLocaleContext and, if there is, create a new SimpleTimeZoneAwareLocaleContext using the existing time zone and the new locale.
• I noticed you retained the @author Nicholas Williams tags only on two test classes. I understand that you pushed a substantially different commit, but it seems that there are a lot of new classes, and changes to existing classes, that are largely based on new classes or changes to existing classes in my pull request (ZoneIdEditor and TimeZoneEditor are the first two that come to mind). I hate to nitpick, but I would certainly appreciate being given joint credit with you for those changes. As you know, "having your name on something" is part of what motivates many in the community to submit pull requests in the first place, and I spent considerable time on this one. :-)

Thanks,

Nick

[spring-projects-issues]

Juergen Hoeller commented

Nick, those are both good points. I'm about to sort them out tonight, so they'll certainly make 4.0 RC2.

As for the author tags, for what it's worth, I simply kept them on the files that started as a copy of yours for me locally. It is indeed appropriate to add a few further attributions to files that were clearly influenced by your pull request as well...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

I've refined LocaleContextHolder's setLocale accordingly now and introduced setTimeZone methods. I've also fixed the toString methods in Simple(TimeZoneAware)LocaleContext to never lead to an NPE. And I've added a few author tags :-)

Juergen

[spring-projects-issues]

Nick Williams commented

Excellent! Looks great, now! Thanks again, Juergen Hoeller!