OFBiz
  1. OFBiz
  2. OFBIZ-3847

Entity ECAs not triggered correctly when using Delegator.storeAll() method

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Release Branch 10.04
    • Fix Version/s: Upcoming Branch
    • Component/s: framework
    • Labels:
      None
    • Sprint:
      Bug Crush Event - 21/2/2015

      Description

      The conditions don't work when updating (not creating) entities using the Delegator.storeAll() method. E.g. the following condition does not work:

      <eca entity="Product" operation="create-store" event="return">
              <condition field-name="autoCreateKeywords" operator="not-equals" value="N"/>
              <action service="indexProductKeywords" mode="sync" value-attr="productInstance"/>
      </eca>
      

      The indexProductKeywords service is called anyway when the product is updated and the autoCreateKeywords was "N" and stays "N". It works correctly for newly created products.

      The problem is in the method GenericDelegator.storeAll(), where unchanged field values are not passed down to the store() method. The store method calls the ECA engine, which does not receive the unchanged values at all and thus cannot evaluate the EECA conditions correctly.

        Activity

        Hide
        Jacques Le Roux added a comment - - edited

        Thanks for feedback Paul You might consider to backport in older releases

        Show
        Jacques Le Roux added a comment - - edited Thanks for feedback Paul You might consider to backport in older releases
        Hide
        Paul Foxworthy added a comment -

        Thanks Jacques.

        We have been using this for years, and no, we haven't had any issues.

        Regards

        Paul Foxworthy

        Show
        Paul Foxworthy added a comment - Thanks Jacques. We have been using this for years, and no, we haven't had any issues. Regards Paul Foxworthy
        Hide
        Jacques Le Roux added a comment -

        Fixed in
        trunk r1716319
        R14.12 r1716322

        Too much conflicts in older releases

        Show
        Jacques Le Roux added a comment - Fixed in trunk r1716319 R14.12 r1716322 Too much conflicts in older releases
        Hide
        Jacques Le Roux added a comment - - edited

        Hi Anne,

        I have updated your patch. Did you encounter any issues with it?

        Show
        Jacques Le Roux added a comment - - edited Hi Anne, I have updated your patch. Did you encounter any issues with it?
        Hide
        Anne Jessel added a comment -

        I've created a patch modeled after Scott's suggestion. I'd appreciate someone reviewing it. It does work for me, however I am concerned that it might fail when the value of a field that is part of the EECA condition is being changed from non-null to null using GenericDelegator.storeAll, because of the way storeAll works. There may be a simple solution I've missed.

        Show
        Anne Jessel added a comment - I've created a patch modeled after Scott's suggestion. I'd appreciate someone reviewing it. It does work for me, however I am concerned that it might fail when the value of a field that is part of the EECA condition is being changed from non-null to null using GenericDelegator.storeAll, because of the way storeAll works. There may be a simple solution I've missed.
        Hide
        Anne Jessel added a comment -

        This bug is one I'd like to fix. Scott's suggestion looks good to me. Anyone have any comments or better ideas before I go ahead and implement it?

        Show
        Anne Jessel added a comment - This bug is one I'd like to fix. Scott's suggestion looks good to me. Anyone have any comments or better ideas before I go ahead and implement it?
        Hide
        Jacques Le Roux added a comment -

        I did not look into code, but I can't find a better idea Scott

        Show
        Jacques Le Roux added a comment - I did not look into code, but I can't find a better idea Scott
        Hide
        Scott Gray added a comment -

        I think this points to a larger problem whereby there is no guarantee that any entity ECA will receive a complete GenericValue representing the entire record.

        For example GenericDelegator.removeByPrimaryKey() only passes the primary key fields to the ECA rule runner and any ECAs that depend on other non-pk fields won't be able to evaluate their condition properly.

        I'm not sure what the full solution would look like, it would be good to get some thoughts of others on this.

        We could consider having EntityEcaCondition.eval() perform a db lookup if any of the fields it needs to evaluate are missing. If they are missing it could fully populate the entity with the missing fields so that a lookup is only required at most once per delegator operation. If no ECA conditions require any missing fields then the lookup wouldn't be performed, so the performance penalty would only be incurred when necessary.

        Show
        Scott Gray added a comment - I think this points to a larger problem whereby there is no guarantee that any entity ECA will receive a complete GenericValue representing the entire record. For example GenericDelegator.removeByPrimaryKey() only passes the primary key fields to the ECA rule runner and any ECAs that depend on other non-pk fields won't be able to evaluate their condition properly. I'm not sure what the full solution would look like, it would be good to get some thoughts of others on this. We could consider having EntityEcaCondition.eval() perform a db lookup if any of the fields it needs to evaluate are missing. If they are missing it could fully populate the entity with the missing fields so that a lookup is only required at most once per delegator operation. If no ECA conditions require any missing fields then the lookup wouldn't be performed, so the performance penalty would only be incurred when necessary.
        Hide
        Martin Kreidenweis added a comment -

        This should fix the issue in GenericDelegator. We can't see why not all the fields should be passed down to store().

        Show
        Martin Kreidenweis added a comment - This should fix the issue in GenericDelegator. We can't see why not all the fields should be passed down to store().

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Martin Kreidenweis
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile