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

Replace bshInterpreter with groovyShell

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: framework
    • Labels:
      None

      Description

      To support the migration to gradle, removing bsh.jar dependency from the project, and improve readability and fonctionnalities following Deepak Dixit idea expressed here https://issues.apache.org/jira/browse/OFBIZ-7576?focusedCommentId=15347972

      1. beanshell-removal-part1.patch
        45 kB
        Jacopo Cappellato
      2. OFBIZ-7763.patch
        40 kB
        Gil Portenseigne

        Activity

        Hide
        gil portenseigne Gil Portenseigne added a comment -

        This patch is that I started to do a month ago about the subject, the new issue implying these changes is that variable in use-when needs "context." prefix to be interprated by groovyShell.

        Show
        gil portenseigne Gil Portenseigne added a comment - This patch is that I started to do a month ago about the subject, the new issue implying these changes is that variable in use-when needs "context." prefix to be interprated by groovyShell.
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        Deepak Dixit keep in mind thats an unfinished work, do not hesitate to comment/improve it

        Show
        gil portenseigne Gil Portenseigne added a comment - Deepak Dixit keep in mind thats an unfinished work, do not hesitate to comment/improve it
        Hide
        jacopoc Jacopo Cappellato added a comment -

        One subtask that could be useful in preparation for the removal would be that of converting the calls to ${bsh: to groovy calls.
        There are just few left in the accounting component.
        Once they are gone, we can start the removal of some code.

        Show
        jacopoc Jacopo Cappellato added a comment - One subtask that could be useful in preparation for the removal would be that of converting the calls to ${bsh: to groovy calls. There are just few left in the accounting component. Once they are gone, we can start the removal of some code.
        Hide
        deepak.dixit Deepak Dixit added a comment - - edited

        Here is the ticket for this work OFBIZ-7764

        Show
        deepak.dixit Deepak Dixit added a comment - - edited Here is the ticket for this work OFBIZ-7764
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Similarly, another subtasks very useful in preparation for the removal is the conversion af all the minilang calls to <call-bsh>
        into calls to <script> using the groovy syntax.

        Show
        jacopoc Jacopo Cappellato added a comment - Similarly, another subtasks very useful in preparation for the removal is the conversion af all the minilang calls to <call-bsh> into calls to <script> using the groovy syntax.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Once all the calls to ${bsh: are converted to calls to ${groovy: and once all the minilang calls to <call-bsh> are converted into <script> calls using the groovy syntax we can apply the patch beanshell-removal-part1.patch
        that will remove a series of classes and artifacts dependent on beanshell.
        This work is not intended to replace the patch provided by Gil, it is just covering other areas of the framework.

        Show
        jacopoc Jacopo Cappellato added a comment - Once all the calls to ${bsh: are converted to calls to ${groovy: and once all the minilang calls to <call-bsh> are converted into <script> calls using the groovy syntax we can apply the patch beanshell-removal-part1.patch that will remove a series of classes and artifacts dependent on beanshell. This work is not intended to replace the patch provided by Gil, it is just covering other areas of the framework.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        There are 143 occurrences of <call-bsh> elements... we may need a call to action for the community to convert all of them.

        Show
        jacopoc Jacopo Cappellato added a comment - There are 143 occurrences of <call-bsh> elements... we may need a call to action for the community to convert all of them.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        In revision 1751947 I have committed my patch named patch beanshell-removal-part1.patch

        To summarize, the remaining tasks are now:

        1) replace bshInterpreter with groovyShell
        2) remove the jars: bsh-engine-modified.jar and bsh-2.0b4.jar

        Show
        jacopoc Jacopo Cappellato added a comment - In revision 1751947 I have committed my patch named patch beanshell-removal-part1.patch To summarize, the remaining tasks are now: 1) replace bshInterpreter with groovyShell 2) remove the jars: bsh-engine-modified.jar and bsh-2.0b4.jar
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        This patch, fix all use-when using groovyShell instead of bsh, compiling with all bsh lib removed.

        I was forced to analyse expression to identify the groovy variable, to set non existent one in context to null in binding, avoiding Exception...

        I will reread it this week-end, it needs more tests...

        Any comments and tests are welcome.

        Show
        gil portenseigne Gil Portenseigne added a comment - This patch, fix all use-when using groovyShell instead of bsh, compiling with all bsh lib removed. I was forced to analyse expression to identify the groovy variable, to set non existent one in context to null in binding, avoiding Exception... I will reread it this week-end, it needs more tests... Any comments and tests are welcome.
        Hide
        rishisolankii Rishi Solanki added a comment - - edited

        Reported issue with probable solution on sub-ticket: OFBIZ-7765

        Show
        rishisolankii Rishi Solanki added a comment - - edited Reported issue with probable solution on sub-ticket: OFBIZ-7765
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        Last rev of my patch, i will commit it soon

        Show
        gil portenseigne Gil Portenseigne added a comment - Last rev of my patch, i will commit it soon
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        Commited in trunk r1752231

        Thanks Jacopo for your patch, Deepak and Rishi for the subtasks and Nicolas for the review.

        Show
        gil portenseigne Gil Portenseigne added a comment - Commited in trunk r1752231 Thanks Jacopo for your patch, Deepak and Rishi for the subtasks and Nicolas for the review.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I read in the commit comment

        Today only alphabetic variable with '_' are detected (i.e. my_variable==null).

        and in commit this snippet

        +            if (UtilValidate.isNotEmpty(expression)) {
        +                //analyse expression to find variables by split non alpha, ignoring "_" to allow my_variable usage
        +                String [] variables = expression.split("[\\P{Alpha}&&[^_]]+");
        +                for (String variable: variables)
        +                    if(!vars.containsKey(variable)) vars.put(variable, null);
        +            }
        

        Should we not make this clearer somewhere (not sure how, just asking at this point)?

        Show
        jacques.le.roux Jacques Le Roux added a comment - I read in the commit comment Today only alphabetic variable with '_' are detected (i.e. my_variable==null). and in commit this snippet + if (UtilValidate.isNotEmpty(expression)) { + //analyse expression to find variables by split non alpha, ignoring "_" to allow my_variable usage + String [] variables = expression.split( "[\\P{Alpha}&&[^_]]+" ); + for ( String variable: variables) + if (!vars.containsKey(variable)) vars.put(variable, null ); + } Should we not make this clearer somewhere (not sure how, just asking at this point)?
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        Yes I tried to be clear enough in code to ease improvement, this trick is there to avoid going through the 1600+ use-when in the codebase, adding "context." to each variable.

        But i do not know where to write it down other than in the code itself. (thanks for the review by the way)

        Show
        gil portenseigne Gil Portenseigne added a comment - Yes I tried to be clear enough in code to ease improvement, this trick is there to avoid going through the 1600+ use-when in the codebase, adding "context." to each variable. But i do not know where to write it down other than in the code itself. (thanks for the review by the way)
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I finally don't think we need to say more about that to users. Variables names should simply follow Java syntax (I just learnt that $ is an acceptable character in a Java variable name but is not recommended, at least to start with http://www.oracle.com/technetwork/java/codeconventions-135099.html#367).

        To follow Java syntax, where variables names can contain but not start by numbers, I'd simply replace Alpha by Alnum in the regexp. Of course, variables names starting by a number and isolated numbers should be removed from the variables names list. I see no other issues introduced by replacing Alpha by Alnum, but better to think twice

        Show
        jacques.le.roux Jacques Le Roux added a comment - I finally don't think we need to say more about that to users. Variables names should simply follow Java syntax (I just learnt that $ is an acceptable character in a Java variable name but is not recommended, at least to start with http://www.oracle.com/technetwork/java/codeconventions-135099.html#367 ). To follow Java syntax, where variables names can contain but not start by numbers, I'd simply replace Alpha by Alnum in the regexp. Of course, variables names starting by a number and isolated numbers should be removed from the variables names list. I see no other issues introduced by replacing Alpha by Alnum, but better to think twice
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        Yes Jacques, i did hesitated to implement it like that, i will try and test this upgrade next week, if no-one has already done it since.

        Show
        gil portenseigne Gil Portenseigne added a comment - Yes Jacques, i did hesitated to implement it like that, i will try and test this upgrade next week, if no-one has already done it since.

          People

          • Assignee:
            gil portenseigne Gil Portenseigne
            Reporter:
            gil portenseigne Gil Portenseigne
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development