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: disallow condition depending on the containing property #128

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

barmac
Copy link
Contributor

@barmac barmac commented Jan 17, 2024

Closes #125


Note that this requires the validator to accept $data property. The monaco editor handles this correctly, and in ajv you need to pass $data: true to the constructor.

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jan 17, 2024
@barmac barmac force-pushed the 125-disallow-properties-depending-on-itself branch 2 times, most recently from 85ad313 to d5c478a Compare January 17, 2024 13:32
@barmac
Copy link
Contributor Author

barmac commented Jan 17, 2024

The dropdown choices condition is still missing here.

@barmac barmac force-pushed the 125-disallow-properties-depending-on-itself branch from d5c478a to 13bd940 Compare January 19, 2024 12:48
@barmac barmac force-pushed the 125-disallow-properties-depending-on-itself branch from 13bd940 to 2ce41dd Compare January 19, 2024 14:29
@barmac barmac force-pushed the 125-disallow-properties-depending-on-itself branch from 2ce41dd to ff6cc7d Compare January 19, 2024 15:12
@barmac barmac marked this pull request as ready for review January 19, 2024 15:12
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 19, 2024
@barmac
Copy link
Contributor Author

barmac commented Jan 19, 2024

Choices#condition is there. The solution works in the ajv validator with $data: true, but does not highlight problems in Monaco (cf. #125 (comment)). However, the main goal of protection against invalid templates is achieved.

@barmac barmac requested review from nikku, a team and philippfromme and removed request for a team January 19, 2024 15:14
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

I'm not sold on whether baking this into the JSON schema validator is a good thing, especially as it adds quite some complexity to it.

Good thing is that we $comment on what is happening why. What would be the alternative, and did you consider it? I guess programmatic validation?

@barmac
Copy link
Contributor Author

barmac commented Jan 22, 2024

I'm not sold on whether baking this into the JSON schema validator is a good thing, especially as it adds quite some complexity to it.

Good thing is that we $comment on what is happening why. What would be the alternative, and did you consider it? I guess programmatic validation?

The alternative would be to implement the solution directly in https://github.com/bpmn-io/element-templates-validator. Let's consider the pros and cons of each option.

JSON schema:
(+) will be picked up automatically by the tools if they accept $data format
(+) is close to the specification of templates
(+) provides JSON path in the error that can be picked up by the tools
(-) is more difficult to understand if you know JS but not JSON schema

JS validator:
(+) easier to understand for JavaScript devs
(-) away from the rest of templates specification (schema)
(-) will never be picked up automatically by JSON schema tools

Also notice that the actual implementation is only in these two files:

The rest is refactoring via 34db754

@nikku
Copy link
Member

nikku commented Jan 22, 2024

I subscribe to the pros and cons, thanks for outlining them here.

Copy link
Member

@nikku nikku 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.

@barmac barmac merged commit dc53035 into main Jan 22, 2024
3 checks passed
@barmac barmac deleted the 125-disallow-properties-depending-on-itself branch January 22, 2024 16:29
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow properties depending on itself
2 participants