Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 16.11.01
    • Component/s: framework
    • Labels:
      None
    • Sprint:
      Community Day 2 - 2015, Community Day 3 - 2015, Re-Factor Sprint 1

      Description

      It appears that the storeAll() variants are being refactor; basically, the useCache parameter is being removed. However, this has caused the
      entity engine to have deprecation with itself.

      1. OFBIZ-6276-EntityStoreOptions-SVN.patch
        63 kB
        Gil Portenseigne
      2. OFBIZ-6276-EntityStoreOptions.patch
        65 kB
        Martin Becker
      3. OFBIZ-6276-variant-2.patch
        4 kB
        Martin Becker
      4. OFBIZ-6276-variant-1.patch
        6 kB
        Martin Becker

        Issue Links

          Activity

          Hide
          doogie Adam Heath added a comment -

          [javac17] /srv/ofbiz/apache-git/framework/entity/src/org/ofbiz/entity/GenericDelegator.java:1415: warning: [deprecation] storeAll(List<GenericValue>,boolean,boolean) in Delegator has been deprecated
          [javac17] public int storeAll(List<GenericValue> values, boolean doCacheClear, boolean createDummyFks) throws GenericEntityException {
          [javac17] ^
          [javac17] /srv/ofbiz/apache-git/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java:329: warning: [deprecation] storeAll(List<GenericValue>,boolean,boolean) in Delegator has been deprecated
          [javac17] delegator.storeAll(valuesToWrite, doCacheClear, createDummyFks);
          [javac17] ^
          [javac17] /srv/ofbiz/apache-git/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java:498: warning: [deprecation] removeAll(List<? extends GenericEntity>,boolean) in Delegator has been deprecated
          [javac17] delegator.removeAll(valuesToDelete, doCacheClear);
          [javac17] ^

          Show
          doogie Adam Heath added a comment - [javac17] /srv/ofbiz/apache-git/framework/entity/src/org/ofbiz/entity/GenericDelegator.java:1415: warning: [deprecation] storeAll(List<GenericValue>,boolean,boolean) in Delegator has been deprecated [javac17] public int storeAll(List<GenericValue> values, boolean doCacheClear, boolean createDummyFks) throws GenericEntityException { [javac17] ^ [javac17] /srv/ofbiz/apache-git/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java:329: warning: [deprecation] storeAll(List<GenericValue>,boolean,boolean) in Delegator has been deprecated [javac17] delegator.storeAll(valuesToWrite, doCacheClear, createDummyFks); [javac17] ^ [javac17] /srv/ofbiz/apache-git/framework/entity/src/org/ofbiz/entity/util/EntitySaxReader.java:498: warning: [deprecation] removeAll(List<? extends GenericEntity>,boolean) in Delegator has been deprecated [javac17] delegator.removeAll(valuesToDelete, doCacheClear); [javac17] ^
          Hide
          doogie Adam Heath added a comment -

          When I first saw this deprecation warning, I thought it would be something simple. Even when I filed this issue, I held back on deep thought. However, the refactor that is being attempted around storeAll is very back for backwards compatibility. A parameter is being removed, that has critical meaning. I don't really think this is something that can safely be implemented. A new method name would need to be created to do that refactor.

          Show
          doogie Adam Heath added a comment - When I first saw this deprecation warning, I thought it would be something simple. Even when I filed this issue, I held back on deep thought. However, the refactor that is being attempted around storeAll is very back for backwards compatibility. A parameter is being removed, that has critical meaning. I don't really think this is something that can safely be implemented. A new method name would need to be created to do that refactor.
          Hide
          mbecker Martin Becker added a comment -

          It has to be ensured, that with the future removal of the doCacheClear parameter from the method signature:

          from
          storeAll(List<GenericValue> values, boolean doCacheClear, boolean createDummyFks)
          to
          storeAll(List<GenericValue> values, boolean createDummyFks)

          the signature afterwards does not match the old signature with different meaning:

          storeAll(List<GenericValue> values, boolean doCacheClear)

          where legacy code would get no compiler error and may miss the changed meaning.

          I implemented two variants of a solution for this:

          With the first I renamed the method with the concrete implementation from storeAll to storeAllInternal and changed the visibility to private, so there is no collision with the other signature with List<GenericValue>, boolean params. The createDummyFks function is made public available with a new method storeAllWithDummyFks.

          The second variant is simpler and without the need of changed method names, it just switches the order of the parameters in the signature, which is not so nice in reading the signature but avoids special method name for the createDummyFks feature.

          The next step for me would be to remove the deprecated code from the Delegator implementation in the context of OFBIZ-6273.

          Show
          mbecker Martin Becker added a comment - It has to be ensured, that with the future removal of the doCacheClear parameter from the method signature: from storeAll(List<GenericValue> values, boolean doCacheClear, boolean createDummyFks) to storeAll(List<GenericValue> values, boolean createDummyFks) the signature afterwards does not match the old signature with different meaning: storeAll(List<GenericValue> values, boolean doCacheClear) where legacy code would get no compiler error and may miss the changed meaning. I implemented two variants of a solution for this: With the first I renamed the method with the concrete implementation from storeAll to storeAllInternal and changed the visibility to private, so there is no collision with the other signature with List<GenericValue>, boolean params. The createDummyFks function is made public available with a new method storeAllWithDummyFks . The second variant is simpler and without the need of changed method names, it just switches the order of the parameters in the signature, which is not so nice in reading the signature but avoids special method name for the createDummyFks feature. The next step for me would be to remove the deprecated code from the Delegator implementation in the context of OFBIZ-6273 .
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Martin,

          I think we already used the same than your 2nd solution which I agree is simpler.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Martin, I think we already used the same than your 2nd solution which I agree is simpler.
          Hide
          mbecker Martin Becker added a comment -

          Hi Jacques,

          what do you mean with "we already used the same"?
          This issue is open and unassigned and I wanted to take a step forward to be able to work on OFBIZ-6273.

          Show
          mbecker Martin Becker added a comment - Hi Jacques, what do you mean with "we already used the same"? This issue is open and unassigned and I wanted to take a step forward to be able to work on OFBIZ-6273 .
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I mean while refactoring after a deprecation we already switched parameters in signature as a way to prevent collisions

          Show
          jacques.le.roux Jacques Le Roux added a comment - I mean while refactoring after a deprecation we already switched parameters in signature as a way to prevent collisions
          Hide
          mbecker Martin Becker added a comment -

          Aaah, check!
          Not really nice but effective

          Show
          mbecker Martin Becker added a comment - Aaah, check! Not really nice but effective
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Yes after reviewing both I'm still undecided. I think indeed the 1st way is cleaner. I will finally wait some other opinions before committing...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Yes after reviewing both I'm still undecided. I think indeed the 1st way is cleaner. I will finally wait some other opinions before committing...
          Hide
          mbecker Martin Becker added a comment -

          Mmmh, 1st way was my 1st idea, but I was not happy with introducing a new method name like "storeAllWithDummyFks" while the strategy is more like adding boolean parameters for such features (e.g. useCache parameter instead of methods like findByAnd and findByAndCache).
          This combined with the small relevance of the "createDummyFks" Feature (my opinion) led me to the simpler API without a different method name but switched parameter order for a method that may not be used very often.

          Show
          mbecker Martin Becker added a comment - Mmmh, 1st way was my 1st idea, but I was not happy with introducing a new method name like "storeAllWithDummyFks" while the strategy is more like adding boolean parameters for such features (e.g. useCache parameter instead of methods like findByAnd and findByAndCache). This combined with the small relevance of the "createDummyFks" Feature (my opinion) led me to the simpler API without a different method name but switched parameter order for a method that may not be used very often.
          Hide
          gareth.carter Gareth Carter added a comment -

          What about new method that allows passing in an options object

          storeAll(List<GenericValue> values, StoreOptions options) ?

          You can deprecate the old methods without changes

          Moving forward, you would just need to add new values to the StoreOptions class rather than adding new parameters to the existing one

          I am not sure about the name StoreOptions though, EntityOptions ?

          Show
          gareth.carter Gareth Carter added a comment - What about new method that allows passing in an options object storeAll(List<GenericValue> values, StoreOptions options) ? You can deprecate the old methods without changes Moving forward, you would just need to add new values to the StoreOptions class rather than adding new parameters to the existing one I am not sure about the name StoreOptions though, EntityOptions ?
          Hide
          mbecker Martin Becker added a comment -

          Good point, I would call it EntityStoreOptions according to the existing EntityFindOptions.

          Show
          mbecker Martin Becker added a comment - Good point, I would call it EntityStoreOptions according to the existing EntityFindOptions.
          Hide
          mbrohl Michael Brohl added a comment -

          +1

          Show
          mbrohl Michael Brohl added a comment - +1
          Hide
          mbecker Martin Becker added a comment -

          Added new patch which removes the use of deprecated API and the deprecated API itself from Delegator and GenericDelegator.

          Introduced the EntityStoreOptions class as mentioned in a comment before to substitute the createDummyFks parameter of storeAll method to avoid ambiguous API after removing the deprecated doCacheClear parameter.

          The decryptFieldValue method, just defined deprecated by Jacopo is left there as deprecated. Only the "old" deprecated methods where removed.

          Show
          mbecker Martin Becker added a comment - Added new patch which removes the use of deprecated API and the deprecated API itself from Delegator and GenericDelegator. Introduced the EntityStoreOptions class as mentioned in a comment before to substitute the createDummyFks parameter of storeAll method to avoid ambiguous API after removing the deprecated doCacheClear parameter. The decryptFieldValue method, just defined deprecated by Jacopo is left there as deprecated. Only the "old" deprecated methods where removed.
          Hide
          mbecker Martin Becker added a comment -

          I forgot to mention: The standard tests were all "green"...

          Show
          mbecker Martin Becker added a comment - I forgot to mention: The standard tests were all "green"...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Martin, I will take care of it this week...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Martin, I will take care of it this week...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          OK a month passed, I will get back to this soon...

          Show
          jacques.le.roux Jacques Le Roux added a comment - OK a month passed, I will get back to this soon...
          Hide
          mbecker Martin Becker added a comment -

          Hi Jacques,
          now there have been two more month passed

          Show
          mbecker Martin Becker added a comment - Hi Jacques, now there have been two more month passed
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Yes I will take care of that soon...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Yes I will take care of that soon...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Martin, could you please provide a pure svn patch? Thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Martin, could you please provide a pure svn patch? Thanks!
          Hide
          mbecker Martin Becker added a comment -

          Hi Jacques, did you have a problem to apply the OFBIZ-6276-EntityStoreOptions.patch?
          It is in universal diff format and I just tried to apply it within my eclipse and it worked fine.

          Show
          mbecker Martin Becker added a comment - Hi Jacques, did you have a problem to apply the OFBIZ-6276 -EntityStoreOptions.patch? It is in universal diff format and I just tried to apply it within my eclipse and it worked fine.
          Hide
          gil portenseigne Gil Portenseigne added a comment -

          Hi Martin, i just try to use your patch, it's working fine, I regenerate a pure SVN patch, from it (using linux svn shell command), there it is !

          Show
          gil portenseigne Gil Portenseigne added a comment - Hi Martin, i just try to use your patch, it's working fine, I regenerate a pure SVN patch, from it (using linux svn shell command), there it is !
          Hide
          mbecker Martin Becker added a comment -

          Thanks a lot!

          Show
          mbecker Martin Becker added a comment - Thanks a lot!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          It's maybe due to changes I have pending, I'll dlb-check, thanks to you both

          Show
          jacques.le.roux Jacques Le Roux added a comment - It's maybe due to changes I have pending, I'll dlb-check, thanks to you both
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks to all for your help (you are mentionned in the commit ) I had indeed conflicting pending changes in the working copy I initially tried

          The fix is committed in trunk at revision: 1704108.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks to all for your help (you are mentionned in the commit ) I had indeed conflicting pending changes in the working copy I initially tried The fix is committed in trunk at revision: 1704108.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I had/have to fix unnoticed issues in *.ftl and *.groovy files as Vyom mentionned in OFBIZ-6643. See r1704478 and OFBIZ-6649

          Show
          jacques.le.roux Jacques Le Roux added a comment - I had/have to fix unnoticed issues in *.ftl and *.groovy files as Vyom mentionned in OFBIZ-6643 . See r1704478 and OFBIZ-6649

            People

            • Assignee:
              jacques.le.roux Jacques Le Roux
              Reporter:
              doogie Adam Heath
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile