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

RHPAM-2182 - Multiple Instance data input and output lack of Data Typ… #3130

Merged
merged 3 commits into from Feb 5, 2020

Conversation

hasys
Copy link
Member

@hasys hasys commented Jan 30, 2020

…e definition

@hasys
Copy link
Member Author

hasys commented Jan 30, 2020

Jenkins execute full downstream build

@hasys
Copy link
Member Author

hasys commented Jan 30, 2020

War file for tests can be found here: https://drive.google.com/open?id=1lCkuMAjUTR4yqD5CMPnWxJPuXz1P8woK

@hasys
Copy link
Member Author

hasys commented Jan 30, 2020

Hi @LuboTerifaj, there are some information to simplify runtime testing:

I didn’t found how to see the difference between specific type and Object on KieServer, so I tested only in Business Central.

Possible usecases (I tested it on Firefox)

  • Default type is Object, if you reset type to empty type and save/reopen type again will be restored to Object.
  • It should be possible to set Custom datatype
  • All default types should be preloaded (Integer, Boolean, Float, Object, String), can be selected and should preserve save/reload
  • All existing in project Data Types should be present in the combobox, can be selected and preserve save/reload
  • If you set Custom datatype to some not existing in the Project type (for an instance java.util.List) this value will be added to
  • DataType should be stored in XML as it is described in Jira issue.
  • In combobox should be shown simplified name version, in XML file should be stored full version. For example for full FQN name java.util.List simplified name will be List [java.util]

Important

Name field just smoke tested (it remains the same as before the change).

Update

Business Central project to use to test MI Input/Output variables at runtime: https://github.com/bpmn-tutorials/MITest

@hasys
Copy link
Member Author

hasys commented Jan 30, 2020

Jenkins execute full downstream build

Copy link
Contributor

@romartin romartin left a comment

Choose a reason for hiding this comment

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

Hey @hasys

Did a code review, just added a few comments, great job, thx!! 👍

Will also test the UI and let you know

@mbiarnes
Copy link
Contributor

Jenkins execute full downstream build

@hasys
Copy link
Member Author

hasys commented Jan 31, 2020

Hi @LuboTerifaj, I applied findings from @romartin and added some coverage. I rebuilded it locally and here you can download latest version: https://drive.google.com/open?id=1n-J_d7jrNzP7ogk4D2NYQRg5hBHT-9JP

Sonar cloud failing due to duplication in marshallers (backend and client side), so I suppose we can ignore it until we will solve duplication issue in it's root (not part of this PR).

Thank you!

@hasys
Copy link
Member Author

hasys commented Jan 31, 2020

Jenkins execute full downstream build

@sonarcloud
Copy link

sonarcloud bot commented Jan 31, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 6 Code Smells

70.6% 70.6% Coverage
13.9% 13.9% Duplication

@mbiarnes
Copy link
Contributor

Jenkins execute full downstream build

@hasys
Copy link
Member Author

hasys commented Jan 31, 2020

@LuboTerifaj, @romartin, hurray, we have green Full Downstream build, thanks @mbiarnes!

I did a quick test against Acceptance Criteria with Full Downstream build in Firefox and didn't found any issues.

@LuboTerifaj the SonarCloud is red due to duplications in marshallers only, but it is inevitable now since we have two copies of Marshallers. So, it is all ready for the review by all canons :) , thank you!

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hello @hasys ,

I've found one issue:

  • If you select any type of variable in editor except String, it is always String in runtime.
    • It may be caused by missing possibility to define custom collections such as List, etc. in process variable definition section. MI Collections properties list the defined collections.
      The issue is already reported as JBPM-7882).
    • In the ideal case, the variable type should be taken from collection, but the behaviour is currently postponed to next releases and it is tracked by BAPL-1519.
    • However, if the variable type is always String in the runtime, I don't see any advantages to have possibility to change it in editor.

Can you please have a look at this issue?
I have uploaded multiple-instance-process.bpmn to RHPAM-2182 where you can check the behaviour when you just slightly modify the process.

Thank you!

Copy link
Contributor

@romartin romartin left a comment

Choose a reason for hiding this comment

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

Hey @hasys , thanks for the changes, code looks good to me! So letting @LuboTerifaj continue with the runtime testing. Thanks guys!

@hasys
Copy link
Member Author

hasys commented Feb 4, 2020

Hi @LuboTerifaj,

I prepared Business Central project for tests: https://github.com/bpmn-tutorials/MITest

This example will send Person object to sub-process and gets Person back. Try to change data type of the in/out variables and you will get an error.

For example I changed Person type inside of the sub-process to Boolean and got this result at runtime:

	com/myspace/mitest/Process_com$u46$myspace$u46$mitest$u46$mPersons1766447153.java (10:517) : The method getName() is undefined for the type Boolean
	com/myspace/mitest/Process_com$u46$myspace$u46$mitest$u46$mPersons1766447153.java (14:627) : The method setBirthday(LocalDate) is undefined for the type Boolean

Let me know if you have any other questions. Thank you!

Copy link
Contributor

@LuboTerifaj LuboTerifaj left a comment

Choose a reason for hiding this comment

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

Hi @hasys , @romartin ,

since the changes don't affect behaviour in the engine and the pull-request just changes the type of variables in bpmn files, I agree with proceeding to merging.

The Mutliple instance feature needs to be redesigned anyway and the issue is already reported: BAPL-1519.

@LuboTerifaj
Copy link
Contributor

Hi @hasys , @romartin ,

the issue mentioned in my comment above is reported here:

  • JBPM-9017 MI Data Input and Output variable types are not handled correctly

@romartin romartin merged commit 9d52649 into kiegroup:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants