Bug in functions schema

[jscheid]

What seems off:

The functions key isn't nested inside properties here:

specification/schema/functions.json

Line 6 in 8a7152e

"functions": {

I think it is meant to be?

What you expected to be:

Nested inside properties.

Anything else we need to know?:

It might be a good idea to run all the schemas through a strict validator. ajv's strict mode would fit the bill, it reports:

Error: strict mode: unknown keyword: "functions" 

Environment:

• Specification version used:

[ricardozanini]

Which spec version? Main? Yes, we are missing a CI to validate our schemas indeed.

[jscheid]

Just follow my link and click "blame" to see it was introduced in bb5fa9f. No idea which spec versions that covers, 0.8+?

[ricardozanini]

The functions is a parent object defined in a separate file. I don't see why we should add functions within a properties attribute. So when validating, one should use all the files including in https://github.com/serverlessworkflow/specification/tree/main/schema directory.

See: https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.json#L94-L95

We have a few SDKs using these files to validate and generate workflows, and no one has reported a bug like this before. Do you mind giving more details of how are you validating on your end?

I'll try to do something on my side too using AJV.

[jscheid]

Ah, I see, if you use workflow.json then the reference resolves correctly.

I got confused because functions.json contains top-level keys type and required so it looks like you could use it as a standalone schema, but these settings aren't actually ever used since the top level isn't referenced anywhere, correct? Instead functions.json is a "library" useful only when referenced from workflow.json.

You could consider removing these top-level keys, and perhaps adding a comment, to make it more clear that the file isn't meant to be used standalone.

[ricardozanini]

@jscheid yes, these files are part of the bundle. This is a good suggestion since one might use only the functions schema to validate a single functions.json file. @cdavernas wdyt? I think it won't hurt to add properties to each independent file in the bundle, or design it in a way that can be used in both cases.

EDIT:

My gut feeling says that if we add properties to the schemas, we will start having applications trying to validate a worfklow like this:

{
    "functions": {
         "functions": {}
     } 
}

(attributes left in blank on purpose)

So what we can do is to have another schema, say function_singleton.json referencing the same function.json  schema. But also too much trouble.

The best of both worlds would be to have properties there and not influence the applications already built on top of it. But I can't test it atm. @jscheid do you mind running a few tests and reporting it back?

[jscheid]

You could move functions into definitions, change the reference in workflow.json into ...#/definitions/functions and add { ... properties: { functions: { "$ref": "#/definitions/functions" } } }. Wouldn't that be the best of both worlds?

[ricardozanini]

Actually, that might work. I'll run an experiment to have a CI check on the schema + examples with this.

[ricardozanini]

@jscheid I quickly tried locally with Ajv and if I change to:

"type": "object",
"oneOf": [
    {
      "type": "string",
      "format": "uri",
      "description": "URI to a resource containing function definitions (json or yaml)"
    },
    {
      "type": "array",
      "description": "Workflow function definitions",
      "items": {
        "type": "object",
        "$ref": "#/definitions/function"
      },
      "additionalItems": false,
      "minItems": 1
    }
],

Won't work since functions can be a string or an object. I believe that's the reason why it was made this way.

In the workflow schema I did:

    "functions": {
      "$ref": "functions.json#"
    }

[ricardozanini]

Ideally, we should study removing the OneOf (string or object) in our schemas and have an object with maybe a ref.

So we would have:

"function": {
    "ref": "file://myfunctions.json"
}

Instead of:

"function": "file://myfunctions.json"

[cdavernas]

@ricardozanini My initial reaction was to tell you that you are crazy, that we already have all SDKs implementing it some sort of PITA way.
But then, I stopped and thought for a second. AsyncAPI does it that way, and as an implementer of both specs, I must say the ref property (vs oneOf) is unbelievably easy to implement.
They have, however, some limitations, and cannot be used to replace all the oneOf instances in the schema (I'm thinking when they are "properly" used, for concretions, for example).

In conclusion, I'm game, but we'd have to advertize such breaking change before proceeding IMHO.

[ricardozanini]

Sure, we can do as part of 0.9. :)

[JBBianchi]

But arent't you moving the oneOf in the validation instead then ? Because if I understand, the function definition would be either { "ref": "an-uri" } or { "name": "my-func", "type": "rest", ... }

[ricardozanini]

@jscheid @cdavernas I made the oneOf work:

"oneOf": [
    {
      "type": "string",
      "format": "uri",
      "description": "URI to a resource containing function definitions (json or yaml)"
    },
    {
      "type": "array",
      "description": "Workflow function definitions",
      "items": {
        "type": "object",
        "$ref": "#/definitions/function"
      },
      "additionalItems": false,
      "minItems": 1
    }
  ]

Just had to remove type.

[ricardozanini]

But arent't you moving the oneOf in the validation instead then ? Because if I understand, the function definition would be either { "ref": "an-uri" } or { "name": "my-func", "type": "rest", ... }

In this case, we can add all the properties and require everything (like name, type, operation) only when ref is null.