V2 openapi fixes

[jerstlouis]

This PR contains several fixes in an initial attempt to fix the OpenAPI example definitions ( #401 and #390 ).

Please review the individual commits, with the earlier commits potentially easier to merge.

See also the obvious propreties typo that I pushed directly (77cec18).

At different commits along this Pull Request:

• swagger-cli bundle works again
• swagger-cli validate works again
• Swagger Editor errors are gone
• Swagger UI shows "valid"

EDIT: Swagger UI now says valid.

(EDIT: It actually did not after properly including the CWL schema, but now it does!)

This also includes a fix for #562.

OAS does not allow using $defs in the final schemas being used in the bundle, so embedding in the repo the CWL and CQL2 schemas allors to tweak that.

Note that that this PR (in the later commits) embeds a local version of the CQL2 schema, CWL schema and a simplified JSON (meta)Schema to get this all to work. If we also embed the GeoJSON schemas (I have this in a separate branch that I did not push), then the bundling is a lot faster and it could be done in a CI/CD without network access.

Also note that we should still review any differences remaining between the files in: schemas/processes-workflows vs. schemas/processes-core.

cc. @pvretano @bpross-52n @fmigneault @gfenoy

Note that I've changed some references to CWLschema at:

https://raw.githubusercontent.com/common-workflow-lab/cwl-ts-auto/main/json_schemas/cwl_schema.yaml

to the w3id.org one which fixes this error in Swagger Editor:

properties members must be schemas

because it does:

    properties:
      $schemas: true
      id: true
      stderr: true
      successCodes: true
      cwlVersion: true
      outputs: true
      label: true
      stdout: true
      requirements: true
      doc: true
      temporaryFailCodes: true
      expression: true

(I don't know if this is supposed to be valid somehow...)

(and now the schema is actually copied to the repo and tweaked to fix the errors)

[fmigneault]

@jerstlouis
Why update the CWL schemas?
The official ones published are already defined:

ogcapi-processes/openapi/paths/processes-dru/pProcessListDeploy.yaml

Lines 32 to 40 in 77cec18

	application/cwl:
	schema:
	$ref: "https://w3id.org/cwl/v1.2/cwl-json-schema.yaml"
	application/cwl+json:
	schema:
	$ref: "https://w3id.org/cwl/v1.2/cwl-json-schema.yaml"
	application/cwl+yaml:
	schema:
	$ref: "https://w3id.org/cwl/v1.2/cwl-json-schema.yaml"

Changing them could make deployments relying on them fail since they are different definitions (technically).

Is this branch aligned with #547
There are massive amounts of pending updates over there, and I trust them more since we validated them during the code sprint.

[jerstlouis]

@fmigneault That is the schema I used there (from w3id.org), just trying to make it work with OAS 3.0 which did not work.

Now although SwaggerUI says valid the schema shows blank so I want to see if something is still wrong.

About #547, it doesn't seem to be touching the openapi/ directory which is all I changed.

[jerstlouis]

@fmigneault Do you know if we've officially given up on OpenAPI 3.0? The OpenAPI definition was previously 3.0 and I was trying to make it work with 3.0...

[pvretano]

@jerstlouis both 3.0 and 3.1 are supposed to be supported in this version. There are conformance classes for both.

[jerstlouis]

@pvretano I was referring specifically to the version of the OpenAPI components that we want to provide.

Of course implementers can describe their API with OAS 3.0 or 3.1 or even another API definition language...

But if we want the components to work with OAS 3.0, then we cannot directly reference JSON Schemas like the CWL and CQL2, hence why I'm trying to embed an OAS 3.0-friendly version of the schemas...

Of course we could also offer a 3.1 version of the building blocks which do directly include the online JSON Schemas, but it would be nice if we still have a working 3.0 version.

[jerstlouis]

@fmigneault Would you have any advice on what to replace the conditionals with the CWL schema ? Some of these could be a oneOf:, but others that only change the default like:

        if:
          properties:
            type:
              const: 'null'
        then:
          properties:
            default:
              type: 'null'

I really don't understand... If something is specified, why have a default conditional on what it is?!
(EDIT: I think I was lost in the meta-levels here... this is redefining some kind of meta schema or something :S)

I guess this really cannot be done reasonably well with OAS 3 and shouldn't need to be done anyways so it should just be

    type:
      $ref: '#/components/schemas/AnyType'
    default:
      $ref: '#/components/schemas/AnyType'

? The purpose of the API definition is documentation rather than validation...

Or is there already an OAS 3-friendly version of this schema or one close to it somewhere that we could use?

[jerstlouis]

@fmigneault There are things in this official CWL schema ( https://www.commonwl.org/v1.2/cwl-json-schema.yaml ) that I believe are not even valid in JSON Schema e.g.:

envValue:
  type:
    $ref: '#/$defs/CWLExpression'

Another error:

      required:
        - expressionLib

this is inside properties but should be outside.

[fmigneault]

Must be just an indent error. We can patch it.
This schema is (pseudo)manually created from extracted code running the https://github.com/crim-ca/weaver/blob/master/weaver/wps_restapi/swagger_definitions.py

Is that the only errors? I can open the fix PR.

[fmigneault]

Provided that swagger-cli say "we can't keep up with issues" at the start of their readme, I would rely more on redocly fixing the few edge cases of OAS that were not working properly.

For cross-repo, it could simply be a recursive git clone --depth 0 done before the operation, similar to other patches done by openapi/update.sh. It doesn't have to all be done by the openapi tool, but we could avoid the manual copies to ensure latest changes are always evaluated rather than finding out much later.

[jerstlouis]

@fmigneault

For cross-repo, it could simply be a recursive git clone --depth 0 done before the operatio

Perhaps if we tag a specific commit / release and sub-directory that could work.

e.g.,

git archive --remote=https://github.com/opengeospatial/ogcapi-common/ 3dfaa0619907be6a6b0d5ce27a3ea4229c1ec4d1:collections/openapi

(archive doesn't seem supported from GitHub, we could do something with sparse-checkout etc. but it's not that simple)

but that would require to make sure everything is perfectly synced to use those external repos.

(and still requires network access after the clone, but we could have a script to fetch all that, and the the bundling command is local)

[fmigneault]

but that would require to make sure everything is perfectly synced to use those external repos.

That is exactly the idea.
If something breaks, we would be made aware of it right away rather than a user raising an issue many months after a version was released with broken standards.

Oh, and regarding the deployment response, I saw on https://github.com/opengeospatial/ogcapi-processes/blob/master/extensions/deploy_replace_undeploy/standard/requirements/deploy-replace-undeploy/deploy/REQ_response-body.adoc, so we cannot have it use processDescription since it changes expectations of the requirement (although formally you could still return inputs/outputs if desired since not validated).

[jerstlouis]

@fmigneault

If something breaks, we would be made aware of it right away rather than a user raising an issue many months after a version was released with broken standards.

But this works both ways.

If we release OGC API - Processes in a working state, and then the other repository changes (especially if we use the GitHub repo which are dynamic and always reflect the Editor's Draft rather than a specific version), then what used to work stops working (but I guess this clone/copy would only happen while Procsses itself in a draft stage... the released version would not update or directly reference the dependencies)

References to a moving target are more problematic, but we can have either an automatic action or a manual process to regularly update all references to external repo on a regular basis. There are also timing issues, where in a certain stage of development / release freeze cycle you might not want to update anything that is being changed elsewhere.

About

https://github.com/opengeospatial/ogcapi-processes/blob/master/extensions/deploy_replace_undeploy/standard/requirements/deploy-replace-undeploy/deploy/REQ_response-body.adoc

There's a grammar issue right now with that, and yes the processDescription still is valid with that requirement. But we can define a new response for it specifically, especially if the inputs / outputs (descriptions, right?) never make sense to include.

But actually I am questioning that?
Is this the basic DRU response when you deploy a new process?
The inputs and outputs description should be included in the response, especially since some process packages could have the server infer these, so it would make complete sense to the tell the client right away what are the inputs and outputs that result from the package it just deployed?

[fmigneault]

@jerstlouis
FYI, schema fixes from common-workflow-language/cwl-v1.2#306 have been deployed:
https://www.commonwl.org/v1.2/cwl-json-schema.yaml

[fmigneault]

If we release OGC API - Processes in a working state, and then the other repository changes (especially if we use the GitHub repo which are dynamic and always reflect the Editor's Draft rather than a specific version), then what used to work stops working

As long as the changes are happening in master/main between the repos, yes, they should break as early as possible so we notice it and fix in timely fashion. Then, once the referenced repo releases its version that is our desired reference, you "pin" your reference to it in OAP. A release of OAP should never happen while relying on a pending draft elsewhere. At "worst", multiple APIs would be released at the same time, but changes shouldn't happen in between, or something is seriously wrong in how OGC versions standards otherwise...

The problem we face is mostly when we have many drafts working in parallel, and we don't notice they are broken because we just didn't happen to sync them for a while. You actually create a fake sense of "things working together", when they are continually broken until someone points it out.

[fmigneault]

Is this the basic DRU response when you deploy a new process?

Yes

The inputs and outputs description should be included in the response[...]

I think the original idea was to keep everything related to "resolve the full process description" by doing an explicit GET /processes/{processId}, rather than certain immutable processes relying on that endpoint while mutable ones using the POST /processes response. In as sense of "single source of truth", always rely on /processes/{processId}.

Indeed, you could tell the client right away all the details to avoid the extra request, but you have no guarantee it will. There could be some extra parsing things happening between those 2 requests that make the result slightly different.

[jerstlouis]

@fmigneault I changed this PR to add the rProcessSummary btw. The implementations can still return the inputs or outputs if they like. If we really want to consider the use case of them being optionally included (which I think we should), we should go back to directly referencing the processDescription that includes these (optional) members.

[fmigneault]

To stay in line with what was developed so far and tested during the code sprint, it's indeed better to leave summary.
Implementations are not forbidden to return more if desired.

[fmigneault]

PR works as it is, but curious to see if the updated CWL schema can work as it is from the source so that it is only an exact local copy for bundling.

[fmigneault]

https://www.commonwl.org/v1.2/cwl-json-schema.yaml also includes this, so it should be self-contained and employable as-is.

[jerstlouis]

@fmigneault Things like $comment are not valid and will generate errors.

[jerstlouis]

(EDIT: please disregards this comment, I was probably wrong)
Also this is not valid:

        enum:
          - WorkReuse

as this is a string WorkReuse needs to be quoted

[jerstlouis]

    additionalProperties: {}

Those things that I mentioned were a source of errors are still there.

(I'm looking at https://www.commonwl.org/v1.2/cwl-json-schema.yaml )

[jerstlouis]

And all the if and then stuff which was the main issue.

[fmigneault]

Ok, that's a shame.

Extra fields seem like a swaggercli problem since unknown fields should just be ignored (that is part of OAS30 as well). Not sure to understand the issue about the enum. That is definitely a parsing error from the implementation. It is valid YAML (it is generated from a yaml.safe_dump call).

Indeed, not much can be done about additionalProperties and if/then.

Thanks for checking. Let's use the workaround schema for now and review once OAS3.1 gets used (hopefully we ditch 3.0 soon to avoid this long-overdue problem).

[jerstlouis]

@fmigneault I don't think this is a SwaggerCLI problem, and I don't think this YAML JSON schema itself conforms to OAS 3.0 specifications. Most or all of the errors I mentioned were with the latest Swagger Editor and Swagger UI validation engines.

Possibly the Redoc tool automatically and silently convert to OAS 3.0 compliant definition while it is bundling, but I think that if we provide OAS 3.0 building blocks, the building blocks themselves should already comply to OAS 3. However, I seem to recall warnings and errors with this even while using the Redoc tool.

Extra fields seem like a swaggercli problem since unknown fields should just be ignored (that is part of OAS30 as well)

OAS 3.0 says in Section 4.8 Specification Extensions:

The field name MUST begin with x-, for example, x-internal-id.

So any strict validation reports that as invalid.
Other than those unknown fields ($schema, $author, $id, $comment, summary, exclusiveMinimum), the OAS3.0 issues are mostly the different interpretations of those additionalProperties, the unsupported if/then and the cwltool:CUDARequirement anchor in $defs.

Not sure to understand the issue about the enum.
That is definitely a parsing error from the implementation.

Sorry I might have been confusing this with something else.
I might have made this explicit quotes change here trying to fix another error.

hopefully we ditch 3.0 soon to avoid this long-overdue problem

I hope we don't "ditch 3.0" and keep this OAS 3.0 definition alive and working, even if we do end up adding an extra openapi3.1/ that could directly reference any valid JSON Schema.

Let's use the workaround schema for now

Sounds good, thanks!