-
Notifications
You must be signed in to change notification settings - Fork 6
Handle version 0 correctly during validation
#216
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in template validation where templates without a version and templates with version 0 were incorrectly treated as duplicates. Previously, the code used template.version || '_' which converted the falsy value 0 to '_', causing version 0 templates to collide with unversioned templates. The fix changes this to use isNil(template.version) ? '_' : template.version, which only treats null and undefined as "no version" while preserving 0 as a valid version value.
Changes:
- Updated version checking logic in both element-templates and cloud-element-templates validators to use
isNil()instead of falsy checks - Added test cases to verify templates with no version and version 0 are treated as distinct
- Updated test fixtures and BPMN diagrams to support the new test scenarios
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/element-templates/Validator.js | Updated _validateTemplate to use isNil() for version checking; imported isNil from min-dash |
| src/cloud-element-templates/Validator.js | Updated _validateTemplate to use isNil() for version checking; imported isNil from min-dash |
| test/spec/element-templates/Validator.spec.js | Added test case verifying templates with no version and version 0 are accepted as different versions |
| test/spec/cloud-element-templates/Validator.spec.js | Added test case verifying templates with no version and version 0 are accepted as different versions |
| test/spec/cloud-element-templates/fixtures/falsy-version.json | Updated fixture to include both a template without version and a template with version 0 |
| test/spec/cloud-element-templates/fixtures/complex.bpmn | Added a task element referencing the falsy-template for integration testing |
| CHANGELOG.md | Added entry documenting the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6d6424f to
2a90321
Compare
2a90321 to
fa248bf
Compare
fa248bf to
caa7f36
Compare
0 correctly during validation
philippfromme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! ✅ I simplified the changelog entry a little bit and adjusted the commit messages. We always want to make sure we mention the issue in the commit message (e.g. Closes #123).
Proposed Changes
Closes #176
The issue was in wrong template validation for case when template without version and with '0' version was assumed as duplicates. In this case template with '0' version was not added to the valid templates list and wasn't proposed for the update. For all other cases, e.g. template without version and template with version > 0, update logic worked as expected.
Checklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}@bpmn-io/srtool