Restrict extra props

[matthias-pichler]

Signed-off-by: Matthias Pichler [email protected]

Please specify parts of this PR update:

• Specification
• Schema
• Examples
• Extensions
• Use Cases
• Community
• CTK
• Other

Discussion or Issue link:

What this PR does:

I added a lot of unevaluatedProperties: false to restrict extra props

Additional information:

[cdavernas]

@JBBianchi Spot on, totally agree with you!

[JBBianchi]

I wonder if this PR isn't a bit too drastic (but it might not be).

An important feature of the spec is its extensibility. By restricting everything, we might lose a some of the flexibility.

For instance (as I commented earlier before deletion), the document could be extended by some runtime with metadata (e.g.: author, tenant, group, ...).

The author of a workflow also have access to very powerful variables, namely $workflow and $task. Therefore, he can define custom properties (e.g. $task.with.params.$filter or $workflow.constants.someKey) that could be used/remapped in some custom functions.

I'm not sure how to verify we didn't restrict the author too much here. At a glance, I think it's quite ok but it's not a very empirical approach ^^'

[matthias-pichler]

I wonder if this PR isn't a bit too drastic (but it might not be).

An important feature of the spec is its extensibility. By restricting everything, we might lose a some of the flexibility.

For instance (as I commented earlier before deletion), the document could be extended by some runtime with metadata (e.g.: author, tenant, group, ...).

The author of a workflow also have access to very powerful variables, namely $workflow and $task. Therefore, he can define custom properties (e.g. $task.with.params.$filter or $workflow.constants.someKey) that could be used/remapped in some custom functions.

I'm not sure how to verify we didn't restrict the author too much here. At a glance, I think it's quite ok but it's not a very empirical approach ^^'

Very good points here, however I feel like I have to counter with the fact that I came across the issue because some example workflows behaved differently than expected 😅 After updating the schema I had to fix 3 examples in the repo and none of us caught these errors. So on behalf of authors I would advocate for better feedback from tooling which need a schema.

Maybe we could introduce a dedicated field for metadata similar to JSON API 🤔

[cdavernas]

I agree with @JBBianchi that we should preserve extensibility, without however, as pointed out by @matthias-pichler-warrify, loosing (schema-based) validation features.

Maybe we could introduce a dedicated field for metadata similar to JSON API 🤔

Maybe we could reintroduce that property in specific places, indeed. Maybe even everywhere? I don't really have a strong opinion on that subject, so I'll leave it up to you guys 😉

[matthias-pichler]

Maybe we could reintroduce that property in specific places, indeed. Maybe even everywhere? I don't really have a strong opinion on that subject, so I'll leave it up to you guys 😉

@cdavernas what I also thought of just now:
alternatively we could enforce a prefix for custom/meta fields? Something like $, @ or _?

document: {}
do:
  - getPet:
       call: http
       with: {}
       $foo: bar

no preference for any given prefix on my side though

[cdavernas]

alternatively we could enforce a prefix for custom/meta fields?

Why not? Good idea, even though I personally am not a huge fan of it, as I think it would "dirty" the definition. But again, no strong opinion here anyways.

@JBBianchi @fjtirado @ricardozanini What do you guys think?

[fjtirado]

I think we might add a "custom" key in those places where we think implementors might add custom stuff.

For example

document: {}
do:
  - getPet:
       call: http
       with: {}
       custom: 
         alternativeBaseURI: www.alternative.com

[matthias-pichler]

@fjtirado

I think we might add a "custom" key in those places where we think implementors might add custom stuff.

Yeah so custom, meta or similar. (name is still open I think). However this would be for authors not implementors

[fjtirado]

@matthias-pichler-warrify Sure authors would use them and implementors might implement it ;)

[cdavernas]

LGTM, cheers! ❤️

We will however need to open a new issue/PR addressing @JBBianchi's comments ;)

[JBBianchi]

I think the callTask shouldn't be restricted, otherwise it clashes with custom functions, doesn't it ?

[ricardozanini]

@JBBianchi is this portion of the schema are you referring too?

- title: CallFunction
        $ref: '#/$defs/taskBase'
        type: object
        unevaluatedProperties: false
        required: [ call ]
        properties:
          call:
            type: string
            not:
              enum: ["asyncapi", "grpc", "http", "openapi"]
            description: The name of the function to call.
          with:
            type: object
            additionalProperties: true
            description: A name/value mapping of the parameters, if any, to call the function with.

If so, I think it's correct. It's the with property that requires any object and the schema seems to allow it.

A way of validating this PR is to add an example that breaks the rule on purpose. We should always add examples/test cases to PRs that change the schema. I'll try to reinforce that in the CI.

[JBBianchi]

@JBBianchi is this portion of the schema are you referring too?

- title: CallFunction
        $ref: '#/$defs/taskBase'
        type: object
        unevaluatedProperties: false
        required: [ call ]
        properties:
          call:
            type: string
            not:
              enum: ["asyncapi", "grpc", "http", "openapi"]
            description: The name of the function to call.
          with:
            type: object
            additionalProperties: true
            description: A name/value mapping of the parameters, if any, to call the function with.

If so, I think it's correct. It's the with property that requires any object and the schema seems to allow it.

You're absolutely right, I wasn't looking at the right place?

[matthias-pichler]

Is there anything else needed to merge this?

[cdavernas]

@matthias-pichler-warrify Not that I know of. Merged, thanks for the reminder!