2022-03-08 12:48:21 +03:00
|
|
|
import collections
|
2021-10-06 10:15:14 +03:00
|
|
|
import pytest
|
|
|
|
import os
|
|
|
|
from validate import check_query_f, check_query, get_conf_f
|
|
|
|
from context import PytestConf
|
|
|
|
|
|
|
|
@pytest.mark.parametrize("transport", ['http'])
|
Rewrite OpenAPI
### Description
This PR rewrites OpenAPI to be more idiomatic. Some noteworthy changes:
- we accumulate all required information during the Analyze phase, to avoid having to do a single lookup in the schema cache during the OpenAPI generation phase (we now only need the schema cache as input to run the analysis)
- we no longer build intermediary endpoint information and aggregate it, we directly build the the `PathItem` for each endpoint; additionally, that means we no longer have to assume that different methods have the same metadata
- we no longer have to first declare types, then craft references: we do everything in one step
- we now properly deal with nullability by treating "typeName" and "typeName!" as different
- we add a bunch of additional fields in the generated "schema", such as title
- we do now support enum values in both input and output positions
- checking whether the request body is required is now performed on the fly rather than by introspecting the generated schema
- the methods in the file are sorted by topic
### Controversial point
However, this PR creates some additional complexity, that we might not want to keep. The main complexity is _knot-tying_: to avoid lookups when generating the OpenAPI, it builds an actual graph of input types, which means that we need something similar to (but simpler than) `MonadSchema`, to avoid infinite recursions when analyzing the input types of a query. To do this, this PR introduces `CircularT`, a lesser `SchemaT` that aims at avoiding ever having to reinvent this particular wheel ever again.
### Remaining work
- [x] fix existing tests (they are all failing due to some of the schema changes)
- [ ] add tests to cover the new features:
- [x] tests for `CircularT`
- [ ] tests for enums in output schemas
- [x] extract / document `CircularT` if we wish to keep it
- [x] add more comments to `OpenAPI`
- [x] have a second look at `buildVariableSchema`
- [x] fix all missing diagnostics in `Analyze`
- [x] add a Changelog entry?
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/4654
Co-authored-by: David Overton <7734777+dmoverton@users.noreply.github.com>
GitOrigin-RevId: f4a9191f22dfcc1dccefd6a52f5c586b6ad17172
2022-06-30 15:55:56 +03:00
|
|
|
@pytest.mark.usefixtures('per_method_tests_db_state')
|
2021-10-06 10:15:14 +03:00
|
|
|
class TestOpenAPISpec:
|
|
|
|
|
|
|
|
@classmethod
|
|
|
|
def dir(cls):
|
|
|
|
return 'queries/openapi'
|
|
|
|
|
|
|
|
def test_empty_openapi_json(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_empty.yaml', transport)
|
2021-10-14 13:31:21 +03:00
|
|
|
|
2021-10-06 10:15:14 +03:00
|
|
|
def test_endpoint_simple(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_get_endpoint_test_simple.yaml', transport)
|
2021-10-14 13:31:21 +03:00
|
|
|
|
2022-01-27 08:54:51 +03:00
|
|
|
def test_endpoint_with_aliases(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_endpoint_with_aliases.yaml', transport)
|
|
|
|
|
2021-10-06 10:15:14 +03:00
|
|
|
def test_endpoint_with_args(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_post_endpoint_test_with_args.yaml', transport)
|
2021-10-14 13:31:21 +03:00
|
|
|
|
2021-10-06 10:15:14 +03:00
|
|
|
def test_endpoint_with_args_url(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_post_endpoint_test_with_args_url.yaml', transport)
|
|
|
|
|
|
|
|
def test_endpoint_with_default_arg(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_post_endpoint_test_with_default_arg.yaml', transport)
|
2021-10-14 13:31:21 +03:00
|
|
|
|
2021-10-06 10:15:14 +03:00
|
|
|
def test_endpoint_with_multiple_methods(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_endpoint_with_multiple_methods.yaml', transport)
|
|
|
|
|
|
|
|
def test_multiple_endpoints(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_multiple_endpoints_test.yaml', transport)
|
2021-10-14 13:31:21 +03:00
|
|
|
|
|
|
|
def test_multiple_endpoints_same_path(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_multiple_endpoints_same_path.yaml', transport)
|
|
|
|
|
2022-01-25 09:27:49 +03:00
|
|
|
def test_multiple_endpoints_with_path_segments(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_multiple_endpoints_with_path_segments.yaml', transport)
|
|
|
|
|
2021-12-22 11:30:15 +03:00
|
|
|
def test_endpoint_with_complex_arg(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_get_endpoint_test_complex_arg.yaml', transport)
|
|
|
|
|
|
|
|
def test_endpoint_with_complex_args(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_get_endpoint_test_complex_args.yaml', transport)
|
|
|
|
|
|
|
|
def test_endpoint_with_recursive_arg(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_post_endpoint_test_recursive_arg.yaml', transport)
|
2022-01-21 08:39:08 +03:00
|
|
|
|
|
|
|
def test_duplicate_field_name(self, hge_ctx, transport):
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_get_endpoint_test_duplicate_field_name.yaml', transport)
|
2022-03-08 12:48:21 +03:00
|
|
|
|
|
|
|
def test_inconsistent_schema_openAPI(self, hge_ctx, transport):
|
|
|
|
# export metadata and create a backup
|
2022-07-05 21:00:08 +03:00
|
|
|
backup_metadata = hge_ctx.v1q(
|
2022-03-08 12:48:21 +03:00
|
|
|
q = {
|
|
|
|
"type": "export_metadata",
|
|
|
|
"args": {}
|
|
|
|
}
|
|
|
|
)
|
|
|
|
|
|
|
|
new_metadata = backup_metadata.copy()
|
|
|
|
|
|
|
|
#create inconsistent metadata
|
|
|
|
inconsistent_query = [collections.OrderedDict([
|
Rewrite `GraphQL.Analysis`
### Motivation
While we strive to write clear code, we have historically struggled at Hasura with having very different styles and standards across the codebase. There's been efforts to standardize our coding style, we have an official styleguide that isn't maintained as closely as it should... We still have some work in front of us.
However, in the last ~year or so, there's been a huge push towards incrementally improving the situation. As part of this we've been blocking PRs that don't add enough comments, or don't improve the files that they touch.
While looking at `Hasura.GraphQL.Analyse`, it became apparent that this file did not meet the engineering standards that I would expect to see addressed during a code review. Some ways in which I think it falls short:
- lack of documentation
- no clear distinction between public / internal components
- "unidiomatic" Haskell code (such as using `Either Result Error`)
While there's no problem with a file looking like this during development, those issues should have been caught at review time. The fact that they weren't indicates a problem in our process that we will need to address: code quality and maintainability is paramount, and we all need to do our part.
### Description
This PR rewrites all of `Hasura.GraphQL.Analyze`, and adapts `Hasura.Server.OpenAPI` accordingly where needed. I've attempted to clarify names and add documentation based on my understanding of the code, and to clean what was unused (such as field variables). I don't think this PR is good enough as is, and I welcome criticism where I got my comments wrong / am happy to help y'all add more.
This PR makes one small change in the way error messages are reported (and adjusts the corresponding test accordingly); each error message is now prefixed with the path within the selection set:
```
⚠️ $.test.foo.bar.baz.mizpelled: field 'mizpelled' not found in object 'Baz'
```
### Note
This PR is currently **on top of #3962**. You can preview the changes in isolation by [diffing the branches](https://github.com/hasura/graphql-engine-mono/compare/nicuveo/clean-rest-endpoint-inconsistency-check..nicuveo/rewrite-analysis).
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3963
Co-authored-by: paritosh-08 <85472423+paritosh-08@users.noreply.github.com>
GitOrigin-RevId: 5ec38e0e753f0c12096a350db0737658495e2f15
2022-04-04 08:53:59 +03:00
|
|
|
('name', 'wrong_queries'),
|
2022-03-08 12:48:21 +03:00
|
|
|
('definition', collections.OrderedDict([
|
|
|
|
('queries', [collections.OrderedDict([
|
Rewrite `GraphQL.Analysis`
### Motivation
While we strive to write clear code, we have historically struggled at Hasura with having very different styles and standards across the codebase. There's been efforts to standardize our coding style, we have an official styleguide that isn't maintained as closely as it should... We still have some work in front of us.
However, in the last ~year or so, there's been a huge push towards incrementally improving the situation. As part of this we've been blocking PRs that don't add enough comments, or don't improve the files that they touch.
While looking at `Hasura.GraphQL.Analyse`, it became apparent that this file did not meet the engineering standards that I would expect to see addressed during a code review. Some ways in which I think it falls short:
- lack of documentation
- no clear distinction between public / internal components
- "unidiomatic" Haskell code (such as using `Either Result Error`)
While there's no problem with a file looking like this during development, those issues should have been caught at review time. The fact that they weren't indicates a problem in our process that we will need to address: code quality and maintainability is paramount, and we all need to do our part.
### Description
This PR rewrites all of `Hasura.GraphQL.Analyze`, and adapts `Hasura.Server.OpenAPI` accordingly where needed. I've attempted to clarify names and add documentation based on my understanding of the code, and to clean what was unused (such as field variables). I don't think this PR is good enough as is, and I welcome criticism where I got my comments wrong / am happy to help y'all add more.
This PR makes one small change in the way error messages are reported (and adjusts the corresponding test accordingly); each error message is now prefixed with the path within the selection set:
```
⚠️ $.test.foo.bar.baz.mizpelled: field 'mizpelled' not found in object 'Baz'
```
### Note
This PR is currently **on top of #3962**. You can preview the changes in isolation by [diffing the branches](https://github.com/hasura/graphql-engine-mono/compare/nicuveo/clean-rest-endpoint-inconsistency-check..nicuveo/rewrite-analysis).
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3963
Co-authored-by: paritosh-08 <85472423+paritosh-08@users.noreply.github.com>
GitOrigin-RevId: 5ec38e0e753f0c12096a350db0737658495e2f15
2022-04-04 08:53:59 +03:00
|
|
|
('name', 'random_query'),
|
2022-03-08 12:48:21 +03:00
|
|
|
('query', 'query { random_field_name test_table { random_col_name } }')
|
|
|
|
])])
|
|
|
|
]))
|
|
|
|
])]
|
|
|
|
res_endpoint = [collections.OrderedDict([
|
|
|
|
("definition", collections.OrderedDict([
|
|
|
|
("query", collections.OrderedDict([
|
|
|
|
("collection_name", "wrong_queries"),
|
|
|
|
("query_name", "random_query")
|
|
|
|
]))
|
|
|
|
])),
|
|
|
|
("url", "some_url"),
|
|
|
|
("methods", ["GET"]),
|
|
|
|
("name", "wrong_endpoint"),
|
Rewrite `GraphQL.Analysis`
### Motivation
While we strive to write clear code, we have historically struggled at Hasura with having very different styles and standards across the codebase. There's been efforts to standardize our coding style, we have an official styleguide that isn't maintained as closely as it should... We still have some work in front of us.
However, in the last ~year or so, there's been a huge push towards incrementally improving the situation. As part of this we've been blocking PRs that don't add enough comments, or don't improve the files that they touch.
While looking at `Hasura.GraphQL.Analyse`, it became apparent that this file did not meet the engineering standards that I would expect to see addressed during a code review. Some ways in which I think it falls short:
- lack of documentation
- no clear distinction between public / internal components
- "unidiomatic" Haskell code (such as using `Either Result Error`)
While there's no problem with a file looking like this during development, those issues should have been caught at review time. The fact that they weren't indicates a problem in our process that we will need to address: code quality and maintainability is paramount, and we all need to do our part.
### Description
This PR rewrites all of `Hasura.GraphQL.Analyze`, and adapts `Hasura.Server.OpenAPI` accordingly where needed. I've attempted to clarify names and add documentation based on my understanding of the code, and to clean what was unused (such as field variables). I don't think this PR is good enough as is, and I welcome criticism where I got my comments wrong / am happy to help y'all add more.
This PR makes one small change in the way error messages are reported (and adjusts the corresponding test accordingly); each error message is now prefixed with the path within the selection set:
```
⚠️ $.test.foo.bar.baz.mizpelled: field 'mizpelled' not found in object 'Baz'
```
### Note
This PR is currently **on top of #3962**. You can preview the changes in isolation by [diffing the branches](https://github.com/hasura/graphql-engine-mono/compare/nicuveo/clean-rest-endpoint-inconsistency-check..nicuveo/rewrite-analysis).
PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3963
Co-authored-by: paritosh-08 <85472423+paritosh-08@users.noreply.github.com>
GitOrigin-RevId: 5ec38e0e753f0c12096a350db0737658495e2f15
2022-04-04 08:53:59 +03:00
|
|
|
("comment", None)
|
2022-03-08 12:48:21 +03:00
|
|
|
])]
|
|
|
|
new_metadata["query_collections"] = inconsistent_query
|
|
|
|
new_metadata["rest_endpoints"] = res_endpoint
|
|
|
|
|
|
|
|
# apply inconsistent metadata
|
2022-07-05 21:00:08 +03:00
|
|
|
hge_ctx.v1q(
|
2022-03-08 12:48:21 +03:00
|
|
|
q={
|
|
|
|
"type": "replace_metadata",
|
|
|
|
"version": 2,
|
|
|
|
"args": {
|
|
|
|
"allow_inconsistent_metadata": True,
|
|
|
|
"metadata": new_metadata
|
|
|
|
}
|
|
|
|
}
|
|
|
|
)
|
|
|
|
|
|
|
|
# check openAPI schema
|
|
|
|
check_query_f(hge_ctx, self.dir() + '/openapi_inconsistent_schema.yaml', transport)
|
|
|
|
|
|
|
|
# revert to old metadata
|
2022-07-05 21:00:08 +03:00
|
|
|
hge_ctx.v1q(
|
2022-03-08 12:48:21 +03:00
|
|
|
q={
|
|
|
|
"type": "replace_metadata",
|
|
|
|
"args": backup_metadata
|
|
|
|
}
|
|
|
|
)
|