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

Null values in Axes, properties, x, anyOf, type #8

Open
ways opened this issue Jan 15, 2024 · 19 comments
Open

Null values in Axes, properties, x, anyOf, type #8

ways opened this issue Jan 15, 2024 · 19 comments

Comments

@ways
Copy link
Contributor

ways commented Jan 15, 2024

Hi

Please se my openapi.json. The relevant code generating this is around this line https://github.com/metno/edrisobaric/blob/f344ca8a414e75b52726a2d727737144bddac8f4/app/routes/position_page.py#L111.

These null values cause trouble in EDR validation and API code generation (https://github.com/OpenAPITools/openapi-generator).

I'm unsure why the null appears. X should not be an optional value. Have you seen the problem in your use, or am I using the library wrong?

@lukas-phaf
Copy link
Collaborator

How I read the spec, all axes are optional, including the X-axis:
https://docs.ogc.org/cs/21-069r2/21-069r2.html#_d5c16418-1a20-4dbf-bf7a-8e685062df97

In the Common Domain Type section, X and Y are not used for a bunch of the supported domain types:
https://docs.ogc.org/cs/21-069r2/21-069r2.html#_2ebca2a8-cdde-41b4-ad83-2ede16d3792e

The null in the openapi file appears because of this:
https://github.com/KNMI/covjson-pydantic/blob/master/src/covjson_pydantic/domain.py#L59

We use this approach (instead of just a dictionary) to have control of what the difference axis types can contain (so that we can enforce dates in the "t" axis, etc).

I don't see how this strictly violates the spec, as we are just saying that these axis are possible, and if they are given (not-null), they not to comply with some requirements. I can see why this might not be what the EDR validator expects to see in the spec file. But as the produced JSON complies with the CoverageJSON specification, maybe it is fair to say the validator is to strict?

We haven't tried EDR validation against our code, and we don't use API code generation, so we have not seen this problem ourselves.

What is the error the validator is giving?

@ways
Copy link
Contributor Author

ways commented Jan 15, 2024

Validator

The official validator is at https://cite.opengeospatial.org/teamengine/about/ogcapi-edr10/1.0/site/index.html

We have a dockerized version (which I hope can be published, instead of having to compile that java project) which gives this output for my API. See attachments.
validator.zip

A summary of the errors:

API definition is not valid. Found following validation items: @ http://localhost:5000/openapi.json[1:5297-1:5303] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/openapi.json[1:5435-1:5441]

@ways
Copy link
Contributor Author

ways commented Jan 15, 2024

API code generation

I won't be using any code generation myself, but users of the API might. Therefore I think it's important that it works.

For generating I did this (below). The "/local/" in out is correct though it seems wrong. Files will end up in sub-dir of current dir, as out/go.

docker pull openapitools/openapi-generator-cli
docker run --network=host --rm -v "${PWD}:/local" openapitools/openapi-generator-cli generate \
    -i http://localhost:5000/openapi.json \
    -g go \
    -o /local/out/go

Then to compile:

cd out/go
go build
# github.com/GIT_USER_ID/GIT_REPO_ID
./model_values_axis_float_.go:24:14: undefined: NullableAnyOf
./model_values_axis_float_.go:26:9: undefined: NullableAnyOf
./model_values_axis_aware_datetime_.go:25:14: undefined: NullableAnyOf
./model_values_axis_aware_datetime_.go:27:9: undefined: NullableAnyOf
./model_values_axis_tuple_.go:24:14: undefined: NullableAnyOf
./model_values_axis_tuple_.go:26:9: undefined: NullableAnyOf
./model_collection.go:26:11: undefined: NullableAnyOf
./model_corridor_variables.go:24:16: undefined: NullableAnyOf
./model_cube_variables.go:24:16: undefined: NullableAnyOf
./model_item_variables.go:24:16: undefined: NullableAnyOf
./model_item_variables.go:24:16: too many errors

This looks like the same problem to me, though I haven't investigated closely.

@ways
Copy link
Contributor Author

ways commented Jan 15, 2024

The null in the openapi file appears because of this: https://github.com/KNMI/covjson-pydantic/blob/master/src/covjson_pydantic/domain.py#L59

So to work around this do I have to sub-class Axis without a default value? I guess then I also have to sub-class Domain to use my "new" Axis. Are there less dirty ways of doing this?

@lukas-phaf
Copy link
Collaborator

lukas-phaf commented Jan 15, 2024

Thanks for looking into this and the extra info.

How do you know the error (http://localhost:5000/openapi.json[1:5297-1:5303] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' ) refers to the Axis?

The openapi.json you point to in your first post has less than 3000 lines (so no line 5297), and the required pattern in the error message does not look appropriate for the Axis value.

@lukas-phaf
Copy link
Collaborator

We have a dockerized version (which I hope can be published, instead of having to compile that java project)

That would be really nice!

@lukas-phaf
Copy link
Collaborator

cd out/go
go build
# github.com/GIT_USER_ID/GIT_REPO_ID
./model_values_axis_float_.go:24:14: undefined: NullableAnyOf
./model_values_axis_float_.go:26:9: undefined: NullableAnyOf
./model_values_axis_aware_datetime_.go:25:14: undefined: NullableAnyOf
./model_values_axis_aware_datetime_.go:27:9: undefined: NullableAnyOf
./model_values_axis_tuple_.go:24:14: undefined: NullableAnyOf
./model_values_axis_tuple_.go:26:9: undefined: NullableAnyOf
./model_collection.go:26:11: undefined: NullableAnyOf
./model_corridor_variables.go:24:16: undefined: NullableAnyOf
./model_cube_variables.go:24:16: undefined: NullableAnyOf
./model_item_variables.go:24:16: undefined: NullableAnyOf
./model_item_variables.go:24:16: too many errors

This looks broader then just the Axis, even broader then the CoverageJSON package. For example, the reference to cube_variables.

@ways
Copy link
Contributor Author

ways commented Jan 16, 2024

Thanks for looking into this and the extra info.

How do you know the error (http://localhost:5000/openapi.json[1:5297-1:5303] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' ) refers to the Axis?

The openapi.json you point to in your first post has less than 3000 lines (so no line 5297), and the required pattern in the error message does not look appropriate for the Axis value.

The json file is actually unformatted, all on one line. 1:5297 refers to line 1, character 5297. It matches the first null value, and there's a repeated message for each null in the file.

@lukas-phaf
Copy link
Collaborator

Ah yes, missed that it is all referring to line 1.
Do you happen to have the unformatted json file so I can take a look?

@ways
Copy link
Contributor Author

ways commented Jan 16, 2024

Raw file from latest version: https://gist.github.com/ways/7acff3af6fc404a283096f5fa5afa2a7

And start of errors for that version:

API definition is not valid. Found following validation items: @ http://localhost:5000/api[1:5515-1:5521] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:5653-1:5659] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:5791-1:5797] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:5893-1:5899] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:5983-1:5989] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6163-1:6169] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6239-1:6245] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6343-1:6349] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6583-1:6589] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6660-1:6666] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6762-1:6768] - ERROR: Value 'null' does not match required pattern 'boolean|object|array|number|integer|string' @ http://localhost:5000/api[1:6956-1:6962] - ERROR: Value 'null' does not match required pattern

@lukas-phaf
Copy link
Collaborator

Thanks for sending that. It looks like anywhere we have an optional type in Pydantic, we get this anyOf with type null in the spec. This was introduced in Pydantic v2. This is definitely not pretty...

There are several issues about this, for example:
fastapi/fastapi#9900

I am looking if there is a nice way to fix this.

@lukas-phaf
Copy link
Collaborator

I found a way to fix this, by explicitly dropping the null type, and also the default value.
I put the code here:
#11

Strictly speaking, the schema generated is now not compatible with the pydantic type, but it does produce an openapi spec that looks more like what people expect. So I am not sure what direction we want to go with this yet, as the openapi spec that is currently generated is correct, but uses a new feature that is not yet supported by a lot of tools.

But it would be nice to know if this at least solves the problems you indicated. Would you be able to test that?
Keep in mind that there will be other places where this problem occurs, both in edr-pydantic and probably places in your own code. Basically everywhere an optional value is used which defaults to None.
So the question is, does this solve the issues (EDR validation and code generation) for the types generated by covjson-pydantic?

@ways
Copy link
Contributor Author

ways commented Jan 18, 2024

Your PR does not break my code and removes null in the spec. The validator seems happy, but of course complains on nulls from edr_pydantic.

Openapi.json generated with this pr: https://gist.github.com/ways/3207b68e32443d77e268501db8cdb9a2

I've also opened a bug report for the validator, in case that is where the problem really should be fixed. opengeospatial/ets-ogcapi-edr10#113

@ways
Copy link
Contributor Author

ways commented Jan 30, 2024

Seems null is allowed in openapi 3.1, but not in openapi 3.0.

Reference: https://redocly.com/docs/openapi-visual-reference/null/

@lukas-phaf
Copy link
Collaborator

Yes, that explains why tooling (like EDR validator and code generators) fail on this, they haven't updated to support the latest spec.
The openapi spec generated does indicated it is openapi 3.1.0, so technically everything is correct.

We are a bit unclear on how to proceed. The current generated openapi more correctly describes the API and it is correct according to the openapi 3.1 standard. On the other hand, it breaks current tooling, but I guess that will be updated.

The "fix" is a nasty hack, and generates an openapi spec document that is not consistent with what the API will accept. But external tooling works better with it.

Can we just wait until the tooling is updated? What do you think?

@lukas-phaf
Copy link
Collaborator

The validator ticket is now marked as "in progress"...
opengeospatial/ets-ogcapi-edr10#113

@ways
Copy link
Contributor Author

ways commented Jan 30, 2024

Yes, that explains why tooling (like EDR validator and code generators) fail on this, they haven't updated to support the latest spec. The openapi spec generated does indicated it is openapi 3.1.0, so technically everything is correct.

We are a bit unclear on how to proceed. The current generated openapi more correctly describes the API and it is correct according to the openapi 3.1 standard. On the other hand, it breaks current tooling, but I guess that will be updated.

The "fix" is a nasty hack, and generates an openapi spec document that is not consistent with what the API will accept. But external tooling works better with it.

Can we just wait until the tooling is updated? What do you think?

Yes, I agree, we can wait and see.

@lukas-phaf
Copy link
Collaborator

Does this actually solve the issue for you (at least the EDR validator part)?
opengeospatial/ets-ogcapi-edr10#115

It looks like any "null" errors are removed.

I am trying to build the validator package myself, by no luck so far.

@ways
Copy link
Contributor Author

ways commented Jun 12, 2024

Does this actually solve the issue for you (at least the EDR validator part)? opengeospatial/ets-ogcapi-edr10#115
It looks like any "null" errors are removed.

I will check, but I'm having trouble using the official validators UI. If I understand it correctly, it no longer complains on the null errors. 👍 I will check with the local validator expert when he's available.

I am trying to build the validator package myself, by no luck so far.

Seems you can just pull it from https://hub.docker.com/r/ogccite/ets-ogcapi-edr10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants