Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-5930

Data resouce caching issue while render data resource on multi tenant environment

    Details

    • Sprint:
      Bug Crush Event - 21/2/2015

      Description

      Need to fix the DataResouce caching issue while render the DataResouce on multi tenant environment.
      It might be possible that same data resource id exists for multiple tenant, and as DataResouce caching does not used tenant specific caching so it may lead to incorrect data rendering.
      Here is the current code to get the DataResource Teamplate from cache

      Template cachedTemplate = FreeMarkerWorker.getTemplate("DataResource:" + dataResourceId);
      

      Need to add the delegator name as prefix to fix the issue.

        Activity

        Hide
        lektran Scott Gray added a comment -

        Hi Deepak,

        Before making this change, please investigate the possibility of using the EntityObjectCache which can be accessed from the delegator using:
        delegator.getCache().put(String, EntityCondition, String, T)
        delegator.getCache().get(String, EntityCondition, String)

        Show
        lektran Scott Gray added a comment - Hi Deepak, Before making this change, please investigate the possibility of using the EntityObjectCache which can be accessed from the delegator using: delegator.getCache().put(String, EntityCondition, String, T) delegator.getCache().get(String, EntityCondition, String)
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Here is the patch for the issue.

        Show
        deepak.dixit Deepak Dixit added a comment - Here is the patch for the issue.
        Hide
        lektran Scott Gray added a comment -

        Since you've already done the work, I think this approach is fine as a stop-gap measure. But we definitely need to come up with a better approach for caching data coming from different delegators (multi-tenant or otherwise). There are numerous places in OFBiz where database derived objects are cached and will have this same problem.

        I'll keep thinking about this while I work on the ehcache integration. Thoughts from others would be most welcome!

        Show
        lektran Scott Gray added a comment - Since you've already done the work, I think this approach is fine as a stop-gap measure. But we definitely need to come up with a better approach for caching data coming from different delegators (multi-tenant or otherwise). There are numerous places in OFBiz where database derived objects are cached and will have this same problem. I'll keep thinking about this while I work on the ehcache integration. Thoughts from others would be most welcome!
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Scott- the problem is in the FreeMarker template cache. How will the entity cache fix this?

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Scott- the problem is in the FreeMarker template cache. How will the entity cache fix this?
        Hide
        lektran Scott Gray added a comment -

        It wouldn't you'd need to use a separate cache for database derived templates. Either in DataResourceWorker or by refactoring FreeMarkerWorker to a bit more delegator aware.

        Show
        lektran Scott Gray added a comment - It wouldn't you'd need to use a separate cache for database derived templates. Either in DataResourceWorker or by refactoring FreeMarkerWorker to a bit more delegator aware.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        I made a similar change in rev 1636158. There are a lot of artifacts whose behavior changes based on the delegator, so I am not sure how we can make them all "delegator-aware."

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - I made a similar change in rev 1636158. There are a lot of artifacts whose behavior changes based on the delegator, so I am not sure how we can make them all "delegator-aware."
        Hide
        lektran Scott Gray added a comment -

        Ideally the delegator would provide normal caches as well as GenericValue specific caches. We'd just stop using UtilCache directly for creating database derived object caches.

        Show
        lektran Scott Gray added a comment - Ideally the delegator would provide normal caches as well as GenericValue specific caches. We'd just stop using UtilCache directly for creating database derived object caches.
        Hide
        lektran Scott Gray added a comment -

        Just to be clear(er), I'm basically suggesting the cache would come through the delegator's Cache class which would transparently prepend the delegator name to cache name, rather than having calling code everywhere appending them to the cache keys. With a standard naming convention it might also help us hide caches they shouldn't be able to see from tenants with access to webtools.

        Show
        lektran Scott Gray added a comment - Just to be clear(er), I'm basically suggesting the cache would come through the delegator's Cache class which would transparently prepend the delegator name to cache name, rather than having calling code everywhere appending them to the cache keys. With a standard naming convention it might also help us hide caches they shouldn't be able to see from tenants with access to webtools.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        That sounds like an interesting idea and it could work. The problem will be solving the build dependencies.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - That sounds like an interesting idea and it could work. The problem will be solving the build dependencies.
        Hide
        toashishvijay Ashish Vijaywargiya added a comment -

        Thanks Deepak for the contribution, Changes are committed in trunk at r1661358 and in R14.12 at r1661359.

        Show
        toashishvijay Ashish Vijaywargiya added a comment - Thanks Deepak for the contribution, Changes are committed in trunk at r1661358 and in R14.12 at r1661359.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Were only trunk and R14.12 concerned?

        Show
        jacques.le.roux Jacques Le Roux added a comment - Were only trunk and R14.12 concerned?
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Thanks Jacques.

        This should also go in R13.07 branch as well.

        Show
        deepak.dixit Deepak Dixit added a comment - Thanks Jacques. This should also go in R13.07 branch as well.
        Hide
        toashishvijay Ashish Vijaywargiya added a comment -

        Hand merged changes in R13.07 at r1661611. Thanks Jacques, Deepak!

        Show
        toashishvijay Ashish Vijaywargiya added a comment - Hand merged changes in R13.07 at r1661611. Thanks Jacques, Deepak!
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Thanks Ashish.

        Show
        deepak.dixit Deepak Dixit added a comment - Thanks Ashish.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Deepak,

        Backported in
        R13.07 r1661611
        R12.04 r1661616

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Deepak, Backported in R13.07 r1661611 R12.04 r1661616
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Ashish, we crossed on wire, no worries

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Ashish, we crossed on wire, no worries

          People

          • Assignee:
            toashishvijay Ashish Vijaywargiya
            Reporter:
            deepak.dixit Deepak Dixit
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile