Allow setting a messages json schema to a local file reference.#150
Open
danfunk wants to merge 1 commit into
Open
Allow setting a messages json schema to a local file reference.#150danfunk wants to merge 1 commit into
danfunk wants to merge 1 commit into
Conversation
So it is possible to update set and retrieve a link to a json schema, as shown below.
```xml
<bpmn:message id="remote_update" name="remote_update">
<bpmn:extensionElements>
<spiffworkflow:property name="formJsonSchemaFilename" value="mission-parameters-schema.json" />
</spiffworkflow:properties>
</bpmn:message>
```
Contributor
📝 WalkthroughWalkthroughThe changes introduce two new helper functions for managing message schema files ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/spec/MessagesSpec.js (1)
315-343: Add the missing clear-schema regression.This only proves that a message with no prior schema stays empty. It will still pass if an existing
formJsonSchemaFilenamesurvives an update. Please seedlove_letterwith an old schema first, then fire the event with the chosen clear contract and assert the extension is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/spiffworkflow/messages/MessageHelpers.js`:
- Around line 920-941: The helper setMessageSchemaFile currently mutates
messageBo.extensionElements directly which bypasses the model command stack;
update it to perform the same changes via the modeling/command stack instead.
Specifically, change setMessageSchemaFile to accept (or obtain) the commandStack
or modeling service and wrap the creation/assignment of extensionElements,
Properties, Property and the prop.value change inside a commandStack.execute (or
modeling.updateModdleProperties / equivalent modeling API) call so the mutation
is executed through the command stack and supports undo/redo and modeling
listeners; reference the existing symbols setMessageSchemaFile, messageBo,
moddle, extensionElements, spiffworkflow:Properties and formJsonSchemaFilename
when locating where to apply this change.
- Around line 835-884: The code in syncCorrelationProperties wrongly requires
msgObject to be present to mark a correlation property as used, causing
properties referenced by other messages to be deleted when msgObject is
undefined; update the usage check inside the loop over
cProperty.correlationPropertyRetrievalExpression to mark isUsed true whenever
msgRef exists and either msgObject is not provided or the msgRef id differs from
msgObject.identifier (e.g. isUsed = msgRef && (!msgObject ||
cpExpression.messageRef.id !== msgObject.identifier) ? true : isUsed), and
additionally tighten the deletion guard for correlation properties by checking
existing messages in definitions.rootElements for any message element whose
correlation_properties reference cProperty.id (use correlation_properties and
correlationPropertyRetrievalExpression identifiers) before removing cProperty.
In
`@app/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageSelect.jsx`:
- Around line 139-145: The code only updates the BPMN message when
event.schema_file is truthy, so an explicit empty string never clears an
existing formJsonSchemaFilename; modify the branch in MessageSelect.jsx that
calls getRoot/findMessageById/setMessageSchemaFile so it treats an explicit ''
as a clear action: if event.schema_file === '' call
setMessageSchemaFile(bpmnMessage, '', moddle) (or remove/undefined depending on
setMessageSchemaFile's contract) to clear the existing value, otherwise when
event.schema_file is defined and non-empty call setMessageSchemaFile as before;
keep using getRoot and findMessageById to locate bpmnMessage and always pass
moddle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12d49d65-05db-44fe-bd24-f22a2c80c123
📒 Files selected for processing (5)
app/spiffworkflow/messages/MessageHelpers.jsapp/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageJsonSchemaSelect.jsapp/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageLaunchEditorButton.jsapp/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageSelect.jsxtest/spec/MessagesSpec.js
💤 Files with no reviewable changes (1)
- app/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageLaunchEditorButton.js
Comment on lines
+835
to
884
| for (let cProperty of correlationProps) { | ||
| let isUsed = false; | ||
| const expressionsToDelete = []; | ||
| for (const cpExpression of cProperty.correlationPropertyRetrievalExpression) { | ||
| const msgRef = cpExpression.messageRef | ||
| ? findMessageElement( | ||
| businessObject, | ||
| cpExpression.messageRef.id, | ||
| definitions | ||
| ) | ||
| : undefined; | ||
| isUsed = | ||
| msgRef && | ||
| msgObject && | ||
| cpExpression.messageRef.id !== msgObject.identifier | ||
| ? true | ||
| : isUsed; | ||
| // if unused false, delete retrival expression | ||
| if (!msgRef) { | ||
| return false; | ||
| expressionsToDelete.push(cpExpression); | ||
| } | ||
| } | ||
|
|
||
| if (messageId && cpExpression.messageRef.id === messageId) { | ||
| return activePropertyIds.has(cProperty.id); | ||
| } | ||
|
|
||
| return true; | ||
| }); | ||
|
|
||
| cProperty.correlationPropertyRetrievalExpression = nextExpressions; | ||
|
|
||
| const hasOtherMessageReferences = nextExpressions.some( | ||
| (cpExpression) => cpExpression.messageRef?.id !== messageId | ||
| ); | ||
| const keepProperty = | ||
| activePropertyIds.has(cProperty.id) || hasOtherMessageReferences; | ||
| const cPropertyIndex = definitions.get('rootElements').indexOf(cProperty); | ||
|
|
||
| if (!keepProperty) { | ||
| if (cPropertyIndex > -1) { | ||
| definitions.rootElements.splice(cPropertyIndex, 1); | ||
| // Delete the retrieval expressions that are not used | ||
| for (const expression of expressionsToDelete) { | ||
| const index = | ||
| cProperty.correlationPropertyRetrievalExpression.indexOf(expression); | ||
| if (index > -1) { | ||
| cProperty.correlationPropertyRetrievalExpression.splice(index, 1); | ||
| const cPropertyIndex = definitions | ||
| .get('rootElements') | ||
| .indexOf(cProperty); | ||
| definitions.rootElements.splice(cPropertyIndex, 1, cProperty); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (cPropertyIndex > -1) { | ||
| definitions.rootElements.splice(cPropertyIndex, 1, cProperty); | ||
| // If Unused, delete the correlation property | ||
| const propertyToBeDeleted = | ||
| isUsed || | ||
| (msgObject && | ||
| msgObject.correlation_properties && | ||
| msgObject.correlation_properties.some( | ||
| (obj) => obj.identifier === cProperty.id | ||
| )); | ||
| if (!propertyToBeDeleted) { | ||
| const index = definitions.get('rootElements').indexOf(cProperty); | ||
| if (index > -1) { | ||
| definitions.rootElements.splice(index, 1); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This deletes unrelated correlation properties during message removal.
syncCorrelationProperties is also called from app/spiffworkflow/messages/MessageInterceptor.js:41-56 without msgObject. In that path, isUsed can never become true because of the msgObject && ... guard, so correlation properties still referenced by other messages are dropped as soon as one message is deleted.
Safer direction
+ const currentMessageId = msgObject?.identifier;
for (let cProperty of correlationProps) {
let isUsed = false;
const expressionsToDelete = [];
- for (const cpExpression of cProperty.correlationPropertyRetrievalExpression) {
- const msgRef = cpExpression.messageRef
- ? findMessageElement(
- businessObject,
- cpExpression.messageRef.id,
- definitions
- )
- : undefined;
- isUsed =
- msgRef &&
- msgObject &&
- cpExpression.messageRef.id !== msgObject.identifier
- ? true
- : isUsed;
+ for (const cpExpression of cProperty.correlationPropertyRetrievalExpression || []) {
+ const referencedId = cpExpression.messageRef?.id;
+ const msgRef = definitions.rootElements?.find(
+ (rootElement) =>
+ rootElement.$type === 'bpmn:Message' && rootElement.id === referencedId
+ );
+ if (msgRef && (!currentMessageId || referencedId !== currentMessageId)) {
+ isUsed = true;
+ }
if (!msgRef) {
expressionsToDelete.push(cpExpression);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/spiffworkflow/messages/MessageHelpers.js` around lines 835 - 884, The
code in syncCorrelationProperties wrongly requires msgObject to be present to
mark a correlation property as used, causing properties referenced by other
messages to be deleted when msgObject is undefined; update the usage check
inside the loop over cProperty.correlationPropertyRetrievalExpression to mark
isUsed true whenever msgRef exists and either msgObject is not provided or the
msgRef id differs from msgObject.identifier (e.g. isUsed = msgRef && (!msgObject
|| cpExpression.messageRef.id !== msgObject.identifier) ? true : isUsed), and
additionally tighten the deletion guard for correlation properties by checking
existing messages in definitions.rootElements for any message element whose
correlation_properties reference cProperty.id (use correlation_properties and
correlationPropertyRetrievalExpression identifiers) before removing cProperty.
Comment on lines
+920
to
+941
| export function setMessageSchemaFile(messageBo, schemaFile, moddle) { | ||
| if (!messageBo.extensionElements) { | ||
| messageBo.extensionElements = moddle.create('bpmn:ExtensionElements'); | ||
| } | ||
| const exts = messageBo.extensionElements; | ||
| let props = exts | ||
| .get('values') | ||
| .find((e) => e.$instanceOf('spiffworkflow:Properties')); | ||
| if (!props) { | ||
| props = moddle.create('spiffworkflow:Properties'); | ||
| exts.get('values').push(props); | ||
| } | ||
| let prop = props | ||
| .get('properties') | ||
| .find((p) => p.name === 'formJsonSchemaFilename'); | ||
| if (!prop) { | ||
| prop = moddle.create('spiffworkflow:Property'); | ||
| props.get('properties').push(prop); | ||
| } | ||
| prop.name = 'formJsonSchemaFilename'; | ||
| prop.value = schemaFile; | ||
| } |
Contributor
There was a problem hiding this comment.
Run schema-file mutations through the command stack.
This helper mutates extensionElements in place. The new callers in the properties panel and add-message event flow use it without an element.update* command, so schema edits bypass undo/redo and other modeling listeners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/spiffworkflow/messages/MessageHelpers.js` around lines 920 - 941, The
helper setMessageSchemaFile currently mutates messageBo.extensionElements
directly which bypasses the model command stack; update it to perform the same
changes via the modeling/command stack instead. Specifically, change
setMessageSchemaFile to accept (or obtain) the commandStack or modeling service
and wrap the creation/assignment of extensionElements, Properties, Property and
the prop.value change inside a commandStack.execute (or
modeling.updateModdleProperties / equivalent modeling API) call so the mutation
is executed through the command stack and supports undo/redo and modeling
listeners; reference the existing symbols setMessageSchemaFile, messageBo,
moddle, extensionElements, spiffworkflow:Properties and formJsonSchemaFilename
when locating where to apply this change.
Comment on lines
+139
to
+145
| if (event.schema_file) { | ||
| const defs = getRoot(element.businessObject); | ||
| const bpmnMessage = findMessageById(defs, event.name); | ||
| if (bpmnMessage) { | ||
| setMessageSchemaFile(bpmnMessage, event.schema_file, moddle); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle empty schema_file as a clear, not a no-op.
This branch only runs for truthy values, so updating an existing message with schema_file: '' leaves the old formJsonSchemaFilename behind. That makes schema removal impossible from this event path.
Possible fix
- if (event.schema_file) {
+ if ('schema_file' in event) {
const defs = getRoot(element.businessObject);
const bpmnMessage = findMessageById(defs, event.name);
if (bpmnMessage) {
- setMessageSchemaFile(bpmnMessage, event.schema_file, moddle);
+ setMessageSchemaFile(bpmnMessage, event.schema_file ?? '', moddle);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (event.schema_file) { | |
| const defs = getRoot(element.businessObject); | |
| const bpmnMessage = findMessageById(defs, event.name); | |
| if (bpmnMessage) { | |
| setMessageSchemaFile(bpmnMessage, event.schema_file, moddle); | |
| } | |
| } | |
| if ('schema_file' in event) { | |
| const defs = getRoot(element.businessObject); | |
| const bpmnMessage = findMessageById(defs, event.name); | |
| if (bpmnMessage) { | |
| setMessageSchemaFile(bpmnMessage, event.schema_file ?? '', moddle); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageSelect.jsx`
around lines 139 - 145, The code only updates the BPMN message when
event.schema_file is truthy, so an explicit empty string never clears an
existing formJsonSchemaFilename; modify the branch in MessageSelect.jsx that
calls getRoot/findMessageById/setMessageSchemaFile so it treats an explicit ''
as a clear action: if event.schema_file === '' call
setMessageSchemaFile(bpmnMessage, '', moddle) (or remove/undefined depending on
setMessageSchemaFile's contract) to clear the existing value, otherwise when
event.schema_file is defined and non-empty call setMessageSchemaFile as before;
keep using getRoot and findMessageById to locate bpmnMessage and always pass
moddle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows us to connect a json schema file to a bpmn:Message. This will allow the BPMN file to contain everything about how a message works, and provides a way to collaborate on a message definition between Ed and Arena such that everything is included in the BPMN file and no additional configuration files are required - working much the same way user tasks currently reference locally json files.
We will still have to deal with the problem of the schema up-search in Arena.
So it is possible to update and retrieve a link to a json schema that is connected directy to a bpmn:message element, as shown below.