serverlessworkflow / sdk-typescript

Typescript SDK for Serverless Workflow
https://serverlessworkflow.io/
Apache License 2.0
64 stars 16 forks source link

Validation does not work as expected when required files are based on `if..then..else..` conditions #95

Closed antmendoza closed 3 years ago

antmendoza commented 3 years ago

I suppose that this is an issue in the library we are using to validate object, (I will check the open issues tomorrow)

What happened:

foreachstateValidator is not working well when required files are based on if..then..else.. conditions

What you expected to happen:

According to the specification, foreachstate with
"name", "type", "inputCollection", "iterationParam", "actions", "end"

is a valid object

How to reproduce it:

The code below

import {actionBuilder, foreachstateBuilder, produceeventdefBuilder} from '../../src';

fdescribe("", () => {

    it('should xxx', function () {

        const object = foreachstateBuilder()
            .name("ProvisionOrdersState")
            .inputCollection("${ .orders }")
            .iterationParam("singleorder")
            .outputCollection("${ .provisionedOrders }")
            .actions([
                actionBuilder()
                    .functionRef({
                            refName: "provisionOrderFunction",
                            arguments: {
                                "order": "${ .singleorder }",
                            },
                        },
                    )
                    .build(),
            ])
            .end({
                    produceEvents: ([
                        produceeventdefBuilder()
                            .eventRef("provisioningCompleteEvent")
                            .data("${ .provisionedOrders }")
                            .build(),
                    ]),
                },
            )
            .build()

        expect(object.name).toEqual("ProvisionOrdersState")
    });

})

that generated the following object (a valid one)

{
    "name": "ProvisionOrdersState",
    "inputCollection": "${ .orders }",
    "iterationParam": "singleorder",
    "outputCollection": "${ .provisionedOrders }",
    "actions": [
        {
            "functionRef": {
                "refName": "provisionOrderFunction",
                "arguments": {
                    "order": "${ .singleorder }"
                }
            }
        }
    ],
    "end": {
        "produceEvents": [
            {
                "eventRef": "provisioningCompleteEvent",
                "data": "${ .provisionedOrders }"
            }
        ]
    },
    "type": "foreach"
}

fails with the following error

[
  {
    instancePath: '',
    schemaPath: '#/then/required',
    keyword: 'required',
    params: { missingProperty: 'workflowId' },
    message: "must have required property 'workflowId'"
  }
]
F

Failures:
1) should xxx
  Message:
    Error: Foreachstate is invalid: must have required property 'workflowId'

Anything else we need to know?:

adding to the object construction .usedForCompensation(false) works fine.

Environment:

JBBianchi commented 3 years ago

I noticed other validation failures with "oneof/required". For instance a valid Eventstate with not "transition" raises

[
  {
    instancePath: '',
    schemaPath: '#/oneOf/1/required',
    keyword: 'required',
    params: { missingProperty: 'transition' },
    message: "must have required property 'transition'"
  },
  {
    instancePath: '',
    schemaPath: '#/oneOf',
    keyword: 'oneOf',
    params: { passingSchemas: [Array] },
    message: 'must match exactly one schema in oneOf'
  }
]

I wonder if it's linked to the warnings:

unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
unknown format "uri" ignored in schema at path "#/oneOf/0"
antmendoza commented 3 years ago

I think that removing duplications in the json schema fix this error

see https://github.com/serverlessworkflow/specification/pull/381

antmendoza commented 3 years ago

I had a look to "conditional subschemas" https://json-schema.org/understanding-json-schema/reference/conditionals.html, apparently if the evaluated property has no value the condition is evaluated as true.

JBBianchi commented 3 years ago

I think that removing duplications in the json schema fix this error

see serverlessworkflow/specification#381

I had that impression indeed but didn't have time to investigate yet.

JBBianchi commented 3 years ago

I had a look to "conditional subschemas" https://json-schema.org/understanding-json-schema/reference/conditionals.html, apparently if the evaluated property has no value the condition is evaluated as true.

You are right, the doc says that exactly. So this issue isn't one (or at least not in the realm of the SDK), the validation seems to work as it should. At the specification level though, we might want to add "required": ["usedForCompensation"] to the if.

antmendoza commented 3 years ago

@JBBianchi

why this has change the label to wontfix ?

JBBianchi commented 3 years ago

Because this is not an issue, is it ? The validation works as the specification says it should. If there is a fix to be made, it would be at the schema level. Am I missing something?

antmendoza commented 3 years ago

still is something that need to be fixed

antmendoza commented 3 years ago

https://github.com/serverlessworkflow/specification/pull/385

JBBianchi commented 3 years ago

"Fixed" not really as it does what it's supposed to. But the default values in general needs to be addresses indeed. More generic version of the issue: #104

antmendoza commented 3 years ago

updated the json schema resolve this.

https://github.com/serverlessworkflow/specification/pull/406 https://github.com/serverlessworkflow/specification/pull/385