Wicket
  1. Wicket
  2. WICKET-4032

ComponentStringResourceLoader must not include the index of repeater items in resource lookup but still resolve properties to them

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.18, 1.5-RC7
    • Fix Version/s: 1.4.19, 1.5.1
    • Component/s: wicket
    • Labels:
      None

      Description

      Issue when using a StringResourceModel to lookup a resource for a component underneath a repeating view.

      When a StringResourceModel is used by a component under a repeating view, the ComponentStringResourceLoader doesn't find the resource.

      This seems to be a problem introduced by the fix for 3671.

      In ComponentStringResourceLoader, getResourcePath excludes all AbstractRepeaters, however getComponentStack doesn't leading to the two being out of sync for the elements of the component hierarchy under the repeating view.

      1. resource-lookup.zip
        34 kB
        Nathan Messer
      2. resourceloadingissue.patch
        0.5 kB
        Nathan Messer
      3. resourceloadingissue.patch
        0.5 kB
        Nathan Messer
      4. resource-lookup2.zip
        44 kB
        Nathan Messer

        Activity

        Hide
        Nathan Messer added a comment -

        Quickstart that works in 1.4.17 but not in 1.4.18

        Show
        Nathan Messer added a comment - Quickstart that works in 1.4.17 but not in 1.4.18
        Hide
        Nathan Messer added a comment -

        I've attached a quick-start that works in 1.4.17 but not in 1.4.18 to show the problem

        Show
        Nathan Messer added a comment - I've attached a quick-start that works in 1.4.17 but not in 1.4.18 to show the problem
        Hide
        Nathan Messer added a comment -

        Patch to fix the issue.
        This excludes AbstractRepeaters from the component stack, unlike the getResourcePath method that excludes AbstractRepeaters children. This leaves the path hierachy sensible, in that it has no 0, 1 etc ids in it, but also leaves the component hierarchy so that components added as children of abstract repeaters can have property files that will be looked in for resources.

        Show
        Nathan Messer added a comment - Patch to fix the issue. This excludes AbstractRepeaters from the component stack, unlike the getResourcePath method that excludes AbstractRepeaters children. This leaves the path hierachy sensible, in that it has no 0, 1 etc ids in it, but also leaves the component hierarchy so that components added as children of abstract repeaters can have property files that will be looked in for resources.
        Hide
        Nathan Messer added a comment -

        Version of the patch with a license granted

        Show
        Nathan Messer added a comment - Version of the patch with a license granted
        Hide
        Peter Ertl added a comment -

        thanks for reporting... the problem is fixed in trunk ... please report if you still got issues

        Show
        Peter Ertl added a comment - thanks for reporting... the problem is fixed in trunk ... please report if you still got issues
        Hide
        Nathan Messer added a comment - - edited

        Unfortunately that doesn't fix the problem. It excludes components with a parent of AbstractRepeater from the component stack, not the AbstractRepeaters themselves.

        This then means that if it is the child of the abstract repeater which has the associated properties file, it won't get checked.

        I've attached another quickstart showing the problem.

        The patch I've submitted does handle this case as well, although it may be that there are other problems with it I'm not aware of.

        The patch I submitted won't check if the AbstractRepeater itself has an associated properties file that may contain resources, which may be a problem.

        Show
        Nathan Messer added a comment - - edited Unfortunately that doesn't fix the problem. It excludes components with a parent of AbstractRepeater from the component stack, not the AbstractRepeaters themselves. This then means that if it is the child of the abstract repeater which has the associated properties file, it won't get checked. I've attached another quickstart showing the problem. The patch I've submitted does handle this case as well, although it may be that there are other problems with it I'm not aware of. The patch I submitted won't check if the AbstractRepeater itself has an associated properties file that may contain resources, which may be a problem.
        Hide
        Peter Ertl added a comment -

        I added a fix that resolves a resource key against AbstractRepeater and it's associated repeater items in the same manner.

        For example in the following hierarchy

        MyPage
        – MyRepeater (id = "repeater")
        – MyRepeaterItem (id = "3")
        – MyWebMarkupContainer (id = "container")

        the key = 'label' resolved against base component "container" will cause a lookup in that order

        • MyPage.properties with key = "repeater.container.label"
        • MyRepeater.properties with key = "container.label"
        • MyRepeaterItem.properties with key = "container.label"
        • MyWebMarkupContainer.properties with key = "label"

        Please verify the fix and let me know if something is broken or missing ...

        Show
        Peter Ertl added a comment - I added a fix that resolves a resource key against AbstractRepeater and it's associated repeater items in the same manner. For example in the following hierarchy MyPage – MyRepeater (id = "repeater") – MyRepeaterItem (id = "3") – MyWebMarkupContainer (id = "container") the key = 'label' resolved against base component "container" will cause a lookup in that order MyPage.properties with key = "repeater.container.label" MyRepeater.properties with key = "container.label" MyRepeaterItem.properties with key = "container.label" MyWebMarkupContainer.properties with key = "label" Please verify the fix and let me know if something is broken or missing ...
        Hide
        Peter Ertl added a comment -

        this seems to affect 1.5 also

        Show
        Peter Ertl added a comment - this seems to affect 1.5 also
        Hide
        Peter Ertl added a comment -

        fixed for 1.5 as well, please check and report any issues... thank you!

        Show
        Peter Ertl added a comment - fixed for 1.5 as well, please check and report any issues... thank you!
        Hide
        Peter Ertl added a comment -

        leaving this issue open for a while to gather feedback if everything is okay

        Show
        Peter Ertl added a comment - leaving this issue open for a while to gather feedback if everything is okay
        Hide
        Igor Vaynberg added a comment -

        can you add a unit test so we dont lose this functionality please

        Show
        Igor Vaynberg added a comment - can you add a unit test so we dont lose this functionality please
        Hide
        Peter Ertl added a comment -

        @igor: done for 1.4 and 1.5

        Show
        Peter Ertl added a comment - @igor: done for 1.4 and 1.5
        Hide
        Nathan Messer added a comment -

        That works for our use cases, thanks Peter

        Show
        Nathan Messer added a comment - That works for our use cases, thanks Peter
        Hide
        Peter Ertl added a comment -

        @Nathan: Thanks for reporting. We always want things to get better

        Show
        Peter Ertl added a comment - @Nathan: Thanks for reporting. We always want things to get better

          People

          • Assignee:
            Peter Ertl
            Reporter:
            Nathan Messer
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development