Skip to content
This repository has been archived by the owner on Feb 13, 2021. It is now read-only.

Implementation of ACTIVITY_SIGNALED event #247

Merged

Conversation

CTI777
Copy link
Contributor

@CTI777 CTI777 commented Feb 15, 2019


assertThat(streamHandler.getReceivedHeaders()).containsKeys(ALL_REQUIRED_HEADERS);

assertThat(receivedEvents)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add another assert to check that processDefinitionId and processInstanceId are set properly?

@CTI777
Copy link
Contributor Author

CTI777 commented Feb 24, 2019

@salaboy @erdemedeiros @igdianov
It seems that we have a problem with MessageProducerCommandContextCloseListener.
In the test when two processes are listening for one signal a processInstanceId in the events related to second process is overwritten with processInstanceId from the first process..
This bug should be fixed, maybe inside this PR? Details should be considered (discussed) additionally, because the header for the messages' bunch should be modified... And this may lead to failing of some other tests...

@salaboy
Copy link
Contributor

salaboy commented Feb 25, 2019

@CTI777 hmm that doesn't sounds like the right behaviour.. we will need to investigate.. Please sync with @erdemedeiros about this.

@igdianov
Copy link
Contributor

@salaboy the close listener injects the execution attributes into events before sending it to the stream under the assumption that there is always single process instance emitting events inside each transaction. This is apparently not the case, so it should be the responsibility of the event aggregator to resolve and apply correct values to each event when it is added to the list. Once the event is added to the list, it would be good practice to make it immutable. However, this is currently not possible because the setters are public. We could possibly wrap it into delegate to seal the event once we add it to the list.

I will help @CTI777 to fix this issue.

@erdemedeiros
Copy link
Collaborator

@igdianov on the same subject: I noticed that currently ProcessDeployedEvents are not setting headers at all (as they are not using closeListener). I created a specific MessageBuilderChainFactory to set only routing key and runtime bundle info headers in this PR . However it's not implementing the interface MessageBuilderChainFactory as I wasn't sure which information I could use add more information there (if there is any). This new MessageBuilderChainFactory is invoked in the CloudProcessDeployedProducer.

@igdianov
Copy link
Contributor

igdianov commented Mar 7, 2019

@igdianov on the same subject: I noticed that currently ProcessDeployedEvents are not setting headers at all (as they are not using closeListener). I created a specific MessageBuilderChainFactory to set only routing key and runtime bundle info headers in this PR . However it's not implementing the interface MessageBuilderChainFactory as I wasn't sure which information I could use add more information there (if there is any). This new MessageBuilderChainFactory is invoked in the CloudProcessDeployedProducer.

@erdemedeiros I will take a look at it.

@igdianov
Copy link
Contributor

@CTI777 @erdemedeiros This PR needs to be rebased on top of develop branch. We also need to add some tests to make sure that execution context is injected in signal audit events.

@erdemedeiros erdemedeiros force-pushed the CTI777-2272-Missing-audit-event-apis-for-signals branch from 8ae0d64 to bc7bfa7 Compare March 26, 2019 13:18
CTI777 and others added 6 commits March 26, 2019 14:57
Also moved test related to signals outside of AuditProducerIT as it is becoming to big, and causing unnecessary conflicts. In the future we may want to split it again in several different classes.
@erdemedeiros erdemedeiros force-pushed the CTI777-2272-Missing-audit-event-apis-for-signals branch from bc7bfa7 to 440e56a Compare March 26, 2019 14:59
@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #247 into develop will decrease coverage by 0.35%.
The diff coverage is 35.71%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #247      +/-   ##
=============================================
- Coverage      73.17%   72.81%   -0.36%     
  Complexity         5        5              
=============================================
  Files            114      115       +1     
  Lines           1476     1490      +14     
  Branches          47       48       +1     
=============================================
+ Hits            1080     1085       +5     
- Misses           357      365       +8     
- Partials          39       40       +1
Impacted Files Coverage Δ Complexity Δ
...vents/listeners/ProcessEngineEventsAggregator.java 46.51% <0%> (-2.27%) 0 <0> (ø)
...converter/ToCloudProcessRuntimeEventConverter.java 15.38% <0%> (-1.64%) 0 <0> (ø)
...ts/configuration/CloudEventsAutoConfiguration.java 100% <100%> (ø) 0 <0> (ø) ⬇️
.../events/listeners/CloudSignalReceivedProducer.java 66.66% <66.66%> (ø) 0 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2896b91...f955582. Read the comment docs.

@igdianov
Copy link
Contributor

igdianov commented Mar 28, 2019

@erdemedeiros I have fixed missing execution context attributes for CloudBPMNSignalEvent. See test.

There is a small nuisance with start event catch signal. The event is dispatched outside of transaction, so it's entity does not have associated process instance id to retrieve the details of current execution context.

@erdemedeiros erdemedeiros merged commit c971ba3 into develop Apr 2, 2019
@erdemedeiros erdemedeiros deleted the CTI777-2272-Missing-audit-event-apis-for-signals branch April 2, 2019 14:49
salaboy pushed a commit that referenced this pull request Apr 16, 2019
* Add error-handling dependency to starters

* Create exception handler specific to runtime bundle

* Clean exception handlers scattered in controllers

* Implementation of signalReceived event (#247)

Related to Activiti/Activiti#2272
Depends on 
AlfrescoArchive/activiti-api#84
AlfrescoArchive/activiti-cloud-api#65
Activiti/Activiti#2511

* Fix audit producer test (#284)

- Take in account new events thrown for signal received event that was not thrown before
- Move test to SignalAuditProducerIT

Related to Activiti/Activiti#2272

*  Move CloudActivityBehaviorFactory to runtime bundle (#281)

* Move CloudActivityBehaviorFactory to runtime bundle

Initially CloudActivityBehaviorFactory was declared in set in Activiti core, but this was breaking intermediate throw signals when non-cloud starter was used.

Related to Activiti/Activiti#2663

* Move CloudActivityBehaviorFactory to runtime bundle

Initially CloudActivityBehaviorFactory was declared in set in Activiti core, but this was breaking intermediate throw signals when non-cloud starter was used.

Related to Activiti/Activiti#2663

* fix(version): update org.activiti.dependencies:activiti-dependencies to 7.1.11

* fix(version): update org.activiti.dependencies:activiti-dependencies to 7.1.12

* fix(version): update org.activiti.dependencies:activiti-dependencies to 7.1.13

* Change HttpStatus for int in RestControllerAdvice

* fix(version): update org.activiti.dependencies:activiti-dependencies to 7.1.14

* fix(version): update org.activiti.cloud.common:activiti-cloud-service-common-dependencies to 7.1.24

* fix(version): update org.activiti.cloud.common:activiti-cloud-service-common-dependencies to 7.1.25

* fix(version): update org.activiti.cloud.build:activiti-cloud-parent to 7.1.7

* fix(version): update org.activiti.cloud.common:activiti-cloud-service-common-dependencies to 7.1.26

* Refactor ActivitiErrorMessage class

* Refactor with MediaType constants

* Refactor error handlers module

* Add swagger generator test (#287)

* Add swagger generator test

* Produce swagger file as a Maven artifact

- add plugin configuration to produce swagger file as a Maven artifact
- produce both Alfresco and Hal format

* Add yaml convertion from json

* Add TODO to swagger yaml generation

* Add generated yaml as an artifact

* Add Gson prettifying features

* Move Swagger generation inside a special type of test

- Add a new configuration for failsafe plugin to hanle *ITSupport.java which will generate the swagger file
- Create a profile generate-swagger enabled by default (can be skipped with `-DskipSwaggerGen` option)

* Add comments

* Add yaml generation back

* Format swagger-hal.json at creation

* Change scope for gson dependency to test

* Add test scope to jackson-dataformat-yaml

* A test to check that processDefinitionVersion set on TaskStarted event (#285)

* A test to check that processDefinitionVersion set on TaskStarted event

* Test modified to check only event's processDefinitionVersion

* Remove icon from connecto definitions (#243)

* Add common handlers module to rest-impl

* Remove handler from ConnectorDefinitionControllerImpl

* Clear exception handler

* Change return type for exception handler

* Optimize imports

* Clear exception handler from ProcessInstanceControllerImpl

* Fix test response type

* Optimize imports

* Change return type of error handler

* Process instance description field removal (#262)

* Process instance description field removal

* Process instance description field removal

* fix(version): update org.activiti.dependencies:activiti-dependencies to 7.1.15

* fix(version): update org.activiti.cloud.common:activiti-cloud-service-common-dependencies to 7.1.27

* fix(version): update org.activiti.cloud.common:activiti-cloud-service-common-dependencies to 7.1.28

* fix(version): update org.activiti.cloud.common:activiti-cloud-service-common-dependencies to 7.1.29

* Update pom.xml

* Update pom

* Add swagger artifacts to BOM (#292)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants