Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Added a function to improve jscript prefix identifier #305

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

abpai94
Copy link
Collaborator

@abpai94 abpai94 commented Sep 4, 2024

When using the following prefix to scripts such as rjs or others:

<dataset>
    <name>member</name>
    <policy>FORCE</policy>
    <forceValues>
        <string>
              <![CDATA[rjs:
              ...jscript_function...
              ]]>
        </string>
    </forceValues>
</dataset>

The following error is thrown:

java.lang.NullPointerException: Cannot invoke "org.lsc.utils.ScriptableEvaluator.evalToObjectList(org.lsc.Task, String, java.util.Map)" because "se" is null

This occurs due to the prefix containing the following:

\n                            
            rjs

The .contains method in Maps object is unable to match this pattern.

This functionality works well with the following script:

<mainIdentifier>rjs:...jscript_function...</mainIdentifier>

As the script is done in-line the .contains method is able to match and find the appropriate jscript evaluator.

I have added a new function that uses pattern matching with the help of a small regex function which should resolve the above described issue.

I have tested this in scenarios where no script engine has been defined without posing any issue.
Also I have built on top of the work in #280 to seamlessly merge this minor fix.

@abpai94
Copy link
Collaborator Author

abpai94 commented Sep 4, 2024

@philhaworteks I have built this minor fix on top of your work in #280. We can merge when your changes are approved.

@abpai94 abpai94 self-assigned this Sep 4, 2024
@artlog
Copy link
Contributor

artlog commented Sep 4, 2024

Even my intial request is not working with java 8 due to java.util.Optional.or that was provided only from java 9.
java.lang.NoSuchMethodError: java.util.Optional.or(Ljava/util/function/Supplier;)Ljava/util/Optional;

@abpai94
Copy link
Collaborator Author

abpai94 commented Sep 5, 2024

I used java 9 to get the Optional code to work. I changed the code to the following:

			defaultImplementation = Optional.ofNullable(Optional.ofNullable(instancesTypeCache.get("js"))
                    .orElse(instancesTypeCache.get("rjs")));

It doesn't look pretty but it works.
It ensures that an optional is provided with rjs script evaluator.

@rouazana
Copy link
Contributor

I'm not sure this is worth it as this solution works fine:

<string><![CDATA[rjs:
              ...jscript_function...
              ]]></string>

@abpai94
Copy link
Collaborator Author

abpai94 commented Sep 16, 2024

Its a minor change that added additional tolerance to detect inline javascript. Seems much cleaner to use regex to determine the JS engine from the prefix.

I made additional changes in the most recent commit that adds further tolerance to detect a variety of cases.
Expected use case:

        <mainIdentifier><![CDATA[js:
           ...jscript...
          ]]></mainIdentifier>

Other use case 1:

        <mainIdentifier><![CDATA[
           js:
           ...jscript...
          ]]></mainIdentifier>

Other use case 2:

        <mainIdentifier>
          <![CDATA[js:
           ...jscript...
          ]]></mainIdentifier>

Other use case 3:

        <mainIdentifier>
          <![CDATA[
          js:
           ...jscript...
          ]]></mainIdentifier>

Other use case 4:

        <mainIdentifier><![CDATA[
           
           js:
           ...jscript...
          ]]></mainIdentifier>

Or any other variants that might be occur.

@abpai94
Copy link
Collaborator Author

abpai94 commented Sep 16, 2024

@rouazana If this seems unnecessary feel free to close off the issue. Found the lack of JS engine detection a minor inconvenience personally.

Copy link
Contributor

@rouazana rouazana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against it as it can improve this piece of code, but I guess there are currently some issues.

src/main/java/org/lsc/utils/ScriptingEvaluator.java Outdated Show resolved Hide resolved
src/main/java/org/lsc/utils/ScriptingEvaluator.java Outdated Show resolved Hide resolved
@abpai94
Copy link
Collaborator Author

abpai94 commented Sep 18, 2024

@rouazana I have implemented some tests and ameliorated the pattern compilation to reduce the overhead.

Copy link
Contributor

@rouazana rouazana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for taking my comments into account!

I have only 2 remaining suggestions:

  1. Try to name your tests with a meaningful function name, for example scriptShouldEvaluateWhenStartingWithANewLine for testJsPrefix1
  2. I'm not sure you are really testing what I mean, for example that the right JS engine is chosen if the js: or rjs: strings appear in different places in the expression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants