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-9841] Start process with correlation key rest api should return… #2564

Merged
merged 1 commit into from Aug 24, 2021
Merged

[JBPM-9841] Start process with correlation key rest api should return… #2564

merged 1 commit into from Aug 24, 2021

Conversation

abhijithumbe
Copy link
Contributor

… 400 http status if correlation key already exist.

Thank you for submitting this pull request

JIRA: (https://issues.redhat.com/browse/JBPM-9841)

@kie-ci
Copy link

kie-ci commented Aug 4, 2021

Can one of the admins verify this patch?

5 similar comments
@kie-ci
Copy link

kie-ci commented Aug 4, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 4, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 4, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 4, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 4, 2021

Can one of the admins verify this patch?

@abhijithumbe abhijithumbe mentioned this pull request Aug 4, 2021
Closed
@elguardian
Copy link
Member

@abhijithumbe overall looks ok, do you mind to take care of @sutaakar comments ?
#2564 (comment)
#2564 (comment)

@abhijithumbe
Copy link
Contributor Author

@abhijithumbe overall looks ok, do you mind to take care of @sutaakar comments ?
#2564 (comment)
#2564 (comment)

@elguardian Yes, I have done the changes as per the comment. Facing some issues with integration tests, working on it.

sutaakar
sutaakar approved these changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@sutaakar sutaakar left a comment

Choose a reason for hiding this comment

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

Looks good to me (minor suggestions above)
Jenkins retest this please

@abhijithumbe abhijithumbe force-pushed the RHPAM-3641 branch 2 times, most recently from d18f736 to 1fe0df3 Compare August 11, 2021 15:13
@elguardian
Copy link
Member

Jenkins retest this please

elguardian
elguardian approved these changes Aug 16, 2021
View reviewed changes
Copy link
Member

@elguardian elguardian left a comment

Choose a reason for hiding this comment

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

LGTM

@elguardian
Copy link
Member

@abhijithumbe do you mind to check the test failure ? might be related

@elguardian
Copy link
Member

Jenkins retest this please.

@mareknovotny
Copy link
Member

Jenkins retest this please

@sonarcloud
Copy link

sonarcloud bot commented Aug 19, 2021

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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mareknovotny
Copy link
Member

jenkins retest this

@abhijithumbe
Copy link
Contributor Author

abhijithumbe commented Aug 19, 2021 •
edited

@elguardian @mareknovotny QueryDataServiceIntegrationTest.testErrorHandlingFailedToSignal test is failing with NPE.

[2021-08-19T08:02:54.386Z] java.lang.NullPointerException: null [2021-08-19T08:02:54.386Z] at org.kie.server.integrationtests.shared.basetests.RestJmsSharedBaseIntegrationTest.lambda$filterErrorsByProcessInstanceId$1(RestJmsSharedBaseIntegrationTest.java:172) [2021-08-19T08:02:54.386Z] at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174) [2021-08-19T08:02:54.386Z] at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948) [2021-08-19T08:02:54.386Z] at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482) [2021-08-19T08:02:54.386Z] at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472) [2021-08-19T08:02:54.386Z] at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708) [2021-08-19T08:02:54.386Z] at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) [2021-08-19T08:02:54.386Z] at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:566) [2021-08-19T08:02:54.386Z] at org.kie.server.integrationtests.shared.basetests.RestJmsSharedBaseIntegrationTest.filterErrorsByProcessInstanceId(RestJmsSharedBaseIntegrationTest.java:172) [2021-08-19T08:02:54.386Z] at org.kie.server.integrationtests.jbpm.QueryDataServiceIntegrationTest.testErrorHandlingFailedToSignal(QueryDataServiceIntegrationTest.java:778) [2021-08-19T08:02:54.386Z] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

I dont see code from this PR is used anywhere in execution of this test. Seems failure is unrelated. Also I am able to execute same test without any issue on my system
Tests run: 162, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 194.194 s - in org.kie.server.integrationtests.jbpm.QueryDataServiceIntegrationTest

@elguardian
Copy link
Member

Hi @abhijithumbe I don't think this test is failing nightly (@mareknovotny ?) therefore if it is consistent it might be related to the PR.
Do you mind to check ?

@elguardian
Copy link
Member

@abhijithumbe do you mind to change in RestJmsSharedBaseIntegrationTest

protected List<ExecutionErrorInstance> filterErrorsByProcessId(Collection<ExecutionErrorInstance> errors, String processId) {
    return errors.stream().filter(error -> processId.equals(error.getProcessId())).collect(Collectors.toList());
}

protected List<ExecutionErrorInstance> filterErrorsByProcessInstanceId(Collection<ExecutionErrorInstance> errors, Long processInstanceId) {
    return errors.stream().filter(error -> processInstanceId.equals(error.getProcessInstanceId())).collect(Collectors.toList());
}

@abhijithumbe
Copy link
Contributor Author

@elguardian done mentioned changes.

@mareknovotny
Copy link
Member

jenkins retest this please

gmunozfe
gmunozfe reviewed Aug 20, 2021
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

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

Swagger ApiResponse should be added to both modified APIs including the code 409 (conflict) for completeness.

@ApiResponse(code = 409, message = "duplicated correlation key")

Also a minor comment to avoid a null check in comparison.

Test QueryDataServiceIntegrationTest.testErrorHandlingFailedToSignal is not failing

...t/kie-server-rest-common/src/main/java/org/kie/server/remote/rest/common/util/RestUtils.java Outdated Show resolved Hide resolved
@abhijithumbe abhijithumbe force-pushed the RHPAM-3641 branch 2 times, most recently from d30e5f1 to 22a36c6 Compare August 22, 2021 13:26
@abhijithumbe
[JBPM-9841] Start process with correlation key rest api should return…
f61fe37 
… 400 http status if correlation key already exist.
@mareknovotny
Copy link
Member

jenkins retest this please

gmunozfe
gmunozfe approved these changes Aug 23, 2021
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment

Choose a reason for hiding this comment

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

Looks good to me, fabulous work @abhijithumbe !

@lazarotti
Copy link
Member

Jenkins retest this please

@elguardian
Copy link
Member

not test failure related to the PR. it is marked as failure due to some other reasons.

@elguardian elguardian merged commit 3236a9e into kiegroup:main Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmunozfe gmunozfe gmunozfe approved these changes

@sutaakar sutaakar sutaakar approved these changes

@elguardian elguardian elguardian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@abhijithumbe @kie-ci @elguardian @mareknovotny @lazarotti @gmunozfe @sutaakar