Open-EO / openeo-backend-validator

Service to validate back-end compliance with the API specification.
Apache License 2.0
1 stars 3 forks source link

Python: Only request supported endpoints #28

Closed m-mohr closed 5 years ago

m-mohr commented 5 years ago

The GET /udf_runtimes endpoint is requested for all back-end although they don't support it. Endpoints should only be requested if they claim to support it in the endpoints array in GET /.

Also, I'm not sure how valuable it is to test whether GET /invalid/path/to/nowhere exists. I'd understand to check something like GET /collection/collection-that-doesnt-exist, but I doubt we can back with the specification that a server must return valid responses for all possible subpaths that are not even defined in the API spec.

cc @soxofaan

soxofaan commented 5 years ago

Endpoints should only be requested if they claim to support it

good point, I'll add that

not sure how valuable it is to test whether GET /invalid/path/to/nowhere exists.

I added that for testing basic error handling as defined at https://open-eo.github.io/openeo-api/errors/ , the NotFound error code in this case ("requested resource does not exist"). The idea is that a backend implementation probably has a generic "not found" handler, and this is just a pragmatic test to see if "404 not found" returns in a format as specified by the spec.

And to come back to the previous point: if a backend does not support /udf_runtimes, it should return with FeatureUnsupported error, if I understand correctly?

m-mohr commented 5 years ago

good point, I'll add that

Great, thanks.

I added that for testing basic error handling as defined at https://open-eo.github.io/openeo-api/errors/ , the NotFound error code in this case ("requested resource does not exist"). The idea is that a backend implementation probably has a generic "not found" handler, and this is just a pragmatic test to see if "404 not found" returns in a format as specified by the spec.

Yes, in theory that would be good, but I doubt all will have such an error handler and I'm not sure how valuable it is to enforce it outside of what the specification defines. Giving a warning would be reasonable, but failing completely due to this rule feels to be too much, I think.

if a backend does not support /udf_runtimes, it should return with FeatureUnsupported error, if I understand correctly?

Indeed, but after the first experiences this was not a good idea and I'll revert this in the API for the next version (see https://github.com/Open-EO/openeo-api/commit/29494aa88d500d52eacc6b511851745dad00fa03).

soxofaan commented 5 years ago

not sure how valuable it is to enforce it outside of what the specification defines.

Maybe I'm understanding wrongly but "404 Not Found" is defined spec-wise in https://open-eo.github.io/openeo-api/errors/#general , right?

a warning would be reasonable, but failing completely due to this rule feels to be too much

This raises the question on how to assign severity levels to violations. This adds a whole new dimension to the problem: how to define the levels, how to detect them, how to report them ... ; which is probably a whole separate discussion :)

More practically from the standpoint of the pytest based scenario's: the evaluation of a test is a binary thing: you pass or you fail, there are not really levels in between. However, it could be possible to annotate tests and put them in categories, so that you could run e.g. a "core" test suite and a "full" test suite.

m-mohr commented 5 years ago

Yes, it is defined as "NotFound" there.

How to assign them is relatively easy, I think. Everything that is a required functionality (specified with RFC compliant words such as REQUIRED, MUST, SHALL, ...) must produce an error and all things that are specified as SHOULD, MAY, OPTIONAL, etc. should probably be warnings. Surely, this is nothing we need to solve now.

Indeed tests are binary and warnings should not let a test fail. ;-) They should still be shown to the user.

m-mohr commented 5 years ago

Thanks, main concern of the issue has been fixed. Closing.