Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4734

I18Interceptor ignores session or cookie Locale after first lookup failure

    Details

      Description

      In Struts 2.5.5 and prior 2.5.x/2.3.x, the I18nInterceptor honoured the locale state set programmatically via session (e.g. session.put(I18nInterceptor.DEFAULT_SESSION_ATTRIBUTE, localeForUser); ).

      Struts 2.5.8 ignores a locale state set programmatically via session (or at least it does so after the 1st failed lookup).

      *Issue cause: Changes in 2.5.8 storeLocale()/readStoredLocale() behaviour causes "storage == Storage.NONE" after 1st lookup failure, and i18nInterceptor will never again check session/cookie scopes for a locale. Appears to have been introduced with changes for WW-4722.

      *Bug elements:

      1) readStoredLocale() - Never checks session/cookie level for a saved locale (after first lookup failure sets "storage = Storage.NONE").
      2) readStoredLocale() - The "if block" checks are inverted. When "storage == Storage.COOKIE" it checks session, when "storage == Storage.SESSION" it checks cookie (appears this was addressed in update to WW-4722 on 2017/01/11).
      3) LocaleFinder/saveLocale() - No longer provides default lookup at session level, no longer preserves the storage level where the lookup succeded.

      *Suggested remedy:

      1) Restore logic equivalent to 2.5.5 and earlier that will always check session and cookie scopes for Locale, irrespective of what the current i18n interceptor instance's storage value is set to.
      Change readStoredLocale() to check all scopes, restore storage scope state to LocaleFinder and calls to saveLocale(). Use LocaleFilender's scope state (tracking immediate request's locale storage level) during request processing (and if possible, leave i18n interceptor's scope fixed/unchanged).
      2) Add logic to I18nInterceptor that preserves the initially configured storage type/scope for the lifetime of the i18n interceptor. This could be done by adding a new protected member to I18nInterceptor (e.g. protected configuredStorage), which gets initialized similarly to "storage" but only modified in the initial setLocaleStorage() call (so its value stays intact until explicitly changed by configuration).

      Either way it appears the I18nInterceptor needs a mechanism to ensure that it will always look for Locale at the session/cookie level (or at the very least the level that was initially configured for the interceptor), irrespective of what the current storage value is set to. Without such logic the i18n interceptor stops looking for anything other than the request/invocation context level (after the first lookup failure, irrespective of original storage setting). Tracking the configured storage scope (global) and the immediate request's scope (local) separately might be appropriate.

      *Note: API documentation for I18nInterceptor "storage" parameter appears incorrect as well. The new configuration parameter in 2.5.8 should indicate "localeStorage" for configuring the locale storage parameter (it indicates "storage" currently, but that fails as it doesn't match the setter's name).

        Activity

        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Great to hear that, thanks a lot

        Show
        lukaszlenart Lukasz Lenart added a comment - Great to hear that, thanks a lot
        Hide
        JcAj4832610521 James Chaplin added a comment -

        Hello Lukasz. Everything seems to work as expected with the 2.5.9 test build (revision 18023, as per your test build comment above), no additional issues noted during an interactive sanity check.

        Show
        JcAj4832610521 James Chaplin added a comment - Hello Lukasz. Everything seems to work as expected with the 2.5.9 test build (revision 18023, as per your test build comment above), no additional issues noted during an interactive sanity check.
        Show
        lukaszlenart Lukasz Lenart added a comment - Wiki was updated https://cwiki.apache.org/confluence/display/WW/I18n+Interceptor
        Hide
        JcAj4832610521 James Chaplin added a comment -

        Ok sorry - I definitely mis-interpreted then . I don't think the missing JavaDoc description is a blocker for the published 2.5.9 test build.

        Show
        JcAj4832610521 James Chaplin added a comment - Ok sorry - I definitely mis-interpreted then . I don't think the missing JavaDoc description is a blocker for the published 2.5.9 test build.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Oh, it's definitely a blocker for 2.5.9 but it was resolved. What I meant, if the missing JavaDoc description is a blocker for already published 2.5.9 test build

        Show
        lukaszlenart Lukasz Lenart added a comment - Oh, it's definitely a blocker for 2.5.9 but it was resolved. What I meant, if the missing JavaDoc description is a blocker for already published 2.5.9 test build
        Hide
        JcAj4832610521 James Chaplin added a comment -

        I might not be understanding the difference between blocker for 2.5.9 and 2.5.8. My intent was to indicate the code fix should definitely be included in 2.5.9 (given the behaviour issue with 2.5.8).

        Since 2.5.8 was already GA I thought the blocker status could only be applied to 2.5.9.

        (If we're talking about the JavaDoc item storage vs. localeStorage, that could be corrected after 2.5.9 since param localeStorage works).

        Show
        JcAj4832610521 James Chaplin added a comment - I might not be understanding the difference between blocker for 2.5.9 and 2.5.8. My intent was to indicate the code fix should definitely be included in 2.5.9 (given the behaviour issue with 2.5.8). Since 2.5.8 was already GA I thought the blocker status could only be applied to 2.5.9. (If we're talking about the JavaDoc item storage vs. localeStorage , that could be corrected after 2.5.9 since param localeStorage works).
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        ... there is a test build of 2.5.9 ready http://markmail.org/thread/lud6ipvgogf3figy

        Show
        lukaszlenart Lukasz Lenart added a comment - ... there is a test build of 2.5.9 ready http://markmail.org/thread/lud6ipvgogf3figy
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        You mean it's a blocker for 2.5.9 or 2.5.8?

        Show
        lukaszlenart Lukasz Lenart added a comment - You mean it's a blocker for 2.5.9 or 2.5.8?
        Hide
        JcAj4832610521 James Chaplin added a comment -

        No problem. I think this issue/fix should probably be a blocker for the 2.5.9 release.

        The only workaround for this issue in 2.5.8 involves replacing the i18n interceptor with a custom implementation (no quick fix via configuration possible), otherwise i18n localization effectively doesn't work for session/cookie (compared to previous 2.5.x and 2.3.x releases).

        Show
        JcAj4832610521 James Chaplin added a comment - No problem. I think this issue/fix should probably be a blocker for the 2.5.9 release. The only workaround for this issue in 2.5.8 involves replacing the i18n interceptor with a custom implementation (no quick fix via configuration possible), otherwise i18n localization effectively doesn't work for session/cookie (compared to previous 2.5.x and 2.3.x releases).
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Cool, thanks for so deep review. I will update the JavaDocs and review description on wiki. Do you think is this a blocker for 2.5.9 release?

        Show
        lukaszlenart Lukasz Lenart added a comment - Cool, thanks for so deep review. I will update the JavaDocs and review description on wiki. Do you think is this a blocker for 2.5.9 release?
        Hide
        JcAj4832610521 James Chaplin added a comment -

        Hello Lukasz.

        The refactored I18nInterceptor.java appears to work as expected now (tried both using the source directly to replace the default i18n interceptor for 2.5.8, and using "struts2-core-2.5.9-20170115.171851-15" core jar). Thanks for looking into the bug and producing a fix for it.

        Note 1: JavaDoc comment for I18nInterceptor.java states: storage (optional) -, but should be localeStorage (optional) - to match the setter (corresponding to <param name="localeStorage">session</param> in the configuration).

        Note 2: According to the old 2.5.5 code it appears that if Storage.COOKIE was configured, storeLocale() stored it in the cookie then set storage type to Storage.SESSION (which caused it also to be stored in session if available). After that the i18n interceptor would be stuck in Storage.SESSION (but lookups would still find things programmatically stored by cookie ... I think). I don't think that wrinkle would negatively impact any 2.5.x apps using Storage.COOKIE with the latest implementation, but I figured it was worth mentioning.

        In any event the recent I18nInterceptor refactoring seems cleaner than the 2.5.5 and 2.5.8 revisions (the configured storage level for the interceptor remains invariant over invocations), and appears to work for Storage.SESSION as expected. If I note any problem later on I will advise. Thanks again.

        Show
        JcAj4832610521 James Chaplin added a comment - Hello Lukasz. The refactored I18nInterceptor.java appears to work as expected now (tried both using the source directly to replace the default i18n interceptor for 2.5.8, and using "struts2-core-2.5.9-20170115.171851-15" core jar). Thanks for looking into the bug and producing a fix for it. Note 1 : JavaDoc comment for I18nInterceptor.java states: storage (optional) - , but should be localeStorage (optional) - to match the setter (corresponding to <param name="localeStorage">session</param> in the configuration). Note 2 : According to the old 2.5.5 code it appears that if Storage.COOKIE was configured, storeLocale() stored it in the cookie then set storage type to Storage.SESSION (which caused it also to be stored in session if available). After that the i18n interceptor would be stuck in Storage.SESSION (but lookups would still find things programmatically stored by cookie ... I think). I don't think that wrinkle would negatively impact any 2.5.x apps using Storage.COOKIE with the latest implementation, but I figured it was worth mentioning. In any event the recent I18nInterceptor refactoring seems cleaner than the 2.5.5 and 2.5.8 revisions (the configured storage level for the interceptor remains invariant over invocations), and appears to work for Storage.SESSION as expected. If I note any problem later on I will advise. Thanks again.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Struts-JDK7-master #585 (See https://builds.apache.org/job/Struts-JDK7-master/585/)
        WW-4734 Fixes proper lookup flow (lukaszlenart: rev 7f80ef1bb52fb10abf85804ef0df35a36d54da96)

        • (edit) core/src/main/java/org/apache/struts2/interceptor/I18nInterceptor.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-JDK7-master #585 (See https://builds.apache.org/job/Struts-JDK7-master/585/ ) WW-4734 Fixes proper lookup flow (lukaszlenart: rev 7f80ef1bb52fb10abf85804ef0df35a36d54da96) (edit) core/src/main/java/org/apache/struts2/interceptor/I18nInterceptor.java
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        I have refactored the interceptor and now it should work as expected. Please review and report any problems

        Show
        lukaszlenart Lukasz Lenart added a comment - I have refactored the interceptor and now it should work as expected. Please review and report any problems
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7f80ef1bb52fb10abf85804ef0df35a36d54da96 in struts's branch refs/heads/master from Lukasz Lenart
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=7f80ef1 ]

        WW-4734 Fixes proper lookup flow

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7f80ef1bb52fb10abf85804ef0df35a36d54da96 in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=7f80ef1 ] WW-4734 Fixes proper lookup flow
        Hide
        lukaszlenart Lukasz Lenart added a comment - - edited

        I had the same feeling but as I didn't understand the decision under the hood I wanted to stick to the existing approach confirmed by a unittest.

        Right now I have dropped changing storage location but the case is with request_only_locale where I want to change locale only for that one request, not permanently. So the interceptor must handle such special case.

        Show
        lukaszlenart Lukasz Lenart added a comment - - edited I had the same feeling but as I didn't understand the decision under the hood I wanted to stick to the existing approach confirmed by a unittest. Right now I have dropped changing storage location but the case is with request_only_locale where I want to change locale only for that one request, not permanently. So the interceptor must handle such special case.
        Hide
        JcAj4832610521 James Chaplin added a comment -

        (Apologies - I'm not used to the text formatting options yet so I'm sticking to plain text).

        Hello Lukasz.

        The change for WW-4722 you mention in the comment above is related, but only part of the issue noted here (it corresponds to element 2 of the bug described above). Even with that latest change the I18nInterceptor behaviour will still be different compared to 2.5.5 and before.

        For example during an initial login (with no previous session or cookie present) the I18nInterceptor will not find a Locale at the session scope and has its storage type set to Storage.NONE. After that even if a Locale is added to that session (I18nInterceptor.DEFAULT_SESSION_ATTRIBUTE) the i18n interceptor will never find it. That appears to hold true forever afterwards (the interceptor's storage type becomes "stuck" at none).

        In 2.5.5 and prior one could set the Locale programmatically (e.g. during or after a login) and the i18n interceptor would find/honour the value. With 2.5.8 it appears broken.

        Even looking at the 2.5.5 and earlier code it isn't clear to me clear why the I18nInterceptor's storage value was dynamically changing anyway. It seems that to work consistently it would always need to search session/cookie in the event there wasn't a specific Locale request parameter (unless I'm missing something ?).

        Show
        JcAj4832610521 James Chaplin added a comment - (Apologies - I'm not used to the text formatting options yet so I'm sticking to plain text). Hello Lukasz. The change for WW-4722 you mention in the comment above is related, but only part of the issue noted here (it corresponds to element 2 of the bug described above). Even with that latest change the I18nInterceptor behaviour will still be different compared to 2.5.5 and before. For example during an initial login (with no previous session or cookie present) the I18nInterceptor will not find a Locale at the session scope and has its storage type set to Storage.NONE. After that even if a Locale is added to that session (I18nInterceptor.DEFAULT_SESSION_ATTRIBUTE) the i18n interceptor will never find it. That appears to hold true forever afterwards (the interceptor's storage type becomes "stuck" at none). In 2.5.5 and prior one could set the Locale programmatically (e.g. during or after a login) and the i18n interceptor would find/honour the value. With 2.5.8 it appears broken. Even looking at the 2.5.5 and earlier code it isn't clear to me clear why the I18nInterceptor's storage value was dynamically changing anyway. It seems that to work consistently it would always need to search session/cookie in the event there wasn't a specific Locale request parameter (unless I'm missing something ?).
        Show
        lukaszlenart Lukasz Lenart added a comment - Is it this related to this change https://issues.apache.org/jira/browse/WW-4722?focusedCommentId=15815515&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15815515 ?

          People

          • Assignee:
            lukaszlenart Lukasz Lenart
            Reporter:
            JcAj4832610521 James Chaplin
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development