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

feat: support execution listeners #168

Merged
merged 4 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const camundaCloud10Rules = withConfig({
'loop-characteristics': 'error',
'message-reference': 'error',
'no-candidate-users': 'error',
'no-execution-listeners': 'error',
'no-expression': 'error',
'no-loop': 'error',
'no-multiple-none-start-events': 'error',
Expand Down Expand Up @@ -75,8 +76,11 @@ const camundaCloud85Rules = withConfig({
'wait-for-completion': 'error'
}, { version: '8.5' });

const camundaCloud86Rules = withConfig(
omit(camundaCloud85Rules, 'inclusive-gateway'), { version: '8.6' });
const camundaCloud86Rules = withConfig({
...omit(camundaCloud85Rules, [ 'inclusive-gateway', 'no-execution-listeners' ]),
'duplicate-execution-listeners': 'error',
'execution-listener': 'error'
}, { version: '8.6' });

const camundaPlatform719Rules = withConfig({
'history-time-to-live': 'info'
Expand Down Expand Up @@ -105,12 +109,14 @@ const rules = {
'called-element': './rules/camunda-cloud/called-element',
'collapsed-subprocess': './rules/camunda-cloud/collapsed-subprocess',
'connector-properties': './rules/camunda-cloud/connector-properties',
'duplicate-execution-listeners': './rules/camunda-cloud/duplicate-execution-listeners',
'duplicate-task-headers': './rules/camunda-cloud/duplicate-task-headers',
'error-reference': './rules/camunda-cloud/error-reference',
'escalation-boundary-event-attached-to-ref': './rules/camunda-cloud/escalation-boundary-event-attached-to-ref',
'escalation-reference': './rules/camunda-cloud/escalation-reference',
'event-based-gateway-target': './rules/camunda-cloud/event-based-gateway-target',
'executable-process': './rules/camunda-cloud/executable-process',
'execution-listener': './rules/camunda-cloud/execution-listener',
'feel': './rules/camunda-cloud/feel',
'history-time-to-live': './rules/camunda-platform/history-time-to-live',
'implementation': './rules/camunda-cloud/implementation',
Expand All @@ -119,6 +125,7 @@ const rules = {
'loop-characteristics': './rules/camunda-cloud/loop-characteristics',
'message-reference': './rules/camunda-cloud/message-reference',
'no-candidate-users': './rules/camunda-cloud/no-candidate-users',
'no-execution-listeners': './rules/camunda-cloud/no-execution-listeners',
'no-expression': './rules/camunda-cloud/no-expression',
'no-loop': './rules/camunda-cloud/no-loop',
'no-multiple-none-start-events': './rules/camunda-cloud/no-multiple-none-start-events',
Expand Down
17 changes: 9 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"modeler-moddle": "^0.2.0",
"sinon": "^17.0.1",
"sinon-chai": "^3.7.0",
"zeebe-bpmn-moddle": "^1.1.0"
"zeebe-bpmn-moddle": "^1.4.0"
},
"dependencies": {
"@bpmn-io/feel-lint": "^1.2.0",
Expand Down
33 changes: 33 additions & 0 deletions rules/camunda-cloud/duplicate-execution-listeners.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
const {
findExtensionElement,
hasDuplicatedPropertiesValues
} = require('../utils/element');

const { reportErrors } = require('../utils/reporter');

const { skipInNonExecutableProcess } = require('../utils/rule');

module.exports = skipInNonExecutableProcess(function() {
function check(node, reporter) {
const executionListeners = findExtensionElement(node, 'zeebe:ExecutionListeners');

if (!executionListeners) {
return;
}

const errors = hasDuplicatedExecutionListeners(executionListeners, node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it's because duplication requires two duplicate properties instead of just one. We could still adjust the helper to allow multiple properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have considered this and decided against it due to the adjustment needed. If we have more cases like this, we may abstract it away, but right now I'd consider it YAGNI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a huge fan of using ERROR_TYPES.PROPERTY_VALUE_DUPLICATED and then duplicatedProperty: 'type' since it doesn't mention the event type at all. We should probably have a separate type PROPERTY_VALUES_DUPLICATED and include all the data necessary to create a custom error message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd just add a hasDuplicatedPropertiesValues helper that encapsulates the logic so the rule simply calls hasDuplicatedPropertiesValues(taskHeaders, 'listeners', [ 'eventType', 'type' ], node). 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philippfromme please have a look now.


if (errors && errors.length) {
reportErrors(node, reporter, errors);
}
}

return {
check
};
});

// helpers //////////
function hasDuplicatedExecutionListeners(executionListeners, parentNode = null) {
return hasDuplicatedPropertiesValues(executionListeners, 'listeners', [ 'eventType', 'type' ], parentNode);
}
34 changes: 34 additions & 0 deletions rules/camunda-cloud/execution-listener.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
const {
findExtensionElement,
hasProperties
} = require('../utils/element');

const { reportErrors } = require('../utils/reporter');

const { skipInNonExecutableProcess } = require('../utils/rule');


module.exports = skipInNonExecutableProcess(function() {
function check(node, reporter) {
const executionListeners = findExtensionElement(node, 'zeebe:ExecutionListeners');

if (!executionListeners) {
return;
}

const listeners = executionListeners.get('listeners');
const errors = listeners.flatMap(listener => hasProperties(listener, {
type: {
required: true
}
}, node));

if (errors.length) {
reportErrors(node, reporter, errors);
}
}

return {
check
};
});
21 changes: 21 additions & 0 deletions rules/camunda-cloud/no-execution-listeners.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const { reportErrors } = require('../utils/reporter');

const { skipInNonExecutableProcess } = require('../utils/rule');

const { hasNoExtensionElement } = require('../utils/element');

const ALLOWED_VERSION = '8.6';

module.exports = skipInNonExecutableProcess(function() {
function check(node, reporter) {
const errors = hasNoExtensionElement(node, 'zeebe:ExecutionListeners', node, ALLOWED_VERSION);

if (errors && errors.length) {
reportErrors(node, reporter, errors);
}
}

return {
check
};
});
49 changes: 49 additions & 0 deletions rules/utils/element.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
const {
filter,
isArray,
isDefined,
isFunction,
isNil,
isObject,
isString,
isUndefined,
matchPattern,
some
} = require('min-dash');

Expand Down Expand Up @@ -133,6 +135,53 @@ module.exports.hasDuplicatedPropertyValues = function(node, propertiesName, prop
return [];
};

// @TODO(@barmac): use tree algorithm to reduce complexity
module.exports.hasDuplicatedPropertiesValues = function(node, containerPropertyName, propertiesNames, parentNode = null) {
const properties = node.get(containerPropertyName);

// (1) find duplicates
const duplicates = properties.reduce((foundDuplicates, property, index) => {
const previous = properties.slice(0, index);
const isDuplicate = previous.find(p => propertiesNames.every(propertyName => p.get(propertyName) === property.get(propertyName)));

if (isDuplicate) {
return foundDuplicates.concat(property);
}

return foundDuplicates;
}, []);

// (2) report error for each duplicate
if (duplicates.length) {
return duplicates.map(duplicate => {
const propertiesMap = {};
for (const property of propertiesNames) {
propertiesMap[property] = duplicate.get(property);
}

// (3) find properties with duplicate
const duplicateProperties = filter(properties, matchPattern(propertiesMap));
const duplicatesSummary = propertiesNames.map(propertyName => `property <${ propertyName }> with duplicate value of <${ propertiesMap[propertyName] }>`).join(', ');

// (4) report error
return {
message: `Properties of type <${ duplicate.$type }> have properties with duplicate values (${ duplicatesSummary })`,
path: null,
data: {
type: ERROR_TYPES.PROPERTY_VALUES_DUPLICATED,
node,
parentNode: parentNode == node ? null : parentNode,
duplicatedProperties: propertiesMap,
properties: duplicateProperties,
propertiesName: containerPropertyName
}
};
});
}

return [];
};

module.exports.hasProperties = function(node, properties, parentNode = null) {
return Object.entries(properties).reduce((results, property) => {
const [ propertyName, propertyChecks ] = property;
Expand Down
1 change: 1 addition & 0 deletions rules/utils/error-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module.exports.ERROR_TYPES = Object.freeze({
PROPERTY_REQUIRED: 'camunda.propertyRequired',
PROPERTY_TYPE_NOT_ALLOWED: 'camunda.propertyTypeNotAllowed',
PROPERTY_VALUE_DUPLICATED: 'camunda.propertyValueDuplicated',
PROPERTY_VALUES_DUPLICATED: 'camunda.propertiesValuesDuplicated',
PROPERTY_VALUE_NOT_ALLOWED: 'camunda.propertyValueNotAllowed',
PROPERTY_VALUE_REQUIRED: 'camunda.propertyValueRequired',
SECRET_EXPRESSION_FORMAT_DEPRECATED: 'camunda.secretExpressionFormatDeprecated'
Expand Down
Loading
Loading