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

Launch Implementer Version of the Specification #49

Closed
7 tasks done
frankkilcommins opened this issue Apr 14, 2023 · 48 comments
Closed
7 tasks done

Launch Implementer Version of the Specification #49

frankkilcommins opened this issue Apr 14, 2023 · 48 comments
Labels
implementor-draft In scope for first version

Comments

@frankkilcommins
Copy link
Collaborator

frankkilcommins commented Apr 14, 2023

This issue is to work towards deliver of an implementer version of the Workflows Specification that can be used by interested vendors (or community members) to build prototype tooling and feedback to the SIG on aspects of the specification.

How a bill becomes law?

We'll work with the OAS TDC to identify the steps needed to release and communicate on an initial implementer spec version. These discussions will also cover how the SIG Workflows group should also function moving forward (perhaps follow IETF process/approach for working groups).

Planned timelines and activities

@frankkilcommins frankkilcommins added the implementor-draft In scope for first version label Apr 26, 2023
@frankkilcommins
Copy link
Collaborator Author

From Neal Caidin on ability to host the Workflows Specification under the OAI:

Hello all. I asked LF if our charter covered adopting a new spec, like the Workflow spec. I hope I asked the right question? Here is the answer - As long as the new specification relates to "providing technical metadata for APIs" then it would appear to fall within the scope of the OpenAPI project. Please note, however, that the license for Specifications is the Apache-2.0 license. We now have various alternatives, and if the community would like to understand what open specification licenses are available to them, please let us know.

Link to slack conversation: https://open-api.slack.com/archives/C1137F8HF/p1683808672092199

@ralfhandl
Copy link
Contributor

Section Data Types: link to the Formats Registry, ideally per format.

@darrelmiller
Copy link
Member

The info.version property is defined as the version of the "Description" which could be composed of multiple documents. In OpenAPI the info.version is defined as the version of the document. Was this different intentional?

@ralfhandl
Copy link
Contributor

Workflows Specification Object Example, step

  - stepId: getPetStep
    description: retrieve a pet by status from the GET pets endpoint
    operationRef: https://petstore3.swagger.io/api/v3/openapi.json#/paths/users/~findbystatus~1{status}/get
    dependsOn: loginStep
    parameters:
      - name: status
        in: query
        value: 'available'
      - name: Authorization
        in: header
        value: $steps.loginUser.outputs.sessionToken

Assuming the sessionToken output of the previous step loginUser is a "raw" token, then the Authorization header is probably of the form

Authorization: Bearer <sessionToken>

How would such a concatenation of the prefix Bearer and an output value be expressed?

I'll look for it in the remainder of the document, but it would be nice if the example already told me 😄.

@darrelmiller
Copy link
Member

The literals description includes string but the table does not. It would be good to clarify whether single and/or double quotes are supported for strings. https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#literals

@darrelmiller
Copy link
Member

It appears that in the OpenAPI spec we did a particularly bad job of specifying how to use $statusCode in an expression. Most of the examples in workflows treat statusCode as a numeric value during comparison. However, there is an example here https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#fixed-fields-10 that treats it as a string. This might be fine but it set off my spidey senses.

@darrelmiller
Copy link
Member

darrelmiller commented Nov 30, 2023

https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#fixed-fields-4

The successCriteria property is a list of Criterion Objects that all must be satisfied. It might be worthwhile providing some guidance as to when to use an AND (&&) operator in a single criterion Object vs multiple criterionObjects. e.g. Will a list of criteron Objects "short circuit" if one fails?

@darrelmiller
Copy link
Member

Is this

- condition: $statusCode == 200 

the same as

-  context: $statusCode
   condition: 200
   type: simple

?
If so, what is the value of type simple?

@darrelmiller
Copy link
Member

the retryAfter field in the Failure Action Object is described as "A non-negative decimal indicating the milliseconds delay". Is this really supposed to say Decimal? i.e. someone can define fractions of a millisecond? Is there a strong reason to deviate from the HTTP standard of using seconds for retryAfter?

@darrelmiller
Copy link
Member

It would be valuable to have some examples and discussion around the onSuccess property. It would be good to move the Note about what happens when there are multiple success actions, up into the onSuccess property. It is the behavior of the array of success actions, not the success action itself that is being defined by the note.

https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#fixed-fields-6

@ralfhandl
Copy link
Contributor

ralfhandl commented Nov 30, 2023

Section Step Object, field operationId

If more than one (non workflowsSpec type) source description is defined within a Workflows Description, then the operationId specified MUST be prefixed with the source name to avoid ambiguity or potential clashes.

How prefixed? Just prepend without separator character? An example would be useful.

Same with field workflowId.

@ralfhandl
Copy link
Contributor

Section Step Object, field operationRef

A complete URI Template SHOULD be used.

The examples show a JSON Pointer with a fragment containing a path through the paths object, the URI template, ending at the operation object.

I find the examples convincing and am unable to deduce them from the field description.

@ralfhandl
Copy link
Contributor

ralfhandl commented Nov 30, 2023

Section Workflow Object, field inputs:

A JSON Schema 2020-12 object representing the input parameters used by this workflow.

Are there any cases where this JSON Schema object wouldn't say type: object?
What do I have to expect/accept as a tool implementor?

How would inputs be referenced if that schema isn't of type: object? The Runtime Expressions only allow

"$inputs." name

which seems to require type: object.

@frankkilcommins
Copy link
Collaborator Author

Section Data Types: link to the Formats Registry, ideally per format.

Thanks @ralfhandl - I've registered #96 for this.

@frankkilcommins
Copy link
Collaborator Author

defined as the version of the "Description"

Thanks @darrelmiller - this is being fixed via #98

@frankkilcommins
Copy link
Collaborator Author

Workflows Specification Object Example, step

  - stepId: getPetStep
    description: retrieve a pet by status from the GET pets endpoint
    operationRef: https://petstore3.swagger.io/api/v3/openapi.json#/paths/users/~findbystatus~1{status}/get
    dependsOn: loginStep
    parameters:
      - name: status
        in: query
        value: 'available'
      - name: Authorization
        in: header
        value: $steps.loginUser.outputs.sessionToken

Assuming the sessionToken output of the previous step loginUser is a "raw" token, then the Authorization header is probably of the form

Authorization: Bearer <sessionToken>

How would such a concatenation of the prefix Bearer and an output value be expressed?

I'll look for it in the remainder of the document, but it would be nice if the example already told me 😄.

String functions representations like concatenation are not currently supported. If we deem it worthwhile, then I would propose that we handle in the same manner as a Criterion.condition in so far as literals can be combined with runtime expressions and also leverage and extended set of operators. Alternatively, we'd need to introduce a limited set of functions (this is thread if we pull at it).

I'd tend to treat this as an enhancement for a future minor rev.

@frankkilcommins
Copy link
Collaborator Author

It appears that in the OpenAPI spec we did a particularly bad job of specifying how to use $statusCode in an expression. Most of the examples in workflows treat statusCode as a numeric value during comparison. However, there is an example here https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#fixed-fields-10 that treats it as a string. This might be fine but it set off my spidey senses.

I don't see it as a big issue, but perhaps we could provide a little more clarity for implementors to take appropriate parsing responsibility before evaluating such a condition

@frankkilcommins
Copy link
Collaborator Author

The literals description includes string but the table does not. It would be good to clarify whether single and/or double quotes are supported for strings. https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#literals

@darrelmiller thanks - nice spot. Addressed (whoops) in #99

@frankkilcommins
Copy link
Collaborator Author

the retryAfter field in the Failure Action Object is described as "A non-negative decimal indicating the milliseconds delay". Is this really supposed to say Decimal? i.e. someone can define fractions of a millisecond? Is there a strong reason to deviate from the HTTP standard of using seconds for retryAfter?

Thanks @darrelmiller - Issue #100 created for this. I've no idea how I deviated here......... I will keep the decimal description unless you think the Retry-After header should also be adjusted: "A delay-seconds value is a non-negative decimal integer, representing time in seconds"

@frankkilcommins
Copy link
Collaborator Author

https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#fixed-fields-4

The successCriteria property is a list of Criterion Objects that all must be satisfied. It might be worthwhile providing some guidance as to when to use an AND (&&) operator in a single criterion Object vs multiple criterionObjects. e.g. Will a list of criteron Objects "short circuit" if one fails?

I think it's self explanatory but happy to add an example if needed. However I'm not sure how prescriptive I want to be here for authors. The general rule of thumb is that if you have assertions which can not easily fit into a single Criterion Object (e.g. you may need to check a status code as well as apply some JSONPath expressions to check response body information).

Yes, if one criterion fails, then the step is deemed as unsuccessful. That may trigger other actions depending on the presence of the onFailure fixed field

@frankkilcommins
Copy link
Collaborator Author

frankkilcommins commented Nov 30, 2023

Is this

- condition: $statusCode == 200 

the same as

-  context: $statusCode
   condition: 200
   type: simple

? If so, what is the value of type simple?

@darrelmiller This made me smile, which is good!

Yes, they are effectively the same but somewhat unexpected usage. And the following is also the same:

- condition: $statusCode == 200 
  type: simple

'type' simple is to support those who want to be verbose in their expressiveness of the criterion. Most often, I would expect where short handed approaches to be leveraged for simple conditions.

Example:

- condition: $statusCode == 200 && $response.body != null

This would not be very clear if represented as the following IMHO as $statusCode is not the only context being evaluated

-  context: $statusCode
   condition: 200 && $response.body != null
   type: simple

Open to suggestions as to how to harden/clarify

@frankkilcommins
Copy link
Collaborator Author

Section Parameter Object, fixed field target says

Can be useful for targeting specific request body part.

Seems that I MUST provide exactly one parameter that is in: body and does not have a target to have a body whose parts I can then modify with further parameters that have a target.

@ralfhandl - I tend to agree with you here. We should perhaps make that explicit in the wording (or wording + examples).

If target is present, MUST I then specify in: body? Or is there an unmentioned in value if bodyPart?

An example would be helpful.

In the spirit it was written, in:body was expected in such scenarios. There is no bodyPart value. The expectation would be that you'd target specific locations should you wish.

There are some examples (which could be improved) at:

@frankkilcommins
Copy link
Collaborator Author

Sections Success Action Object and Failure Action Object, field stepID say

The referenced stepId SHOULD be within the current workflow.

If the step is in a different workflow, how do I provide values for the inputs of that other workflow?

Section Step Object, field stepId says

The id SHOULD be unique amongst all steps described in the workflow.

So I can have the same step id within one workflow, and I definitely can have the same step id within different workflows.

Which of these identically named steps is to be executed? All of them? The first? A randomly chosen one?

As an implementor I'd prefer to have a "MUST" in these three sentences to unambiguously identify the step to go to.

@ralfhandl #105 created to address the most pressing part of this feedback

Section Step Object, field stepId says

The id SHOULD be unique amongst all steps described in the workflow.

Workflow prefixing can be used to enable the ability to locate the desired step should a situation arise where there is a need to have a step with the same id within two described workflows. I'm OK living with this one.

@frankkilcommins
Copy link
Collaborator Author

The Table of Contents has multiple issues:

  1. "Criterion Object" is listed before "Reference Object", and the corresponding sections have the opposite order
  2. Workflows Specification - Version 1.0.0 looks odd, which may be caused by "Workflows Specification" being heading level 1 and "Version 1.0.0" being heading level 4 - where are the two intermediate levels?
  3. Same for Definitions - Workflows Description: "Definitions" is heading level 2, "Workflows Description" is heading level 5
  4. Sections "Security Considerations" and "IANA Considerations" are not listed
  5. Appendix B is not listed

@ralfhandl #106 created to address this feedback

@frankkilcommins
Copy link
Collaborator Author

frankkilcommins commented Dec 20, 2023

Section Reference Object, field $ref says

This MUST be in the form of a URI.

Which is only half of the truth: the fragment part - if present - apparently MUST be in the form of a JSON Pointer URI Fragment Identifier Representation.

Otherwise how is an implementation supposed to interpret the fragment part?

@ralfhandl I'll look to update the media type registrations to specify JSON Pointer as the fragment identifier mechanism, and add some additional text also into the section to clarify

#107 created for this feedback

@frankkilcommins
Copy link
Collaborator Author

Section Criterion Object says

String comparisons SHOULD be case insensitive.

Please make this a MUST for either sensitive or insensitive, don't care much which.

Different behavior across different workflow execution engines would be a nuisance.

@ralfhandl #108 created for this feedback

@frankkilcommins
Copy link
Collaborator Author

Section Runtime Expressions, ABNF rules message-header-reference and message-payload-reference: are these leftovers from deleted text or "sneak previews" of things to come?

Either way I'd remove them from this specification version.

@ralfhandl a sneak preview indeed. Created #109 to tidy up

@frankkilcommins
Copy link
Collaborator Author

Section IANA COnsiderations, vnd. prefix:

  • This prefix is for vendors, whereas standards don't require a prefix.

I see OAI in the standards category, not in the vendors category.

@darrelmiller any comment here? I was following along from the old comment on media types for OAS but I tend to agree with @ralfhandl that we should not be flagging these as vendor specific.

I'm happy to submit a change request on the registrations based on your feedback.

@handrews
Copy link
Member

@frankkilcommins that comment about vnd. media types long predates the more recent OAS media type draft RFC.

@frankkilcommins
Copy link
Collaborator Author

It would be valuable to have some examples and discussion around the onSuccess property. It would be good to move the Note about what happens when there are multiple success actions, up into the onSuccess property. It is the behavior of the array of success actions, not the success action itself that is being defined by the note.

https://github.com/OAI/sig-workflows/blob/main/versions/1.0.0.md#fixed-fields-6

@darrelmiller #118 created to move the notes up to the Step onSuccess and onFailure fixed fields.

@frankkilcommins
Copy link
Collaborator Author

Section Parameter Object says

There are five possible locations specified by the in field:

followed by a list of six values.

If the step has a workflowId I assume that all parameters must have in: workflow. In which case it would be easier to just omit the in field and remove the value workflow from the list, bringing it back to five possible values.

@ralfhandl Issue #119 created to review/harden based on this feedback

@frankkilcommins
Copy link
Collaborator Author

Section IANA COnsiderations, vnd. prefix:

  • This prefix is for vendors, whereas standards don't require a prefix.

I see OAI in the standards category, not in the vendors category.

@ralfhandl just closing off this issue comment. Please see here for current status/context.

@frankkilcommins
Copy link
Collaborator Author

Section Step Object, field operationId

If more than one (non workflowsSpec type) source description is defined within a Workflows Description, then the operationId specified MUST be prefixed with the source name to avoid ambiguity or potential clashes.

How prefixed? Just prepend without separator character? An example would be useful.

Same with field workflowId.

@ralfhandl issue #124 created to address this feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementor-draft In scope for first version
Projects
None yet
Development

No branches or pull requests

4 participants