Details

    • Type: Improvement
    • Status: Patch Available
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: Trunk
    • Fix Version/s: None
    • Component/s: ALL COMPONENTS
    • Labels:
      None

      Description

      I found an inconsistency in the code for string comparison statusId.equals("PRUN_COMPLETED") whereas it should be written as "PRUN_COMPLETED".equals(statusId)
      cause the former can throw NullPointerException if the variable found to be NULL.

      This pattern should be applied to

      • Java Files
      • Groovy Files
      • FTL Files

      Here is the reference for the discussion done on the dev list.
      http://markmail.org/message/iqfaab3fl3ukxchy

      1. OFBIZ-9251_ftl_accounting.patch
        23 kB
        Devanshu Vyas
      2. OFBIZ-9251_ftl_framework.patch
        33 kB
        Devanshu Vyas
      3. OFBIZ-9251_ftl_manuf_workeffrt.patch
        7 kB
        Devanshu Vyas
      4. OFBIZ-9251_ftl_order.patch
        158 kB
        Devanshu Vyas
      5. OFBIZ-9251_ftl_party_content.patch
        25 kB
        Devanshu Vyas
      6. OFBIZ-9251_ftl_plugins_ebay_ebaystore.patch
        18 kB
        Devanshu Vyas
      7. OFBIZ-9251_ftl_plugins_ecommerce.patch
        106 kB
        Devanshu Vyas
      8. OFBIZ-9251_ftl_plugins_pricat_projmgr.patch
        9 kB
        Devanshu Vyas
      9. OFBIZ-9251_ftl_product.patch
        78 kB
        Devanshu Vyas
      10. OFBIZ-9251_ftl_themes.patch
        11 kB
        Devanshu Vyas
      11. OFBIZ-9254_FTL-I.patch
        16 kB
        Devanshu Vyas
      12. OFBIZ-9254_groovy_applications.patch
        15 kB
        Devanshu Vyas
      13. OFBIZ-9254_groovy_framework.patch
        10 kB
        Devanshu Vyas
      14. OFBIZ-9254_java_accounting.patch
        37 kB
        Devanshu Vyas
      15. OFBIZ-9254_java_base.patch
        11 kB
        Devanshu Vyas
      16. OFBIZ-9254_java_content.patch
        56 kB
        Devanshu Vyas
      17. OFBIZ-9254_java_entity.patch
        9 kB
        Devanshu Vyas
      18. OFBIZ-9254_java_order.patch
        44 kB
        Devanshu Vyas
      19. OFBIZ-9254_java_Party_WE_MFC.patch
        21 kB
        Devanshu Vyas
      20. OFBIZ-9254_java_product.patch
        43 kB
        Devanshu Vyas
      21. OFBIZ-9254_java_service.patch
        17 kB
        Devanshu Vyas
      22. OFBIZ-9254_java_test_entityext_common_cattalina_datafile.patch
        11 kB
        Devanshu Vyas
      23. OFBIZ-9254_java_webapp.patch
        8 kB
        Devanshu Vyas
      24. OFBIZ-9254_java_webtools.patch
        5 kB
        Devanshu Vyas
      25. OFBIZ-9254_java_widget.patch
        26 kB
        Devanshu Vyas
      26. OFBIZ-9254_plugins_groovy.patch
        19 kB
        Devanshu Vyas
      27. OFBIZ-9254_plugins_java.patch
        48 kB
        Devanshu Vyas

        Issue Links

          Activity

          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          This is a patch file for fixing the string comparisons(equals() and equalsIgnoreCase()) in groovy files.

          Show
          devanshu.vyas Devanshu Vyas added a comment - This is a patch file for fixing the string comparisons(equals() and equalsIgnoreCase()) in groovy files.
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          This is the patch file for fixing the string comparisons(equals() and equalsIgnoreCase()) in java files.

          Show
          devanshu.vyas Devanshu Vyas added a comment - This is the patch file for fixing the string comparisons(equals() and equalsIgnoreCase()) in java files.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Devanshu,

          I have reviewed the Groovy patch and it's OK with me.

          Reviewing the Java patch is something else (10+ times more). I was wondering: did you do your changes with the help of a regexp S/R or all by hand? Because I could use a regexp S/R to make the same changes and compares with yours.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Devanshu, I have reviewed the Groovy patch and it's OK with me. Reviewing the Java patch is something else (10+ times more). I was wondering: did you do your changes with the help of a regexp S/R or all by hand? Because I could use a regexp S/R to make the same changes and compares with yours.
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          Thanks Jacques for reviewing the patch.
          I have done all the changes by hand. I did try a couple of regular expressions but was not able to create the perfect one for this case.

          Show
          devanshu.vyas Devanshu Vyas added a comment - Thanks Jacques for reviewing the patch. I have done all the changes by hand. I did try a couple of regular expressions but was not able to create the perfect one for this case.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Devanshu,

          Perfect, I'll try to find a regexp and compare the changes. Else I'll simply review, quite hypnotic...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Devanshu, Perfect, I'll try to find a regexp and compare the changes. Else I'll simply review, quite hypnotic...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Devanshu,

          I agree it needs to be done by hand. I did not digg much but I clearly see that side effects are possible. So this will wait a bit on my side. I'm sorry about that because I know by experience than, starting in 1 to 3 months, your patch will begin to deprecate :/ Maybe someone courageous will help...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Devanshu, I agree it needs to be done by hand. I did not digg much but I clearly see that side effects are possible. So this will wait a bit on my side. I'm sorry about that because I know by experience than, starting in 1 to 3 months, your patch will begin to deprecate :/ Maybe someone courageous will help...
          Hide
          deepak.dixit Deepak Dixit added a comment -

          Its too large patch

          Devanshu,

          Could you please break them into component level?

          Show
          deepak.dixit Deepak Dixit added a comment - Its too large patch Devanshu, Could you please break them into component level?
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          Thanks Jacques, for your courageous efforts!! The patch file is really large...

          And Thanks Deepak, for the suggestion and appreciate your help here.
          I think I can break the patch at the component levels and then it will easy for you all to review.

          Show
          devanshu.vyas Devanshu Vyas added a comment - Thanks Jacques, for your courageous efforts!! The patch file is really large... And Thanks Deepak, for the suggestion and appreciate your help here. I think I can break the patch at the component levels and then it will easy for you all to review.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Devanshu,

          Yes best way to handle it indeed, notably because it allows several people to work on it.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Devanshu, Yes best way to handle it indeed, notably because it allows several people to work on it.
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          The patch file contains fixes for the Accounting component.

          Show
          devanshu.vyas Devanshu Vyas added a comment - The patch file contains fixes for the Accounting component.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Devanshu,

          I suggest we drop the big file, to avoid confusion I let you decide when...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Devanshu, I suggest we drop the big file, to avoid confusion I let you decide when...
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          Thanks Jacques!
          Removed the large patch files to avoid confusion.

          Show
          devanshu.vyas Devanshu Vyas added a comment - Thanks Jacques! Removed the large patch files to avoid confusion.
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          Removed incorrect file earlier uploaded, added correct patch file of plugins for groovy file fixes.

          Show
          devanshu.vyas Devanshu Vyas added a comment - Removed incorrect file earlier uploaded, added correct patch file of plugins for groovy file fixes.
          Hide
          deepak.dixit Deepak Dixit added a comment - - edited

          OFBIZ-9254_groovy_applications.patch has been committed at ofbiz-framework trunk at r#1795341
          OFBIZ-9254_groovy_framework.patch has been committed at ofbiz-framework trunk at r#1795342
          OFBIZ-9254_plugins_groovy.patch has been committed at ofbiz-framework trunk at r#1795344

          Show
          deepak.dixit Deepak Dixit added a comment - - edited OFBIZ-9254 _groovy_applications.patch has been committed at ofbiz-framework trunk at r#1795341 OFBIZ-9254 _groovy_framework.patch has been committed at ofbiz-framework trunk at r#1795342 OFBIZ-9254 _plugins_groovy.patch has been committed at ofbiz-framework trunk at r#1795344
          Hide
          devanshu.vyas Devanshu Vyas added a comment - - edited

          As per the discussion on the dev list, I have first prepared a patch(OFBIZ-9254_FTL-I.patch) which contains the fix for all the instances where .equals() method is used for comparisons in place of '==' in FTL files.

          Show
          devanshu.vyas Devanshu Vyas added a comment - - edited As per the discussion on the dev list, I have first prepared a patch( OFBIZ-9254 _FTL-I.patch) which contains the fix for all the instances where .equals() method is used for comparisons in place of '==' in FTL files.
          Hide
          devanshu.vyas Devanshu Vyas added a comment -

          Added patch files for fixing inconsistent string comparisons in FTL files.

          Show
          devanshu.vyas Devanshu Vyas added a comment - Added patch files for fixing inconsistent string comparisons in FTL files.
          Hide
          arunpati Arun Patidar added a comment -

          Thanks Devanshu. I am going to start reviewing and committing patches one by one.

          Show
          arunpati Arun Patidar added a comment - Thanks Devanshu. I am going to start reviewing and committing patches one by one.
          Hide
          arunpati Arun Patidar added a comment - - edited

          OFBIZ-9254_FTL-I.patch :- Committed revision 1801268
          OFBIZ-9254_ftl_themes.patch :- Committed revision 1801278
          OFBIZ-9254_ftl_product.patch: -Committed revision 1801280
          OFBIZ-9254_ftl_plugins_pricat_projmgr.patch: Committed revision 1801282
          OFBIZ-9254_ftl_plugins_ecommerce.patch:Committed revision 1801284

          Show
          arunpati Arun Patidar added a comment - - edited OFBIZ-9254 _FTL-I.patch :- Committed revision 1801268 OFBIZ-9254 _ftl_themes.patch :- Committed revision 1801278 OFBIZ-9254 _ftl_product.patch: -Committed revision 1801280 OFBIZ-9254 _ftl_plugins_pricat_projmgr.patch: Committed revision 1801282 OFBIZ-9254 _ftl_plugins_ecommerce.patch:Committed revision 1801284

            People

            • Assignee:
              deepak.dixit Deepak Dixit
              Reporter:
              devanshu.vyas Devanshu Vyas
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development