Question about validation

tsurdilo commented 3 years ago

What is the question:

Running the following code which is in main readme (only thing I changed is removed the end defnition)

import { WorkflowValidator, Specification } from '@severlessworkflow/sdk-typescript';

const workflow: Specification.Workflow = {
  id: 'helloworld',
  version: '1.0',
  name: 'Hello World Workflow',
  description: 'Inject Hello World',
  start: 'Hello State',
  states: [
    {
      type: 'inject',
      name: 'Hello State',
      data: {
        result: "Hello World!"
      }
    } as Specification.Injectstate
  ]
};
const workflowValidator: WorkflowValidator = new WorkflowValidator(workflow);
if (!workflowValidator.validate()) {
  workflowValidator.errors.forEach(error => console.error(error.message));
}

this is not giving me any errors. i think it should give user "no end definition found"

If you think this is bug..i would love to help with fixing this :)

antmendoza commented 3 years ago

I think that it is correct, since usedForCompensation = false 🤔

try to remove "name", for example

JBBianchi commented 3 years ago

It's indeed valid. usedForCompensation not being specified, the then block is used:

"required": [
  "name",
  "type",
  "data"
]

In the example, those 3 properties are specified and valid so the schema is :)

tsurdilo commented 3 years ago

oh wow then we have an issue with the schema. will fix asap. if usedforcompensation is false(default) one of the transition/end should be required. thanks!

JBBianchi commented 3 years ago

@tsurdilo there is another discussion about this, #95, which has the same root cause.

I think there are 3 ways to face the issue, from the one that seems the best to me to the least optimal one, I'd say:

Add "required": ["usedForCompensation"] to the if block so it's not possible to have an empty usedForCompensation, thus avoiding the default then problem
Invert the if/then/else logic and start with "if false" which would lead to use then if the property is false or "falsish" (undefined/null)
Add a default: false property to the description of usedForCompensation which indicates the persons implementing the schema that there is a default value to the property. (but if the person implementing the schema forget the defaulting, then the problem will subsist)

The latest might be combined with another one, the first one for instance.

tsurdilo commented 3 years ago

@antmendoza @JBBianchi

workflow.json schema defines:

      "if": {
        "properties": {
          "usedForCompensation": {
            "const": true
          }
        }
      },
      "then": {
        "required": [
          "name",
          "type",
          "data"
        ]
      },
      "else": {
        "oneOf": [
          {
            "required": [
              "name",
              "type",
              "data",
              "end"
            ]
          },
          {
            "required": [
              "name",
              "type",
              "data",
              "transition"
            ]
          }
        ]
      }

the default value for "usedForCompensation" is false everywhere from what I see:

"usedForCompensation": {
          "type": "boolean",
          "default": false,
          "description": "If true, this state is used to compensate another state. Default is false"
        }

so if usedForCompensation is not defined, should default to false and then the "else" block should kick in making either "end" or "transition" required.

Unless

 "if": {
        "properties": {
          "usedForCompensation": {
            "const": true
          }
        }
      }

is wrong. what this should define is "if value of usedForCompensation is true". wdyt?

tsurdilo commented 3 years ago

I think it should be:

"if": {
        "not": {
          "properties": {
            "usedForCompensation": {
              "enum": [ false ]
            }
          }
        }
      }

there are other places where i am apparently making the same mistake as well

JBBianchi commented 3 years ago

I didn't pay enough attention, I didn't check for the default values there, sorry.

usedForCompensation should indeed be defaulted to false by the SDK, thus making the validation work as it's intended.

It is still "implementation dependant" though, see https://json-schema.org/understanding-json-schema/reference/conditionals.html:

Note

In this example, “country” is not a required property. Because the “if” schema also 
doesn’t require the “country” property, it will pass and the “then” schema will apply.
Therefore, if the “country” property is not defined, the default behavior is to 
validate “postal_code” as a USA postal code. 

--> The “default” keyword doesn’t have an effect, but is nice to include for 
readers of the schema to more easily recognize the default behavior.

tsurdilo commented 3 years ago

@JBBianchi @antmendoza : https://github.com/serverlessworkflow/specification/pull/385 This should help I hope. Let me know if you think I need to change it to anything else.

JBBianchi commented 3 years ago

@tsurdilo @antmendoza This raises a more generic question about properties with default value (that are not required).

Let's take again the example of usedForCompensation. The schema says it has a default value of 'false' but, at the same time, the field is not required from what I understood.

So if we take that snippet:

const injected = injectstateBuilder()
  .name("Hello State")
  .data({
    "result": "Hello World!"
  })
  .end(true)
  .build();

What should injected.usedForCompensation be ?

undefined (none has been specified by the developper and the property is not required, so it's valid and the builder doesn't have to do anything)
false (none has been specified by the developper but the schema defines a default value to the builder assigns it)

tsurdilo commented 3 years ago

What should injected.usedForCompensation be ?

undefined (none has been specified by the developper and the property is not required, so it's valid and the builder doesn't have to do anything)

false (none has been specified by the developper but the schema defines a default value to the builder assigns it)

From the logical perspective it should be "false" (it should always use the default value if not specified).

One more thing just to mention when you convert this object model to json/yaml, it should not include the "usedForCompensation" property. For properties that are not required and have a default, if possible, if they are not explicitly specified in the user definition, they should not be included in the resulting markup. Hope this makes sense.

JBBianchi commented 3 years ago

What should injected.usedForCompensation be ?

undefined (none has been specified by the developper and the property is not required, so it's valid and the builder doesn't have to do anything)

false (none has been specified by the developper but the schema defines a default value to the builder assigns it)

From the logical perspective it should be "false" (it should always use the default value if not specified).

One more thing just to mention when you convert this object model to json/yaml, it should not include the "usedForCompensation" property. For properties that are not required and have a default, if possible, if they are not explicitly specified in the user definition, they should not be included in the resulting markup. Hope this makes sense.

You're are saying two different things here ^^' ... It should be consistent in both case, "in memory" or "serialized".

At first glance, in my vision of the SDK (which is to wrap and validate the models, nothing more), the property should stay undefined. A runtime using the SDK though would have to deduce the default value as the property is not set.

I must admit that it's a tricky one.

tsurdilo commented 3 years ago

@JBBianchi yeah I understand. this was a pain in the java sdk and had to add a ton of custom serializers / deserializers like this one https://github.com/serverlessworkflow/sdk-java/blob/main/api/src/main/java/io/serverlessworkflow/api/serializers/CronSerializer.java

I think for now if its possible to just make sure that in the object model the default values are placed (if associated non-required elements have a default value) that would rock. I don't think we should just push that onto runtimes as those definitions of defaults are in our schema defs.

As far as if those values are spit back out in the json/yaml thats a non-issue for me really.

antmendoza commented 3 years ago

@JBBianchi yeah I understand. this was a pain in the java sdk and had to add a ton of custom serializers / deserializers like this one https://github.com/serverlessworkflow/sdk-java/blob/main/api/src/main/java/io/serverlessworkflow/api/serializers/CronSerializer.java

I think for now if its possible to just make sure that in the object model the default values are placed (if associated non-required elements have a default value) that would rock. I don't think we should just push that onto runtimes as those definitions of defaults are in our schema defs.

As far as if those values are spit back out in the json/yaml thats a non-issue for me really.

Hi @tsurdilo @JBBianchi , should this be agreed with the .net and go sdks?

I agree anyway, having default values in place avoid misinterpretations for both runtimes engines and personas.

tsurdilo commented 3 years ago

@antmendoza i think the problem is that without setting the default values (for those that have them and that are not required), the validity of the workflow definition is compromised.

default values again for props that are not required and not set by user must be set automatically by the object model

if some sdks dont do it that should be fixed imo please

antmendoza commented 3 years ago

@tsurdilo if the validity of the workflow definition is compromised without setting default values, should we consider those properties to be required by the json schema?

tsurdilo commented 3 years ago

@antmendoza sorry i am confused on this a little. from just the DSL perspective, lets use this example of the top-level expressionLang property:

 "expressionLang": {
      "type": "string",
      "description": "Identifies the expression language used for workflow expressions. Default is 'jq'",
      "default": "jq",
      "minLength": 1
    }

this parameter is not required, so user dont have to specify it. however when you read in json/yaml which does not have this set, the object model if asked "whats the expression language" should return "jq" which is the default value.

Does this help? Maybe I'm just not describing it right.

tsurdilo commented 3 years ago

@antmendoza what we do in java sdk at least, we pre-set the default values, so for example in Workflow.java we would have:

@JsonProperty("expressionLang")
@JsonPropertyDescription("Identifies the expression language used for workflow expressions. Default is 'jq'")
@Size(min = 1)
private java.lang.String expressionLang = "jq";

and it has the setters to change it if its specified in the DSL json/yaml.

tsurdilo commented 3 years ago

@antmendoza are you (sorry that I don't know) auto-generating from schemas? if so maybe that tool has a setting to pre-set defaults?

antmendoza commented 3 years ago

Yes @tsurdilo , is been doing in that way.

Thanks for the clarification ;)

JBBianchi commented 3 years ago

@tsurdilo If I may, there is two moving parts here. On one side, types/interfaces are generated based on the schema. They don't hold any value, they don't exist at runtime, they mostly are used for safety/ease of dev.

On the other hand, builders are generated based on the types declaration files (extracted with a simple regex) and have no direct link with the schema. The builders, as you might know, are proxies used to actually build the objects.

To define consts, we added a little code snippet dictionary that allows to inject some custom code in the generated builders. This allows to add the type to the different states for instance, and, in the pending PR (#108), to also add defaults. Unfortunatly, this little snippet dictionary is maintained manually for now, it's not generated. There is an open issue to address the problem and already generate part of this based on the schema, #105.

I hope this gives you a better understanding of how thing works under the hood. As usual, feel free to ask any question or provide feedbacks.

tsurdilo commented 3 years ago

@JBBianchi thanks! just wondering so there is no way to generate the typescript classes out of the schema? all the builders really look the same to me more or less and interfaces are nice to have but like you said can be used to create new classes but dont hold any value.

i guess there is no "default" keyword in typescript like java has for interfaces default return value :)

But either way feel free to ignore this as I amI by far not an expert in typescript, just would like to help somehow with this so you dont have to manually maintain default values for stuff.

JBBianchi commented 3 years ago

@JBBianchi thanks! just wondering so there is no way to generate the typescript classes out of the schema? all the builders really look the same to me more or less and interfaces are nice to have but like you said can be used to create new classes but dont hold any value.

The only tool outputing classes was quicktype but it was not working as I would expect it to. The major problem I recall with it is that it's merging union types into one class.

i guess there is no "default" keyword in typescript like java has for interfaces default return value :)

Not that I know at least but I'm no expert neither so maybe I'm missing something.

antmendoza commented 3 years ago

I think that there is nothing here still to discuss, but feel free to reopen it or leave a comment if I am wrong

serverlessworkflow / sdk-typescript

Question about validation #99