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.

Sign up for GitHub

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

Jump to bottom

JBPM-10079 ClassCastException when submitting a form in kie-server containing date process variable #2781

Merged
merged 1 commit into from
May 28, 2022
Merged

JBPM-10079 ClassCastException when submitting a form in kie-server containing date process variable #2781

merged 1 commit into from
May 28, 2022

Conversation

bxf12315
Copy link
Member

@bxf12315 bxf12315 commented May 17, 2022 •
edited
Loading

Thank you for submitting this pull request

JIRA: (please edit the JIRA link if it exists)

https://issues.redhat.com/browse/JBPM-10079

referenced Pull Requests: (please edit the URLs of referenced pullrequests if they exist)

  • paste the link(s) from GitHub here
  • link 2
  • link 3 etc.
How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.

build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

How to retest this PR or trigger a specific build:

  • a pull request please add comment: Jenkins retest this

  • a full downstream build please add comment: Jenkins run fdb

  • a compile downstream build please add comment: Jenkins run cdb

  • a full production downstream build please add comment: Jenkins execute product fdb

  • an upstream build please add comment: Jenkins run upstream

@bxf12315 bxf12315 requested a review from pefernan May 17, 2022 13:53
@bxf12315 bxf12315 force-pushed the JBPM-10077-update branch 2 times, most recently from eb6c75b to 2a211c6 Compare May 18, 2022 01:35
@bxf12315
Copy link
Member Author

Jenkins run fdb

1 similar comment
@bxf12315
Copy link
Member Author

Jenkins run fdb

@bxf12315 bxf12315 changed the title fix ClassCastException for RHPAM-4189 [draft]fix ClassCastException for RHPAM-4189 May 19, 2022
@bxf12315
Copy link
Member Author

Jenkins run fdb

@bxf12315 bxf12315 changed the title [draft]fix ClassCastException for RHPAM-4189 [draft]JBPM-10079 ClassCastException when submitting a form in kie-server containing date process variable May 19, 2022
@bxf12315
Copy link
Member Author

Jenkins run fdb

@bxf12315
Copy link
Member Author

Jenkins run fdb

@bxf12315
Copy link
Member Author

Jenkins run fdb

@bxf12315 bxf12315 force-pushed the JBPM-10077-update branch 2 times, most recently from 52a0d00 to 8c89a1e Compare May 22, 2022 11:51
@bxf12315
Copy link
Member Author

Jenkins run fdb

jstastny-cz
jstastny-cz requested changes May 23, 2022
View reviewed changes
Copy link

@jstastny-cz jstastny-cz left a comment

Choose a reason for hiding this comment

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

Please organize the code better for the PR review.

...-parent/kie-server-api/src/main/java/org/kie/server/api/marshalling/json/JSONMarshaller.java Outdated
// add byte array handling support to allow byte[] to be send as payload
classes.add(JaxbByteArray.class);
classes.add(java.time.LocalDateTime.class);
Copy link

Choose a reason for hiding this comment

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

Is this safe with regards to the if clause below if (!formatDate) { ? I would suspect that date types would have same handling in this respect (just guessing though).

Copy link
Member Author

Choose a reason for hiding this comment

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

the formatDate from issue https://issues.redhat.com/browse/JBPM-7424.I will change it.

...r-parent/kie-server-api/src/test/java/org/kie/server/api/marshalling/JSONMarshallerTest.java Outdated
this.sqlDate = sqlDate;
}

public Timestamp getTimestamp() {
Copy link

Choose a reason for hiding this comment

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

this one is imported as package, yet in other occurrences its FQN is used, java.sql.Timestamp, may be import by accident?

...-jbpm-ui/src/main/java/org/kie/server/services/jbpm/ui/form/render/AbstractFormRenderer.java Outdated
.append("' : ")
.append(appendExtractionExpression(type, field.getBinding(), field.getId(), jsType, field.isShowTime(), field.getType()))
.append(",");
}else {
Copy link

Choose a reason for hiding this comment

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

whitespace formatting

...-jbpm-ui/src/main/java/org/kie/server/services/jbpm/ui/form/render/AbstractFormRenderer.java Outdated
.append(appendExtractionExpression(type, field.getBinding(), field.getId(), jsType, field.isShowTime()))
.append(wrapEndFieldType(type))
.append(",");
if ("datetime-local".equals(type) || "Date".equals(type)
Copy link

Choose a reason for hiding this comment

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

Would it be possible to turn the condition into a boolean method instead? Some distinctive naming that will allow using the method also in appendExtractionExpression as the logic seems to be the same.

Copy link

Choose a reason for hiding this comment

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

Ad my comment about changing case elsewhere - is this capital letter in Date the actual reason?

...-jbpm-ui/src/main/java/org/kie/server/services/jbpm/ui/form/render/AbstractFormRenderer.java Outdated
jsonTemplate.append("getDateFormated('")
.append(id)
.append("')");
} else if (type.equals("Date")) {
Copy link

Choose a reason for hiding this comment

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

are you sure about this change of case?

Copy link
Member Author

@bxf12315 bxf12315 May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just want to re-use

droolsjbpm-integration/kie-server-parent/kie-server-services/kie-server-services-jbpm-ui/src/main/java/org/kie/server/services/jbpm/ui/form/render/AbstractFormRenderer.java

Line 111 in 8c89a1e

this.inputTypes.put("DatePicker", "Date");

...-jbpm-ui/src/main/java/org/kie/server/services/jbpm/ui/form/render/AbstractFormRenderer.java Outdated
}
} else if (type.equals("datetime")) {
Copy link

Choose a reason for hiding this comment

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

here we could re-use the boolean method proposed above. It would merge ~4 else-if clauses into a single one.

@bxf12315 bxf12315 force-pushed the JBPM-10077-update branch 2 times, most recently from 4acbc7b to 9a64a75 Compare May 23, 2022 10:31
@bxf12315
Copy link
Member Author

Jenkins retest this

pefernan
pefernan requested changes May 24, 2022
View reviewed changes
Copy link
Member

@pefernan pefernan left a comment

Choose a reason for hiding this comment

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

@bxf12315 good work! Overall I'm okay with the change, there are some fixes to do:

  • remove the unused import in the JsonMarshallerTest (it breaks the build)
  • add the java.sql date types (for consistency)
  • found an issue in task forms with java.time.LocalDate types, the date is added in the input value but it's not displayed in the field. That's because the value contines the time.

...r-parent/kie-server-api/src/test/java/org/kie/server/api/marshalling/JSONMarshallerTest.java Outdated
@@ -18,6 +18,7 @@

import java.io.File;
import java.net.URL;
import java.sql.Timestamp;
Copy link
Member

Choose a reason for hiding this comment

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

not used import

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

...-jbpm-ui/src/main/java/org/kie/server/services/jbpm/ui/form/render/AbstractFormRenderer.java
@@ -509,6 +534,7 @@ private String getFieldType(FormField field) {
case "java.time.LocalDateTime":
return "datetime-local";
case "java.util.Date":
return "Date";
case "java.sql.Date":
case "java.sql.Timestamp":
Copy link
Member

Choose a reason for hiding this comment

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

looks like renderer supports java.sql Date types but they are not present in the functionMappings or in the JsonMarshaller. I'd add them for consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

our data modeler and form modeler do not support java.sql.*, so I don't add it.

Copy link
Member

Choose a reason for hiding this comment

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

@bxf12315 +1

...s/kie-server-services-jbpm-ui/src/main/resources/form-templates-providers/js/kieserver-ui.js
@@ -556,11 +556,36 @@ function closeCreationForm(fieldId) {
currentRow = null;
}

function getLocalDateWithoutTime(elementId) {
function getFormattedLocalDateTime(jsondate) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd preffer a single function that recives the datatype as a param but I'm ok with this

@sonarcloud
Copy link

sonarcloud bot commented May 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

90.8% 90.8% Coverage
0.0% 0.0% Duplication

jstastny-cz
jstastny-cz approved these changes May 25, 2022
View reviewed changes
Copy link

@jstastny-cz jstastny-cz left a comment

Choose a reason for hiding this comment

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

Tested with process with variable directly using (not through dataobject) varying datetime related types:

  • java.util.Date
  • java.time.LocalTime
  • java.time.LocalDate
  • java.time.LocalDateTime
  • java.time.OffsetDateTime

I have limited insight into code, so I leave that to other reviewers to assess.

@jstastny-cz
Copy link

For some reason the reviewers section does not reflect my review verdict.

pefernan
pefernan approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@pefernan pefernan left a comment

Choose a reason for hiding this comment

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

+1

@bxf12315 bxf12315 changed the title [draft]JBPM-10079 ClassCastException when submitting a form in kie-server containing date process variable JBPM-10079 ClassCastException when submitting a form in kie-server containing date process variable May 25, 2022
@mareknovotny mareknovotny merged commit df0122b into kiegroup:main May 28, 2022
bxf12315 added a commit to Cory-jbpm/droolsjbpm-integration that referenced this pull request Jun 14, 2022
@bxf12315
JBPM-10079 ClassCastException when submitting a form in kie-server co…
bc87e8d 
…ntaining date process variable (kiegroup#2781)
bxf12315 added a commit to Cory-jbpm/droolsjbpm-integration that referenced this pull request Jul 22, 2022
@bxf12315
JBPM-10079 ClassCastException when submitting a form in kie-server co…
c5ce95e 
…ntaining date process variable (kiegroup#2781)
nmirasch pushed a commit that referenced this pull request Jul 27, 2022
@bxf12315 @nmirasch
JBPM-10079 ClassCastException when submitting a form in kie-server co…
95a51ae 
…ntaining date process variable (#2781)
nmirasch pushed a commit that referenced this pull request Jul 27, 2022
@bxf12315 @nmirasch
JBPM-10079 ClassCastException when submitting a form in kie-server co…
e925f19 
…ntaining date process variable (#2781)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pefernan pefernan pefernan approved these changes

@jstastny-cz jstastny-cz jstastny-cz approved these changes

@ChughShilpa ChughShilpa Awaiting requested review from ChughShilpa

@nmirasch nmirasch Awaiting requested review from nmirasch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bxf12315 @jstastny-cz @pefernan @nmirasch @mareknovotny