Wicket
  1. Wicket
  2. WICKET-4162

Add new StringResourceLoader to allow Wicket extensions to provide localization resource bundles

    Details

      Description

      Issue:
      It is currently not possible to provide default localization resource bundles for components in wicket extension modules (e.g. wicket-extensions). See issues WICKET-3911 and WICKET-4154 where this was a problem.

      Proposed changes:
      1-Add a new IStringResourceLoader type (WicketExtensionStringResourceLoader?) which checks the root of the jar containing the component for properties in extension.properties.

      2-Insert this new resource loader in the existing default resource loader chain:
      ResourceSettings() constructor:
      stringResourceLoaders.add(new ComponentStringResourceLoader());
      stringResourceLoaders.add(new PackageStringResourceLoader());
      stringResourceLoaders.add(new ClassStringResourceLoader(application.getClass()));
      // New loader inserted here
      stringResourceLoaders.add(new WicketExtensionStringResourceLoader());
      stringResourceLoaders.add(new ValidatorStringResourceLoader());

      3-Start adding extension.properties files in wicket-extension and friends.

      Questions:
      Is a similar effort already underway?
      Are there any better ideas?
      Should the new loader be inserted at the end of the chain instead?
      Do you prefer other names for the loader or the properties file?

      I am prepared to provide a patch sometime next week if this is agreed on.

      1. WICKET-4162.patch
        13 kB
        Sven Meier
      2. jar_resource.patch
        9 kB
        Bertrand Guay-Paquet

        Issue Links

          Activity

          Hide
          Sven Meier added a comment -

          @Bertrand
          Yes, resource keys are no longer isolated. I'm aware of this, but this holds for other resource loaders too.

          @Igor
          I'd expect each initializer to be in its own package. But you have a point here, this solution could break. It's a compromise, but at least it fits nicely into the current resource loading.

          Show
          Sven Meier added a comment - @Bertrand Yes, resource keys are no longer isolated. I'm aware of this, but this holds for other resource loaders too. @Igor I'd expect each initializer to be in its own package. But you have a point here, this solution could break. It's a compromise, but at least it fits nicely into the current resource loading.
          Hide
          Sven Meier added a comment -

          default resource strings now provided by InitializerStringResourceLoader

          Show
          Sven Meier added a comment - default resource strings now provided by InitializerStringResourceLoader
          Hide
          Igor Vaynberg added a comment - - edited

          i think this is an ok compromise but not an ideal implementation. imho, the proper way would be to change IResourceStreamLocator methods that return IResourceStream to return IResourceStream[] properly representing the fact that there can be multiple resources on the classpath. but this is a much bigger refactor...

          the intializer solution suffers the same problem if two jars contain an initializer with the same name, eg Initializer that happen to be in the same package not likely, but still possible

          also, i think its ok to backport this into 1.5.x and deprecate the old Jar variant.

          Show
          Igor Vaynberg added a comment - - edited i think this is an ok compromise but not an ideal implementation. imho, the proper way would be to change IResourceStreamLocator methods that return IResourceStream to return IResourceStream[] properly representing the fact that there can be multiple resources on the classpath. but this is a much bigger refactor... the intializer solution suffers the same problem if two jars contain an initializer with the same name, eg Initializer that happen to be in the same package not likely, but still possible also, i think its ok to backport this into 1.5.x and deprecate the old Jar variant.
          Hide
          Bertrand Guay-Paquet added a comment -

          I had a look at your patch and it looks good.

          One observation however: with the previous (flawed) scheme, a resource key namespace was created for each jar. With your modifications, this is no longer the case (i.e. a resource key is looked up in all initializers in whatever order they were added to the application). I don't think this is a problem presently, but some resource keys could be problematic (e.g. NavigatorLabel, UploadStatusResource.status, etc.) because they are somewhat generically named. Should the Wicket built-in resource keys be changed to something prefixed by the name of the module like "wicket-extensions.whatever"?

          As for porting the modifications to 1.5, I don't think anybody had time to depend on the old resource loader class your patch replaces, so I'd vote for porting to 1.5.

          Show
          Bertrand Guay-Paquet added a comment - I had a look at your patch and it looks good. One observation however: with the previous (flawed) scheme, a resource key namespace was created for each jar. With your modifications, this is no longer the case (i.e. a resource key is looked up in all initializers in whatever order they were added to the application). I don't think this is a problem presently, but some resource keys could be problematic (e.g. NavigatorLabel, UploadStatusResource.status, etc.) because they are somewhat generically named. Should the Wicket built-in resource keys be changed to something prefixed by the name of the module like "wicket-extensions.whatever"? As for porting the modifications to 1.5, I don't think anybody had time to depend on the old resource loader class your patch replaces, so I'd vote for porting to 1.5.
          Hide
          Sven Meier added a comment -

          We have the following problem with the current solution:
          All jars in a web application are loaded via the same classloader, thus resulting in naming conflicts, if multiple jars want to provide "wicket-jar.properties". Wicket's resource loading isn't aware of such a situation and I don't think we should change this.

          I've replaced the implementation for default resources instead in trunk:
          Now IInitalizers can provide resources (see new InitializerStringResourceLoader), working similar as applications specific resources. Furthermore this solution supports resources for non-components too, see UploadStatusResource.

          If no one objects, I'll backport this fix to 1.5.x

          Show
          Sven Meier added a comment - We have the following problem with the current solution: All jars in a web application are loaded via the same classloader, thus resulting in naming conflicts, if multiple jars want to provide "wicket-jar.properties". Wicket's resource loading isn't aware of such a situation and I don't think we should change this. I've replaced the implementation for default resources instead in trunk : Now IInitalizers can provide resources (see new InitializerStringResourceLoader), working similar as applications specific resources. Furthermore this solution supports resources for non-components too, see UploadStatusResource. If no one objects, I'll backport this fix to 1.5.x
          Hide
          Sven Meier added a comment -

          This logic is located in ResourceStreamLocator. I don't want to change this class (I'm not sure it is even possible to change it this deep into Wicket's resource loading) but I don't want to reimplement resource loading in JarStringResourceLoader neither.

          Show
          Sven Meier added a comment - This logic is located in ResourceStreamLocator. I don't want to change this class (I'm not sure it is even possible to change it this deep into Wicket's resource loading) but I don't want to reimplement resource loading in JarStringResourceLoader neither.
          Hide
          Igor Vaynberg added a comment -

          use getResources() instead..

          Show
          Igor Vaynberg added a comment - use getResources() instead..
          Hide
          Sven Meier added a comment -

          Seems we still don't have a solution .

          ResourceStreamLocator tries to locate "wicket-jar.properties" from classloader:

          URL url = classLoader.getResource(path);

          But if there are multiple "wicket-jar.properties" files on the class path via the same class loader, it will consider the first found resource only.
          Note that this is the case currently only if you run wicket-examples inside Eclipse:
          /wicket-core/src/test/java/wicket-jar.properties
          /wicket-extensions/src/main/java/wicket-jar.properties
          Both resources have the same name and are loaded via the same class loader.

          Show
          Sven Meier added a comment - Seems we still don't have a solution . ResourceStreamLocator tries to locate "wicket-jar.properties" from classloader: URL url = classLoader.getResource(path); But if there are multiple "wicket-jar.properties" files on the class path via the same class loader, it will consider the first found resource only. Note that this is the case currently only if you run wicket-examples inside Eclipse: /wicket-core/src/test/java/wicket-jar.properties /wicket-extensions/src/main/java/wicket-jar.properties Both resources have the same name and are loaded via the same class loader.
          Hide
          Igor Vaynberg added a comment -

          resource loading from wicket-jar is not working. start examples and go to:

          http://localhost:8080/repeater/wicket/bookmarkable/org.apache.wicket.examples.repeater.DataTablePage

          get

          Caused by: java.util.MissingResourceException: Unable to find property: 'NavigatorLabel' for component: table:topToolbars:toolbars:0:span:navigatorLabel [class=org.apache.wicket.extensions.markup.html.repeater.data.table.NavigatorLabel]

          Show
          Igor Vaynberg added a comment - resource loading from wicket-jar is not working. start examples and go to: http://localhost:8080/repeater/wicket/bookmarkable/org.apache.wicket.examples.repeater.DataTablePage get Caused by: java.util.MissingResourceException: Unable to find property: 'NavigatorLabel' for component: table:topToolbars:toolbars:0:span:navigatorLabel [class=org.apache.wicket.extensions.markup.html.repeater.data.table.NavigatorLabel]
          Hide
          Martin Grigorov added a comment -

          Let's see how many users will be confused
          package.properties is not so well known, imo.

          Show
          Martin Grigorov added a comment - Let's see how many users will be confused package.properties is not so well known, imo.
          Hide
          Igor Vaynberg added a comment -

          meh, now its inconsistent between package.properties and wicket-jar.properties

          now i will be trying wicket-package.properties or jar.properties and wonder why it doesnt work...

          Show
          Igor Vaynberg added a comment - meh, now its inconsistent between package.properties and wicket-jar.properties now i will be trying wicket-package.properties or jar.properties and wonder why it doesnt work...
          Hide
          Sven Meier added a comment -

          Added new JarStringResourceLoader to ResourceSettings on last position.

          Show
          Sven Meier added a comment - Added new JarStringResourceLoader to ResourceSettings on last position.
          Hide
          Sven Meier added a comment -

          "wicket-jar.properties", fixed tests, use new string resource loader as last.

          Show
          Sven Meier added a comment - "wicket-jar.properties", fixed tests, use new string resource loader as last.
          Hide
          Martin Grigorov added a comment -

          +1 for wicket-jar.properties in 1.5.x
          Please create a task ticket to prefix package.properties (and others if there are more) for Wicket.next.

          Show
          Martin Grigorov added a comment - +1 for wicket-jar.properties in 1.5.x Please create a task ticket to prefix package.properties (and others if there are more) for Wicket.next.
          Hide
          Sven Meier added a comment -

          wicket-jar.properties and all tests working

          Show
          Sven Meier added a comment - wicket-jar.properties and all tests working
          Hide
          Sven Meier added a comment -

          Sorry, but I need your opinions on this once again:

          After pondering about this issue a few days, I now prefer Bertrand's initial suggestion "wicket-jar.properties", as "jar" matches "package" much better.
          The fact that these properties are used as defaults isn't inherent with these files, it's a matter of configuration in ResourceSettings.

          Additionally I don't think we should defer prefixing these files with "wicket-" namespace until package properties have been migrated. Why not doing it right now for this new feature?

          Show
          Sven Meier added a comment - Sorry, but I need your opinions on this once again: After pondering about this issue a few days, I now prefer Bertrand's initial suggestion "wicket-jar.properties", as "jar" matches "package" much better. The fact that these properties are used as defaults isn't inherent with these files, it's a matter of configuration in ResourceSettings. Additionally I don't think we should defer prefixing these files with "wicket-" namespace until package properties have been migrated. Why not doing it right now for this new feature?
          Hide
          Igor Vaynberg added a comment -

          i think simply default.properties will do, at least it matches package.properties.

          if we want to namespace these with wicket- we should do it for both, maybe in wicket.next.

          Show
          Igor Vaynberg added a comment - i think simply default.properties will do, at least it matches package.properties. if we want to namespace these with wicket- we should do it for both, maybe in wicket.next.
          Hide
          Sven Meier added a comment -

          No need for another patch, thank you.

          Let's wait a little for the opinion of other devs.

          Show
          Sven Meier added a comment - No need for another patch, thank you. Let's wait a little for the opinion of other devs.
          Hide
          Bertrand Guay-Paquet added a comment -

          Sure "wicket-default.properties" is fine and I agree with your point. wicket-jar emphasizes the "jar" aspect of the feature while wicket-default does as you described. In my opinion both are good candidates, so if you think wicket-default is better, let's settle on that!

          Should I send another patch for this change?

          Show
          Bertrand Guay-Paquet added a comment - Sure "wicket-default.properties" is fine and I agree with your point. wicket-jar emphasizes the "jar" aspect of the feature while wicket-default does as you described. In my opinion both are good candidates, so if you think wicket-default is better, let's settle on that! Should I send another patch for this change?
          Hide
          Sven Meier added a comment -

          I've taken another look at your patch and I think it's working exactly like it should.

          For WICKET-3911 we need another solution, Wicket's string loading is optimized for components, not resources.

          One question remains: the file name. IMHO "wicket-default.properties" would stress the fact that these properties are used as defaults only (similar to the term "defaultValue" in Localizer).

          WDYT?

          Show
          Sven Meier added a comment - I've taken another look at your patch and I think it's working exactly like it should. For WICKET-3911 we need another solution, Wicket's string loading is optimized for components, not resources. One question remains: the file name. IMHO "wicket-default.properties" would stress the fact that these properties are used as defaults only (similar to the term "defaultValue" in Localizer). WDYT?
          Hide
          Bertrand Guay-Paquet added a comment -

          In response to the first problem:
          In fact this is an intentional feature. Consider the case where I subclass DataTable in my app's jar. I still want to benefit from the strings in wicket-extensions for it.

          In response to the second problem:
          You are right. Here is what UploadStatusResource uses to lookup a string (simplified):
          new StringResourceModel(
          "UploadStatusResource.status",
          (Component)null,
          Model.of(info),
          "default value").getString();

          Down the line, when the JarStringResourceLoader is called, it doesn't have any practical means of finding which jar should be inspected because the component is null.

          I see 2 options, neither of which I like:
          1-Walk up the call stack before Localizer and StringResourceModel to find in which jar the getString() call originated. I haven't researched this one much so I am not sure it is even feasible.

          2-When the component is null, infer the class whose jar must be inspected based on the resource key. For example, the key from the example above would need to be changed to "org.apache.wicket.extensions.ajax.markup.html.form.upload.UploadStatusResource.status". To allow for "." in the resource key (e.g. status.success), the inferring process would need to walk up the key string along '.' character boundaries.

          Is there another way?

          Show
          Bertrand Guay-Paquet added a comment - In response to the first problem: In fact this is an intentional feature. Consider the case where I subclass DataTable in my app's jar. I still want to benefit from the strings in wicket-extensions for it. In response to the second problem: You are right. Here is what UploadStatusResource uses to lookup a string (simplified): new StringResourceModel( "UploadStatusResource.status", (Component)null, Model.of(info), "default value").getString(); Down the line, when the JarStringResourceLoader is called, it doesn't have any practical means of finding which jar should be inspected because the component is null. I see 2 options, neither of which I like: 1-Walk up the call stack before Localizer and StringResourceModel to find in which jar the getString() call originated. I haven't researched this one much so I am not sure it is even feasible. 2-When the component is null, infer the class whose jar must be inspected based on the resource key. For example, the key from the example above would need to be changed to "org.apache.wicket.extensions.ajax.markup.html.form.upload.UploadStatusResource.status". To allow for "." in the resource key (e.g. status.success), the inferring process would need to walk up the key string along '.' character boundaries. Is there another way?
          Hide
          Sven Meier added a comment -

          Thanks for the patch.

          I see two problems at the moment:

          • it seems string loaded by JarStringResourceLoader are not isolated, e.g. Component A from jar aa would be able to get strings from jar bb (I'm not sure this is something we want to prevent).
          • non-component strings are not supported, i.e. we're not be able to use this enhancement for WICKET-3911 (UploadStatusResource is no component)
          Show
          Sven Meier added a comment - Thanks for the patch. I see two problems at the moment: it seems string loaded by JarStringResourceLoader are not isolated, e.g. Component A from jar aa would be able to get strings from jar bb (I'm not sure this is something we want to prevent). non-component strings are not supported, i.e. we're not be able to use this enhancement for WICKET-3911 (UploadStatusResource is no component)
          Hide
          Bertrand Guay-Paquet added a comment -

          I settled on wicket-jar.properties. Here's why I didn't use wicket.jar.properties :
          The ResourceNameIterator did not cooperate with the '.' in the "wicket.jar" path. Because of special interpretation of the ".jar" as an extension, tt did not provide a '.' charater at the end of "wicket.jar". So instead of looking up "wicket.jar.properties", "wicket.jarproperties" was used.

          The test case for a component located in another jar is missing. Short of including a binary jar as a test resource, I can't see how to build a test case for it. I did however test it with wicket-extensions and it works just fine.

          Show
          Bertrand Guay-Paquet added a comment - I settled on wicket-jar.properties. Here's why I didn't use wicket.jar.properties : The ResourceNameIterator did not cooperate with the '.' in the "wicket.jar" path. Because of special interpretation of the ".jar" as an extension, tt did not provide a '.' charater at the end of "wicket.jar". So instead of looking up "wicket.jar.properties", "wicket.jarproperties" was used. The test case for a component located in another jar is missing. Short of including a binary jar as a test resource, I can't see how to build a test case for it. I did however test it with wicket-extensions and it works just fine.
          Hide
          Bertrand Guay-Paquet added a comment -

          proposed modifications

          Show
          Bertrand Guay-Paquet added a comment - proposed modifications
          Hide
          Martin Grigorov added a comment -

          wicket.jar.properties ?
          Use wicket as namespace to avoid collisions with someone else's code.

          Show
          Martin Grigorov added a comment - wicket.jar.properties ? Use wicket as namespace to avoid collisions with someone else's code.
          Hide
          Sven Meier added a comment -

          I agree that this is the way to go: "fallback.properties" or "jar.properties" are my suggestions for file name alternatives.

          Show
          Sven Meier added a comment - I agree that this is the way to go: "fallback.properties" or "jar.properties" are my suggestions for file name alternatives.

            People

            • Assignee:
              Sven Meier
              Reporter:
              Bertrand Guay-Paquet
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development