Experimantal GraphQL Endpoint
Review Request #12036 — Created Feb. 4, 2022 and discarded
Add an experimental GraphQL API endpoint.
This change introduces a new endpoint at
/graphql
, which handles GraphQL requests. The endpoint accepts POST requests with the body containing a valid GraphQL query. The schema for the API is generated by functions that map existingWebAPIResource
's into GraphQL types. This change also includes the query resolvers, which are called by thepy-gql
functiongraphql_blocking
, that query, filter, and return data from the database. Currently, only theUser
andReviewRequest
objects have been defined in the schema and have resolvers.The filtering logic in
resolve_review_requests
uses essentially the same logic as the filtering in thereview_request
resource, with some slight changes due to the difference in GrapgQL schema and REST query parameters. Ideally, this code would be refactored and DRY'ed out. Additionally, when building the schema we may eventually want to define theResourceList
andResource
types as a nested type, to take full advantage of the technology. For now, these are typed as objects withhref
,method
, andtitle
as they currently are in the other API resources.
Added unit tests for
GraphQLResource
and theUser
,Users
,ReviewRequest
, andReviewRequests
resolvers.
Ran allreviewboard
tests.
Summary | ID |
---|---|
40a838e2c8bca9927c3aceeb8b0025031ca0f4e2 |
Description | From | Last Updated |
---|---|---|
E501 line too long (88 > 79 characters) |
reviewbot | |
E501 line too long (134 > 79 characters) |
reviewbot | |
E501 line too long (91 > 79 characters) |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot | |
E124 closing bracket does not match visual indentation |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E222 multiple spaces after operator |
reviewbot | |
E222 multiple spaces after operator |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (100 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (116 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot | |
W292 no newline at end of file |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E261 at least two spaces before inline comment |
reviewbot | |
E501 line too long (101 > 79 characters) |
reviewbot | |
E221 multiple spaces before operator |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E225 missing whitespace around operator |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E124 closing bracket does not match visual indentation |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E222 multiple spaces after operator |
reviewbot | |
E222 multiple spaces after operator |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (100 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (116 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot | |
W292 no newline at end of file |
reviewbot | |
E221 multiple spaces before operator |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E124 closing bracket does not match visual indentation |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (100 > 79 characters) |
reviewbot | |
E501 line too long (116 > 79 characters) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E501 line too long (82 > 79 characters) |
reviewbot | |
E225 missing whitespace around operator |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (96 > 79 characters) |
reviewbot | |
E122 continuation line missing indentation or outdented |
reviewbot | |
W292 no newline at end of file |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E305 expected 2 blank lines after class or function definition, found 1 |
reviewbot | |
E124 closing bracket does not match visual indentation |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E501 line too long (82 > 79 characters) |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E201 whitespace after '{' |
reviewbot | |
E202 whitespace before '}' |
reviewbot | |
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E231 missing whitespace after ':' |
reviewbot | |
E501 line too long (120 > 79 characters) |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E201 whitespace after '{' |
reviewbot | |
E202 whitespace before '}' |
reviewbot | |
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E201 whitespace after '{' |
reviewbot | |
E202 whitespace before '}' |
reviewbot | |
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
We'll need to add a dependency for this to reviewboard/dependencies.py |
david | |
variable names should always start lower-case. Here and elsewhere. |
david | |
For normal strings we prefer single quotes. The only times we use double quotes are docstrings/multiline strings, or if the … |
david | |
type is a builtin. We should use a different name for this. In python, it's also much more efficient to … |
david | |
E999 SyntaxError: invalid syntax |
reviewbot | |
Can be elif type in ('Choice', 'List'): |
david | |
We should build an array and then use '\n'.join(...) instead of concatenations. |
david | |
None of these are actually format strings. |
david | |
Using the f-string syntax here is confusing (it's not entirely clear what is schema syntax and what is format syntax. … |
david | |
This seems very inefficient. Is there some built-in pagination structure to py_gql? |
david | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E501 line too long (129 > 79 characters) |
reviewbot | |
E501 line too long (83 > 79 characters) |
reviewbot | |
E501 line too long (82 > 79 characters) |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E501 line too long (83 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
This needs a module docstring. |
david | |
These should be sorted into two groups: All "third party" imports (including djblets) All reviewboard imports. Please sort them alphabetically … |
david | |
This should be in the imperative mood ("Return" instead of "Returns"). |
david | |
This needs an explanation. |
david | |
Please change "String" to "str" |
david | |
I believe that blank line is not needed after function docstring. |
sheenaNg | |
Please add a blank line between these two. |
david | |
Please add a blank line between these two. |
david | |
E999 SyntaxError: invalid syntax |
reviewbot | |
Returns -> Return |
david | |
This needs an explanation. |
david | |
String -> str |
david | |
I believe that blank line is not needed after function docstring. |
sheenaNg | |
Blank line between these two. |
david | |
This is a little funky. How about we define a schema_fmt and then do schema = schema_fmt.format(...)? |
david | |
This wrapping is awkward. How about: ```python def resolve_users( _root, ctx, _info, query_params={ 'page': 0, ... }): |
david | |
Blank line in here. We also need entries for _root and _info, which at least explain why they're unused. |
david | |
We can use a list comprehension here: data = [ serializer.serialize_object(user, request) for user in query ] |
david | |
Same comments here as the docstring for resolve_users. |
david | |
Blank line between these two. |
david | |
Blank line between these two. |
david | |
Same comments here as the docstring for resolve_users. |
david | |
I think it would be easier to read if a blank line is added between these two lines. |
sheenaNg | |
I think it would be easier to read if a blank line is added between these two lines. |
sheenaNg | |
I think it would be easier to read if a blank line is added between these two lines. |
sheenaNg | |
I think it would be easier to read if a blank line is added between these two lines. |
sheenaNg | |
Same comments here as the docstring for resolve_users. |
david | |
graphql -> GraphQL |
david | |
Please add a docstring. |
david | |
Can you please add a blank line between them? |
sheenaNg | |
This needs a module docstring, and imports should be broken up into three, alphabetically-sorted groups: Standard library (json) 3rd party … |
david | |
E999 SyntaxError: invalid syntax |
reviewbot | |
graphql -> GraphQL |
david | |
This decorator takes any "<URL>" in the docstring and replaces it with the value of sample_api_url. Since we're not listing … |
david | |
I think a blank line is not needed here. |
sheenaNg | |
Don't need this decorator. |
david | |
Since you're using the @webapi_test_template, perhaps do something like """Testing the <URL> API for a specific user and url serialization"""? … |
david | |
Let's rewrap: self.assertEqual( rsp['user']['links'], { 'self': { 'href': '...', 'method': 'GET', }, }) |
david | |
I think it would be easier to read if a blank line is added before calling the API endpoint. This … |
sheenaNg | |
I wonder why you did not assign the returned data to the variable "rsp" and check the "stat" and "error" … |
sheenaNg | |
A bunch of the test cases in here don't have docstrings. |
david | |
A blank line is not needed here. |
sheenaNg | |
Again, I wonder why you did not assign the returned data to the variable "rsp" and add assertions to check … |
sheenaNg | |
A blank line is not needed here. |
sheenaNg | |
Can you please add a blank line between these two just to be consistent with the other unit tests? |
sheenaNg | |
A blank line is not needed here. |
sheenaNg | |
A blank line is not needed here. |
sheenaNg | |
A blank line is not needed here. |
sheenaNg | |
Missing docstring that describes the test? |
sheenaNg | |
W291 trailing whitespace |
reviewbot | |
A blank line is not needed here. |
sheenaNg | |
Missing docstring that describes the test. |
sheenaNg | |
Should be "GraphQL" |
david | |
This stuff shouldn't be part of your change. It looks like you (or one of your tools) did a git … |
david | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot | |
E999 SyntaxError: invalid syntax |
reviewbot |
- Change Summary:
-
- implementing a better way to leverage existing WebAPIResource definitions to create graphql schema
- Summary:
-
Experimantal GraphQL Endpoint[WIP] Experimantal GraphQL Endpoint
- Commits:
-
Summary ID e9f1bae53b13c5fe1781fcf79ede9d35b49ba969 190da7a4bbff2a5d1737843a684c1233a9e63941
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID 190da7a4bbff2a5d1737843a684c1233a9e63941 4e40484e77b69ac1ff5391b81111f05eea058b45
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID 4e40484e77b69ac1ff5391b81111f05eea058b45 f834e918c4ebe05055b7349c21edecceaf6911d1
Checks run (1 failed, 1 succeeded)
flake8
-
Warning: Showing 30 of 31 failures.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
- fix flake8 errors
- Commits:
-
Summary ID f834e918c4ebe05055b7349c21edecceaf6911d1 e108bff9cf59fb1eb6b3cad9398f5cc9078ad431
-
-
-
-
For normal strings we prefer single quotes. The only times we use double quotes are docstrings/multiline strings, or if the string itself has a single quote in it.
-
type
is a builtin. We should use a different name for this.In python, it's also much more efficient to build a list of strings and then use something like
'\n'.join(schema_parts)
than to do a bunch of repeated concatenations. -
-
-
-
Using the f-string syntax here is confusing (it's not entirely clear what is schema syntax and what is format syntax. I think it'd be better to use .format syntax in here.
-
- Change Summary:
-
- addressing feedback
- Commits:
-
Summary ID e108bff9cf59fb1eb6b3cad9398f5cc9078ad431 a168dd304ec1e1b0a62e090e342d6cfab58b6891
- Commits:
-
Summary ID a168dd304ec1e1b0a62e090e342d6cfab58b6891 898fc97862dbc9651e1c8018d0d367ac8e81969d
- Commits:
-
Summary ID 898fc97862dbc9651e1c8018d0d367ac8e81969d 46378a7521ac685e9a4eb32c1988b8adfd787bb5
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID 46378a7521ac685e9a4eb32c1988b8adfd787bb5 6b5b73f8edb805f90ed680fe53960a2df323b896
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID 6b5b73f8edb805f90ed680fe53960a2df323b896 48c25be4c8f3d6ffef54bdc444c1b902fdb5d8b6
- Commits:
-
Summary ID 48c25be4c8f3d6ffef54bdc444c1b902fdb5d8b6 ac6505249a885ae5b5de69089074592bc677c926 - Diff:
-
Revision 11 (+2764 -26)
-
We're in the home stretch here. I added a bunch of comments which are primarily style things, but if you have to choose where to spend your time, the most important thing is filling out the "Description" field to explain what this does, how it works, and (especially important) what is left to do.
-
-
These should be sorted into two groups:
All "third party" imports (including djblets)
All reviewboard imports.Please sort them alphabetically within the groups.
-
-
-
-
-
-
-
-
-
-
This is a little funky. How about we define a
schema_fmt
and then doschema = schema_fmt.format(...)
? -
This wrapping is awkward. How about:
```python
def resolve_users(
_root,
ctx,
_info,
query_params={
'page': 0,
...
}): -
Blank line in here. We also need entries for
_root
and_info
, which at least explain why they're unused. -
We can use a list comprehension here:
data = [ serializer.serialize_object(user, request) for user in query ]
-
-
-
-
-
-
-
-
This needs a module docstring, and imports should be broken up into three, alphabetically-sorted groups:
- Standard library (json)
- 3rd party (django, djblets)
- Current module (reviewboard)
-
-
This decorator takes any "<URL>" in the docstring and replaces it with the value of
sample_api_url
. Since we're not listing URLs in these test cases, we don't need the decorator. -
-
Since you're using the
@webapi_test_template
, perhaps do something like """Testing the <URL> API for a specific user and url serialization"""? Please go through all the docstrings and either make it use "<URL>" or remove the template decorator. -
Let's rewrap:
self.assertEqual( rsp['user']['links'], { 'self': { 'href': '...', 'method': 'GET', }, })
-
-
-
This stuff shouldn't be part of your change. It looks like you (or one of your tools) did a
git fetch
. Can you rebase onto the latest master so your diff is clean?
- Commits:
-
Summary ID ac6505249a885ae5b5de69089074592bc677c926 14f6c2abc3b12ce214ff80e013b57ea4ebbffa7c - Diff:
-
Revision 12 (+2910)
-
-
-
-
-
-
-
-
-
-
I think it would be easier to read if a blank line is added before calling the API endpoint.
This change could be applied to the rest of the unit tests.
-
I wonder why you did not assign the returned data to the variable "rsp" and check the "stat" and "error" like you did to the other unit tests that you expected errors?
-
-
Again, I wonder why you did not assign the returned data to the variable "rsp" and add assertions to check the expected values for this unit test as well as the following few unit tests?
-
-
-
-
-
-
-
-
- Change Summary:
-
- addressing Sheena's review feedback
- Commits:
-
Summary ID 14f6c2abc3b12ce214ff80e013b57ea4ebbffa7c 0706eca329e2bdfde38dc66f31240d625d517ae4 - Diff:
-
Revision 13 (+2952)
Checks run (1 failed, 1 succeeded)
flake8
- Summary:
-
[WIP] Experimantal GraphQL EndpointExperimantal GraphQL Endpoint
- Description:
-
~ [WIP] Experimantal GraphQL Endpoint
~ Add an experimental GraphQL API endpoint.
+ + This change introduces a new endpoint at
/graphql
, which handles GraphQL requests. The endpoint accepts POST requests with the body containing a valid GraphQL query. The schema for the API is generated by functions that map existingWebAPIResource
's into GraphQL types. This change also includes the query resolvers, which are called by thepy-gql
functiongraphql_blocking
, that query, filter, and return data from the database. Currently, only theUser
andReviewRequest
objects have been defined in the schema and have resolvers.+ + The filtering logic in
resolve_review_requests
uses essentially the same logic as the filtering in thereview_request
resource, with some slight changes due to the difference in GrapgQL schema and REST query parameters. Ideally, this code would be refactored and DRY'ed out. Additionally, when building the schema we may eventually want to define theResourceList
andResource
types as a nested type, to take full advantage of the technology. For now, these are typed as objects withhref
,method
, andtitle
as they currently are in the other API resources. - Testing Done:
-
+ Added unit tests for
GraphQLResource
and theUser
,Users
,ReviewRequest
, andReviewRequests
resolvers.+ Ran all reviewboard
tests. - Commits:
-
Summary ID 0706eca329e2bdfde38dc66f31240d625d517ae4 40a838e2c8bca9927c3aceeb8b0025031ca0f4e2 - Depends On:
-
- Diff:
Revision 14 (+2980)