Wicket
  1. Wicket
  2. WICKET-3948

IResourceCachingStrategy is too much bound to PackageResource, make it more general

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5-RC5.1
    • Fix Version/s: 1.5-RC6
    • Component/s: wicket
    • Labels:
      None

      Description

      Somebody recently complained on the wicket userlist that IResourceCachingStrategy is not very versatile but only useable for PackageResources. I 100% agree would like to generalize the caching part by introducing the interface 'ICacheableResourceReference' (instead of referring to PackageResource). This makes it easy to use caching for other resource types. Since we are so close to 1.5 I would like to ask first if the patch is acceptable by you.

      1. cache7.patch
        53 kB
        Peter Ertl

        Activity

        Hide
        Martin Grigorov added a comment -

        Few questions:

        1) Why ResourceReference.getLastModificationTime() is removed ?
        2) Can we add a test or wicket-example with non-Package ResRef. For example with context relative resources as the user asked in the mailing lists.

        Show
        Martin Grigorov added a comment - Few questions: 1) Why ResourceReference.getLastModificationTime() is removed ? 2) Can we add a test or wicket-example with non-Package ResRef. For example with context relative resources as the user asked in the mailing lists.
        Hide
        Peter Ertl added a comment -

        1) actually there was no reference to getLastModificationTime() in wicket besides the caching code. We can keep it there if it makes sense, no problem for me.

        2) I am currently trying to set up a sample...

        Show
        Peter Ertl added a comment - 1) actually there was no reference to getLastModificationTime() in wicket besides the caching code. We can keep it there if it makes sense, no problem for me. 2) I am currently trying to set up a sample...
        Hide
        Peter Ertl added a comment -

        reworked the caching code

        I think it's cleaner now and also supports ContextRelativeResource (including a test case).

        Should ResourceReference#getResource() be considered an expensive operation since I had to use it?

        Show
        Peter Ertl added a comment - reworked the caching code I think it's cleaner now and also supports ContextRelativeResource (including a test case). Should ResourceReference#getResource() be considered an expensive operation since I had to use it?
        Hide
        Martin Grigorov added a comment -

        #getResource() is used also by ResourceMapper to map the correct ResRef+Resource (see line 156 in current trunk). It would be nice if the returned resource is used both for #equals() check and for the caching.

        Additionally I see in the patch:
        if (Strings.isEmpty(cacheUrl.getFileName()))
        +

        { + throw new IllegalStateException("caching strategy returned empty name for " + resource); + }

        Recently Dashorst improved BasicResourceReferenceMapper to return null and log a debug message instead.
        Update: I saw that you actually reverted his change. Why ?!

        In AbstractResource
        private IResourceCachingStrategy getCachingStrategy()
        make it protected to be more flexible - use per resource caching strategy, be able to test Resource without the need to have Application in the thread local context.

        Show
        Martin Grigorov added a comment - #getResource() is used also by ResourceMapper to map the correct ResRef+Resource (see line 156 in current trunk). It would be nice if the returned resource is used both for #equals() check and for the caching. Additionally I see in the patch: if (Strings.isEmpty(cacheUrl.getFileName())) + { + throw new IllegalStateException("caching strategy returned empty name for " + resource); + } Recently Dashorst improved BasicResourceReferenceMapper to return null and log a debug message instead. Update: I saw that you actually reverted his change. Why ?! In AbstractResource private IResourceCachingStrategy getCachingStrategy() make it protected to be more flexible - use per resource caching strategy, be able to test Resource without the need to have Application in the thread local context.
        Hide
        Peter Ertl added a comment -

        Martin, thanks for your feedback which I really appreciate!

        Using #equals() works for ContextRelativeResource where I changed it appropriately (much nicer now).

        Unfortunately #equals() will not work for PackageResource since we have to make the caching dependent on the response from the resource, not the resource itself. See for youself in the 'pub' example and the 'beer' image in wicket-examples. The same PackageResource with locale = null could result in returning 'image_en.jpg' for an English browser locale and 'image_de.jpg' for a German one.

        As fas as I remember Martijn had the case that a path was requested with an empty filename (some resource path ending with '/' ) which caused loads of ugly exceptions in his log.

        However the proper solution should be to not invoke the caching strategy for empty filenames at all. However a caching strategy that gets a non-empty filename and returns an empty filename has to be treated as broken and surely deserves an exception.

        Show
        Peter Ertl added a comment - Martin, thanks for your feedback which I really appreciate! Using #equals() works for ContextRelativeResource where I changed it appropriately (much nicer now). Unfortunately #equals() will not work for PackageResource since we have to make the caching dependent on the response from the resource, not the resource itself. See for youself in the 'pub' example and the 'beer' image in wicket-examples. The same PackageResource with locale = null could result in returning 'image_en.jpg' for an English browser locale and 'image_de.jpg' for a German one. As fas as I remember Martijn had the case that a path was requested with an empty filename (some resource path ending with '/' ) which caused loads of ugly exceptions in his log. However the proper solution should be to not invoke the caching strategy for empty filenames at all. However a caching strategy that gets a non-empty filename and returns an empty filename has to be treated as broken and surely deserves an exception.
        Hide
        Peter Ertl added a comment -

        updated the patch btw

        Show
        Peter Ertl added a comment - updated the patch btw
        Hide
        Martin Grigorov added a comment -

        In ContextRelativeResource:
        public Serializable getCacheKey()
        +

        { + return this; + }

        Can we return something like: "contextRelative:" + path. This way the cache key wont keep a reference to the resource instance.

        Why PackageResourceReference has #getCacheKey() ? The caching now works with IResource (IStaticCacheableResource), not with ResRefs, right ?

        Show
        Martin Grigorov added a comment - In ContextRelativeResource: public Serializable getCacheKey() + { + return this; + } Can we return something like: "contextRelative:" + path. This way the cache key wont keep a reference to the resource instance. Why PackageResourceReference has #getCacheKey() ? The caching now works with IResource (IStaticCacheableResource), not with ResRefs, right ?
        Hide
        Peter Ertl added a comment -

        PackageResourceReference#getCacheKey() was a left-over from refactoring. Can't wait to get JDK 6 and @Override on interfaces

        "contextRelative:" + path will do fine (despite the fact that there's a 0.000001% collision probability that somebody else uses the same key in another resource type).

        I removed #getLastModified from ResourceReference since it's not referred anymore

        Currently (in the patch)

        resource.getResourceStream().lastModifiedTime()

        is used.

        patch has been updated...

        Show
        Peter Ertl added a comment - PackageResourceReference#getCacheKey() was a left-over from refactoring. Can't wait to get JDK 6 and @Override on interfaces "contextRelative:" + path will do fine (despite the fact that there's a 0.000001% collision probability that somebody else uses the same key in another resource type). I removed #getLastModified from ResourceReference since it's not referred anymore Currently (in the patch) resource.getResourceStream().lastModifiedTime() is used. patch has been updated...
        Hide
        Martin Grigorov added a comment -

        No more comments from me.

        Show
        Martin Grigorov added a comment - No more comments from me.
        Hide
        Igor Vaynberg added a comment -

        looks ok to me, lets apply it and flush out any problems.

        Show
        Igor Vaynberg added a comment - looks ok to me, lets apply it and flush out any problems.
        Hide
        Martin Grigorov added a comment -

        Is there anything else to be done here ?

        Show
        Martin Grigorov added a comment - Is there anything else to be done here ?
        Hide
        Peter Ertl added a comment -

        unless you find any bugs it should be fine

        I am closing this issue for now but won't bother to reopen it for bugs.

        Show
        Peter Ertl added a comment - unless you find any bugs it should be fine I am closing this issue for now but won't bother to reopen it for bugs.

          People

          • Assignee:
            Peter Ertl
            Reporter:
            Peter Ertl
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development