Wicket
  1. Wicket
  2. WICKET-4270

Remove superfluous call to ResourceReference#getResource() in ResourceMapper when trying to map the resource URL

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.3, 6.0.0-beta1
    • Fix Version/s: 1.5.4, 6.0.0-beta1
    • Component/s: wicket
    • Labels:
      None

      Description

      The getResource() method is being called for a mounted ResourceReference even when the url does not match. So far it seems to only happen when some wicket Ajax is included in the page. To get around the problem, I've added a check on my ResourceReference that looks for the mounted name in the request url but it seems like the mounting should handle that.

      To reproduce the error:
      1. Call mountResource("uploadedfiles", MyResourceReference.class);
      2. Add an AjaxFallbackLink to a page
      3. In MyResourceReference getResource() method return null;
      4. Run the application and open the page with an AjaxFallbackLink and you will get
      java.lang.NullPointerException
      at org.apache.wicket.request.mapper.ResourceMapper.mapHandler(ResourceMapper.java:167)
      at org.apache.wicket.request.mapper.CompoundRequestMapper.mapHandler(CompoundRequestMapper.java:156)
      at org.apache.wicket.request.cycle.RequestCycle.mapUrlFor(RequestCycle.java:401)
      at org.apache.wicket.request.cycle.RequestCycle.urlFor(RequestCycle.java:485)
      at org.apache.wicket.markup.html.internal.HeaderResponse.renderJavaScriptReference(HeaderResponse.java:205)
      ...

      The class org.apache.wicket.request.mapper.ResourceMap calls the getResource() (line 167) method even though the url does not match the mounted name.

      1. resource_example.zip
        66 kB
        Philip McCullick

        Activity

        Hide
        Philip McCullick added a comment -

        A sample quickstart that has a HomePage with a single AjaxFallbackLink and a mounted resource.

        Show
        Philip McCullick added a comment - A sample quickstart that has a HomePage with a single AjaxFallbackLink and a mounted resource.
        Hide
        Sven Meier added a comment -

        This has nothing to do with matching of an URL but the inverse:
        ResourceMapper checks whether it can map a request handler to a URL. For this it checks equality on the resource reference and the resource itself.

        What problem do you see here, and why aren't you implementing MyResourceReference#getResource() properly?

        Show
        Sven Meier added a comment - This has nothing to do with matching of an URL but the inverse: ResourceMapper checks whether it can map a request handler to a URL. For this it checks equality on the resource reference and the resource itself. What problem do you see here, and why aren't you implementing MyResourceReference#getResource() properly?
        Hide
        Philip McCullick added a comment -

        The return null in the example is just to show that the MyResourceReference.getResource method was being called. I was suprised to see that getResource was called on pages where there were no instances of my resource reference. It makes sense that it searches for a ResourceReference that returns a IResource that equals the requested resource. The name of the method was what was confusing me. The comment for ResourceReference.getResource says something like "Gets the resource" so I assumed that it was alright to actually load a file at that point. So the getResource should really pass a light version of the resource since it will be called multiple times by other resources trying try to find the right handler.

        You can close the ticket, I don't see an option for me to close it.

        Show
        Philip McCullick added a comment - The return null in the example is just to show that the MyResourceReference.getResource method was being called. I was suprised to see that getResource was called on pages where there were no instances of my resource reference. It makes sense that it searches for a ResourceReference that returns a IResource that equals the requested resource. The name of the method was what was confusing me. The comment for ResourceReference.getResource says something like "Gets the resource" so I assumed that it was alright to actually load a file at that point. So the getResource should really pass a light version of the resource since it will be called multiple times by other resources trying try to find the right handler. You can close the ticket, I don't see an option for me to close it.
        Hide
        Sven Meier added a comment -

        > ResourceReference.getResource ... I assumed that it was alright to actually load a file at that point.

        Yes indeed, getResource() has to be a lightweight operation.

        Show
        Sven Meier added a comment - > ResourceReference.getResource ... I assumed that it was alright to actually load a file at that point. Yes indeed, getResource() has to be a lightweight operation.
        Hide
        Sven Meier added a comment -

        Works as expected.

        Show
        Sven Meier added a comment - Works as expected.
        Hide
        Martin Grigorov added a comment -

        I wonder whether
        if (resourceReference.equals(handler.getResourceReference()) == false &&
        resourceReference.getResource().equals(handler.getResource()) == false)

        { return null; }

        is correct.

        If the ResourceReference doesn't match I see no reason to check its resource too. I think we should use '||' instead.

        Show
        Martin Grigorov added a comment - I wonder whether if (resourceReference.equals(handler.getResourceReference()) == false && resourceReference.getResource().equals(handler.getResource()) == false) { return null; } is correct. If the ResourceReference doesn't match I see no reason to check its resource too. I think we should use '||' instead.
        Hide
        Sven Meier added a comment -

        '||' might be to broad: What if the references are equal, but the resources are instantiated on each invocation?
        Currently this condition will pass, with '||' it wouldn't map a url.

        But why do we check the resource at all? If ResourceMapper is mapping a specific resourceReference, shouldn't an equality test on the reference be sufficient?

        Show
        Sven Meier added a comment - '||' might be to broad: What if the references are equal, but the resources are instantiated on each invocation? Currently this condition will pass, with '||' it wouldn't map a url. But why do we check the resource at all? If ResourceMapper is mapping a specific resourceReference, shouldn't an equality test on the reference be sufficient?
        Hide
        Martin Grigorov added a comment -

        I mean: if the ResRef doesn't match then don't check its resource too.
        I'm also not sure why we check the resource itself when we actually mount the ResRef.

        WICKET-3506 is related but with it the ResRef has been added in the condition.

        @Pertl, @Igor: do you know why ?

        Show
        Martin Grigorov added a comment - I mean: if the ResRef doesn't match then don't check its resource too. I'm also not sure why we check the resource itself when we actually mount the ResRef. WICKET-3506 is related but with it the ResRef has been added in the condition. @Pertl, @Igor: do you know why ?
        Hide
        Peter Ertl added a comment -

        originally we had this:

        // see if request handler addresses the resource we serve
        if (resourceReference.getResource().equals(handler.getResource()) == false)

        { return null; }

        then there came WICKET-3506 and Igor changed this location to:

        // see if request handler addresses the resource we serve
        if (resourceReference.equals(handler.getResourceReference()) == false &&
        resourceReference.getResource().equals(handler.getResource()) == false)
        { return null; }

        the commit message tells us:

        " added equals/hashcode to some iresource impls. change resource mapper to compare resourcereferences
        " as well as iresource instances for better matching. Issue: WICKET-3506

        So far and after reading WICKET-3506 this makes sense for me...

        The option for improvement would be to just compare ResourceReference with equals()

        This would require a properly working ResourceReference#equals()

        The current ResourceReference#equals() works by comparing the resource key:

        public final static class Key implements Serializable

        { // ....... final String scope; final String name; final Locale locale; final String style; final String variation; // ......... }

        Since all descendant classes of ResourceReferences must provide a valid key equals should probably always work as expected.

        I don't want to suggest this before Igor gave us his opinion, but basically just comparing ResourceReference should be fine imho

        Show
        Peter Ertl added a comment - originally we had this: // see if request handler addresses the resource we serve if (resourceReference.getResource().equals(handler.getResource()) == false) { return null; } then there came WICKET-3506 and Igor changed this location to: // see if request handler addresses the resource we serve if (resourceReference.equals(handler.getResourceReference()) == false && resourceReference.getResource().equals(handler.getResource()) == false) { return null; } the commit message tells us: " added equals/hashcode to some iresource impls. change resource mapper to compare resourcereferences " as well as iresource instances for better matching. Issue: WICKET-3506 So far and after reading WICKET-3506 this makes sense for me... The option for improvement would be to just compare ResourceReference with equals() This would require a properly working ResourceReference#equals() The current ResourceReference#equals() works by comparing the resource key: public final static class Key implements Serializable { // ....... final String scope; final String name; final Locale locale; final String style; final String variation; // ......... } Since all descendant classes of ResourceReferences must provide a valid key equals should probably always work as expected. I don't want to suggest this before Igor gave us his opinion, but basically just comparing ResourceReference should be fine imho
        Hide
        Igor Vaynberg added a comment -

        i think we should be ok comparing just the references...

        Show
        Igor Vaynberg added a comment - i think we should be ok comparing just the references...
        Hide
        Peter Ertl added a comment -

        since we all agree I removed the check using getResource() so we actually just compare the ResourceReference using equals() ... fixed for 1.5 und 6.x

        Show
        Peter Ertl added a comment - since we all agree I removed the check using getResource() so we actually just compare the ResourceReference using equals() ... fixed for 1.5 und 6.x

          People

          • Assignee:
            Peter Ertl
            Reporter:
            Philip McCullick
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development