OFBiz
  1. OFBiz
  2. OFBIZ-528

Upgrade Beanshell library to version 2.0b4

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Component/s: framework
    • Labels:
      None
    • Environment:

      Windows XP, MySQL 5.0, JDK1.5, ofbiz_opentaps_473442

      Description

      As discussed recently on ofbiz-users mailing list, I proposed to upgrade the Beanshell jar used by OFBiz (framework/base/lib/scripting/bsh.jar), which is based on a 1.3a1, to the current "stable" version, 2.0b4.

      This has the following advantages:

      • JDK1.5 interpretation capability
      • various bugfixes

      However as explained by David E. Jones here:
      https://cwiki.apache.org/confluence/display/OFBTECH/OFBiz+Customized+BeanShell+Source+(with+ParsedScript+object)

      ...it will be necessary to patch the source to allow for the script parsing on which OFBiz depends.

      This fix MAY also resolve this bug:
      https://issues.apache.org/jira/browse/OFBIZ-303

      1. viewprofile.bsh
        5 kB
        Cameron Smith
      2. ParsedScriptObjectExtension.patch
        9 kB
        David E. Jones
      3. Interpreter.java
        40 kB
        Cameron Smith
      4. BshClassManager.java
        20 kB
        Cameron Smith

        Activity

        Hide
        Jacques Le Roux added a comment -

        Updated the link to <<OFBiz Customized BeanShell Source (with ParsedScript object)>> in wiki

        Show
        Jacques Le Roux added a comment - Updated the link to <<OFBiz Customized BeanShell Source (with ParsedScript object)>> in wiki
        Show
        Cameron Smith added a comment - Suggestion to ZK project: http://sourceforge.net/tracker/index.php?func=detail&aid=1715690&group_id=152762&atid=785194
        Hide
        Cameron Smith added a comment -

        Thanks. I will incorporate this in my company's branch of OFBiz the next time we merge (may not be for a while!).

        Meanwhile, I am preparing a set of performance improvements to the ZK rich client library (potix.com). It depends heavily on beanshell, and the most obvious speed-up would be for it to use the OFBiz parsed script caching extension. This may increase the pressure, ahem, on Pat to reconsider including this extension in the original beanshell.

        Show
        Cameron Smith added a comment - Thanks. I will incorporate this in my company's branch of OFBiz the next time we merge (may not be for a while!). Meanwhile, I am preparing a set of performance improvements to the ZK rich client library (potix.com). It depends heavily on beanshell, and the most obvious speed-up would be for it to use the OFBiz parsed script caching extension. This may increase the pressure, ahem, on Pat to reconsider including this extension in the original beanshell.
        Hide
        David E. Jones added a comment -

        Okay, it's in!

        Show
        David E. Jones added a comment - Okay, it's in!
        Hide
        David E. Jones added a comment -

        This patch is not meant for inclusion in OFBiz.

        It is a modification to a SPL/GPL licensed file, used in OFBiz under the SPL. As such these changes are SPL licensed and meant to be included in beanshell itself, not as part of OFBiz.

        Show
        David E. Jones added a comment - This patch is not meant for inclusion in OFBiz. It is a modification to a SPL/GPL licensed file, used in OFBiz under the SPL. As such these changes are SPL licensed and meant to be included in beanshell itself, not as part of OFBiz.
        Hide
        David E. Jones added a comment -

        The source code for 2.0b4 used for this is revision 21 from the SVN repository at:

        http://ikayzo.org/svn/beanshell

        I'll attach the patch used for these changes to this issue.

        Thanks for your work on this Cameron. I spent a bit of time minimizing the patch, ie changing as little as possible, and changing the formatting to be more consistent with what bsh uses. There are 2 motivations for this:

        1. make it easier to update in the future
        2. increase our chances of getting this accepted into beanshell itself (though not really likely, it could happen!)

        Show
        David E. Jones added a comment - The source code for 2.0b4 used for this is revision 21 from the SVN repository at: http://ikayzo.org/svn/beanshell I'll attach the patch used for these changes to this issue. Thanks for your work on this Cameron. I spent a bit of time minimizing the patch, ie changing as little as possible, and changing the formatting to be more consistent with what bsh uses. There are 2 motivations for this: 1. make it easier to update in the future 2. increase our chances of getting this accepted into beanshell itself (though not really likely, it could happen!)
        Hide
        Jacques Le Roux added a comment -

        I agree with you Jacopo : +1 for being more aggressive on jar updates (even if it seems something very minor to me for now)

        Show
        Jacques Le Roux added a comment - I agree with you Jacopo : +1 for being more aggressive on jar updates (even if it seems something very minor to me for now)
        Hide
        Jacopo Cappellato added a comment -

        I've not review this contribution, but if the issues will be limited to fixing for npe on bsh scripts, then I'd say that committing it right after the tag/branch has been done would be the right timing for this.
        In general, I'd like to be a bit more aggressive (right after the branch) in updating the included jar files (adding the rev number suffix and updating to the last stable versions) and removing the unused ones.

        Show
        Jacopo Cappellato added a comment - I've not review this contribution, but if the issues will be limited to fixing for npe on bsh scripts, then I'd say that committing it right after the tag/branch has been done would be the right timing for this. In general, I'd like to be a bit more aggressive (right after the branch) in updating the included jar files (adding the rev number suffix and updating to the last stable versions) and removing the unused ones.
        Hide
        Cameron Smith added a comment -

        Hi. The only specific issue I found, was that certain existing .bsh scripts, perhaps 1 in 10, broke with null pointers when I upgraded. In these scripts, it was always possible to fix the problem very easily by putting in null checks.

        In other words, I believe that that bsh 2.0 simply defaults variables to null more often.

        Since we do not use the OFBiz frontend (except for webtools, which works fine), but use the ZK rich client framework, I have not looked into this more.

        What I can say, is that the ZK framework itself, and any user written scripts, depend utterly on beanshell, and we have not found a single interpreter bug ever, using the jar file attached to this JIRA. That is, if we write correct Java in our scripts, they work perfectly. Apart from myself, we have 4 developers and 2 testers coding and/or using ZK scripts on a regular basis, so I reckon if there were any nasties, we would have found them by now.

        We have two products currently using ZK and both are in the final stages of pre-production testing/beta testing, and I would be very surprised at this stage if we discover any beanshell-related issues.

        Therefore, my only preoccupation with upgrading OFBiz to the attached jar would be that, either as part of the same commit, or very soon afterwards, there would have to be a systematic "sweep" of all OFBiz screens which use .bsh scripts, to tidy up their null handling. That is, lots of little no-brainer fixes.

        Show
        Cameron Smith added a comment - Hi. The only specific issue I found, was that certain existing .bsh scripts, perhaps 1 in 10, broke with null pointers when I upgraded. In these scripts, it was always possible to fix the problem very easily by putting in null checks. In other words, I believe that that bsh 2.0 simply defaults variables to null more often. Since we do not use the OFBiz frontend (except for webtools, which works fine), but use the ZK rich client framework, I have not looked into this more. What I can say, is that the ZK framework itself, and any user written scripts, depend utterly on beanshell, and we have not found a single interpreter bug ever, using the jar file attached to this JIRA. That is, if we write correct Java in our scripts, they work perfectly. Apart from myself, we have 4 developers and 2 testers coding and/or using ZK scripts on a regular basis, so I reckon if there were any nasties, we would have found them by now. We have two products currently using ZK and both are in the final stages of pre-production testing/beta testing, and I would be very surprised at this stage if we discover any beanshell-related issues. Therefore, my only preoccupation with upgrading OFBiz to the attached jar would be that, either as part of the same commit, or very soon afterwards, there would have to be a systematic "sweep" of all OFBiz screens which use .bsh scripts, to tidy up their null handling. That is, lots of little no-brainer fixes.
        Hide
        David E. Jones added a comment -

        Cameron: I hope you're still around!

        Now that we are actually getting a release branch going, soon after the branch I would like to get BeanShell updated.

        You mentioned that you didn't feel your efforts were ready to go back into OFBiz. Do you have any other thoughts about what seemed to be an issue, or anything that has happened since then?

        Show
        David E. Jones added a comment - Cameron: I hope you're still around! Now that we are actually getting a release branch going, soon after the branch I would like to get BeanShell updated. You mentioned that you didn't feel your efforts were ready to go back into OFBiz. Do you have any other thoughts about what seemed to be an issue, or anything that has happened since then?
        Hide
        Cameron Smith added a comment -

        David, please read my previous comments carefully - I specifically did not submit anything as a patch because I wanted your and others' feedback, and also because I wanted to do more testing beforehand.

        There is no point me submitting a patched viewprofile.sh if there are 10 other scripts which will break in the same way because of null-handling, but I did not come across them! If there are web-based regression tests for OFBiz I will happily run them, but I believe we do not yet have these. Which is why I reckon it would be better, for such a fundamental bit of the system, for someone else to read over the code changes.

        I am happy to submit the Interpreter.java and BshClassManager.java changes as patches, but against what? Since these files are not in the OFBiz source tree, and much of the rest of the code has changed between 1.3.x and 2.x, I believe I should submit them as patches versus Beanshell source, and not against your original 1.3.x patched versions. What do you think?

        Finally, my understanding is that the OFBiz weekly builds which I am using come straight from the trunk, at a certain known SVN version. Since it is impractical for me to check out the source directly from trunk (due to bandwidth issues here in Moz), what I have done in previous patches I have submitted, is downloaded the immediately previous weekly build (which still includes .svn subdirs), "linked it" to the SVN trunk via Subclipse, and then manually updated the files in the specific area I am changing to the trunk version if necessary. Finally, I make the patch against these trunk versions.

        I await your feedback on these points before proceeding.

        Show
        Cameron Smith added a comment - David, please read my previous comments carefully - I specifically did not submit anything as a patch because I wanted your and others' feedback, and also because I wanted to do more testing beforehand. There is no point me submitting a patched viewprofile.sh if there are 10 other scripts which will break in the same way because of null-handling, but I did not come across them! If there are web-based regression tests for OFBiz I will happily run them, but I believe we do not yet have these. Which is why I reckon it would be better, for such a fundamental bit of the system, for someone else to read over the code changes. I am happy to submit the Interpreter.java and BshClassManager.java changes as patches, but against what? Since these files are not in the OFBiz source tree, and much of the rest of the code has changed between 1.3.x and 2.x, I believe I should submit them as patches versus Beanshell source, and not against your original 1.3.x patched versions. What do you think? Finally, my understanding is that the OFBiz weekly builds which I am using come straight from the trunk, at a certain known SVN version. Since it is impractical for me to check out the source directly from trunk (due to bandwidth issues here in Moz), what I have done in previous patches I have submitted, is downloaded the immediately previous weekly build (which still includes .svn subdirs), "linked it" to the SVN trunk via Subclipse, and then manually updated the files in the specific area I am changing to the trunk version if necessary. Finally, I make the patch against these trunk versions. I await your feedback on these points before proceeding.
        Hide
        David E. Jones added a comment -

        Could you submit the viewprofile.bsh changes as a patch file instead of the complete file?

        Also for the Interpreter.java and BshClassManager.java files could you submit those as patches as well?

        Patches can be created with something like "svn diff > changes.patch" or "diff -u file1 file2 > changes.patch"

        Show
        David E. Jones added a comment - Could you submit the viewprofile.bsh changes as a patch file instead of the complete file? Also for the Interpreter.java and BshClassManager.java files could you submit those as patches as well? Patches can be created with something like "svn diff > changes.patch" or "diff -u file1 file2 > changes.patch"
        Hide
        David E. Jones added a comment -

        I haven't reviewed the rest of this, but in general submissions that are intended for the OFBiz trunk should be tested against the OFBiz trunk, and not against an opentaps or any other OFBiz distribution that may have changes that would affect the issue.

        Show
        David E. Jones added a comment - I haven't reviewed the rest of this, but in general submissions that are intended for the OFBiz trunk should be tested against the OFBiz trunk, and not against an opentaps or any other OFBiz distribution that may have changes that would affect the issue.
        Hide
        Cameron Smith added a comment -

        The attached files are for validation and testing by the community. They were compiled against 1.5. If I don't hear anything to the contrary in the next few days, I will get the upcoming weekly build on friday, and recompile and test everything with both JDK1.4 and JDK1.5 before submitting final versions of everything.

        I would like people's opinions on the following points especially...
        a. I suggested a clearer name for the new jar, in keeping with the current effort to include versions in jar names, which I wholeheartedly support
        b. BshClassManager.createClassManager() is now just a convenience method, because of changes within Beanshell since 1.3.x. I believe we should in fact ditch this method for two reasons...
        i. It is one less difference between our source and orig. Beanshell source
        ii. Within OFBiz source (BshUtil.getMasterInterpreter), it is clearer what we are doing if we explicitly call createClassManager(null)
        c. There appears to be somewhat different handling of nulls in this version of Beanshell. I have not investigated fully yet, but I had to tweak at least one script for compatibility. There may be others. For example look at attached viewprofile.bsh, line 89. In "pure java", I should definitely not have had to concatenate an empty String before passing in the argument to Boolean.valueOf().

        Anyway, the attached code has been submitted to the following tests:
        1. ofbiz_opentaps_473442 builds cleanly against this jar
        2. All OFBiz screens THAT I HAD NOTICED which previously were breaking with bsh, are now working. In one case, (see notes above) I had to slightly tweak the file.
        3. ZK 2.1.2, which did not work with original bsh.jar, works perfectly
        4. The beanshell 2.0b4 "test suite", runs with exactly the same passes and fails that it shows when run against the original 2.0b4.

        Show
        Cameron Smith added a comment - The attached files are for validation and testing by the community. They were compiled against 1.5. If I don't hear anything to the contrary in the next few days, I will get the upcoming weekly build on friday, and recompile and test everything with both JDK1.4 and JDK1.5 before submitting final versions of everything. I would like people's opinions on the following points especially... a. I suggested a clearer name for the new jar, in keeping with the current effort to include versions in jar names, which I wholeheartedly support b. BshClassManager.createClassManager() is now just a convenience method, because of changes within Beanshell since 1.3.x. I believe we should in fact ditch this method for two reasons... i. It is one less difference between our source and orig. Beanshell source ii. Within OFBiz source (BshUtil.getMasterInterpreter), it is clearer what we are doing if we explicitly call createClassManager(null) c. There appears to be somewhat different handling of nulls in this version of Beanshell. I have not investigated fully yet, but I had to tweak at least one script for compatibility. There may be others. For example look at attached viewprofile.bsh, line 89. In "pure java", I should definitely not have had to concatenate an empty String before passing in the argument to Boolean.valueOf(). Anyway, the attached code has been submitted to the following tests: 1. ofbiz_opentaps_473442 builds cleanly against this jar 2. All OFBiz screens THAT I HAD NOTICED which previously were breaking with bsh, are now working. In one case, (see notes above) I had to slightly tweak the file. 3. ZK 2.1.2, which did not work with original bsh.jar, works perfectly 4. The beanshell 2.0b4 "test suite", runs with exactly the same passes and fails that it shows when run against the original 2.0b4.
        Hide
        Cameron Smith added a comment -

        Example of necessary alteration to existing beanshell script, for null handling

        Show
        Cameron Smith added a comment - Example of necessary alteration to existing beanshell script, for null handling
        Hide
        Cameron Smith added a comment -

        Beanshell 2.0b4, patched to allow script parsing, based on David Jones' original changes from: http://docs.ofbiz.org/pages/viewpageattachments.action?pageId=741

        Show
        Cameron Smith added a comment - Beanshell 2.0b4, patched to allow script parsing, based on David Jones' original changes from: http://docs.ofbiz.org/pages/viewpageattachments.action?pageId=741

          People

          • Assignee:
            David E. Jones
            Reporter:
            Cameron Smith
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development