Experimantal GraphQL Endpoint

Review Request #12036 — Created Feb. 4, 2022 and discarded

Information

Review Board
master

Reviewers

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 existing WebAPIResource's into GraphQL types. This change also includes the query resolvers, which are called by the py-gql function graphql_blocking, that query, filter, and return data from the database. Currently, only the User and ReviewRequest 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 the review_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 the ResourceList and Resource types as a nested type, to take full advantage of the technology. For now, these are typed as objects with href, method, and title as they currently are in the other API resources.

Added unit tests for GraphQLResource and the User, Users, ReviewRequest, and ReviewRequests resolvers.
Ran all reviewboard tests.

Summary ID
Experimantal GraphQL Endpoint
40a838e2c8bca9927c3aceeb8b0025031ca0f4e2
Description From Last Updated

E501 line too long (88 > 79 characters)

reviewbotreviewbot

E501 line too long (134 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E222 multiple spaces after operator

reviewbotreviewbot

E222 multiple spaces after operator

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (100 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (116 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E261 at least two spaces before inline comment

reviewbotreviewbot

E501 line too long (101 > 79 characters)

reviewbotreviewbot

E221 multiple spaces before operator

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E222 multiple spaces after operator

reviewbotreviewbot

E222 multiple spaces after operator

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (100 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (116 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E221 multiple spaces before operator

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (100 > 79 characters)

reviewbotreviewbot

E501 line too long (116 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (96 > 79 characters)

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E201 whitespace after '{'

reviewbotreviewbot

E202 whitespace before '}'

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E501 line too long (120 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E201 whitespace after '{'

reviewbotreviewbot

E202 whitespace before '}'

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E201 whitespace after '{'

reviewbotreviewbot

E202 whitespace before '}'

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

We'll need to add a dependency for this to reviewboard/dependencies.py

daviddavid

variable names should always start lower-case. Here and elsewhere.

daviddavid

For normal strings we prefer single quotes. The only times we use double quotes are docstrings/multiline strings, or if the …

daviddavid

type is a builtin. We should use a different name for this. In python, it's also much more efficient to …

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

Can be elif type in ('Choice', 'List'):

daviddavid

We should build an array and then use '\n'.join(...) instead of concatenations.

daviddavid

None of these are actually format strings.

daviddavid

Using the f-string syntax here is confusing (it's not entirely clear what is schema syntax and what is format syntax. …

daviddavid

This seems very inefficient. Is there some built-in pagination structure to py_gql?

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (129 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

This needs a module docstring.

daviddavid

These should be sorted into two groups: All "third party" imports (including djblets) All reviewboard imports. Please sort them alphabetically …

daviddavid

This should be in the imperative mood ("Return" instead of "Returns").

daviddavid

This needs an explanation.

daviddavid

Please change "String" to "str"

daviddavid

I believe that blank line is not needed after function docstring.

sheenaNgsheenaNg

Please add a blank line between these two.

daviddavid

Please add a blank line between these two.

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

Returns -> Return

daviddavid

This needs an explanation.

daviddavid

String -> str

daviddavid

I believe that blank line is not needed after function docstring.

sheenaNgsheenaNg

Blank line between these two.

daviddavid

This is a little funky. How about we define a schema_fmt and then do schema = schema_fmt.format(...)?

daviddavid

This wrapping is awkward. How about: ```python def resolve_users( _root, ctx, _info, query_params={ 'page': 0, ... }):

daviddavid

Blank line in here. We also need entries for _root and _info, which at least explain why they're unused.

daviddavid

We can use a list comprehension here: data = [ serializer.serialize_object(user, request) for user in query ]

daviddavid

Same comments here as the docstring for resolve_users.

daviddavid

Blank line between these two.

daviddavid

Blank line between these two.

daviddavid

Same comments here as the docstring for resolve_users.

daviddavid

I think it would be easier to read if a blank line is added between these two lines.

sheenaNgsheenaNg

I think it would be easier to read if a blank line is added between these two lines.

sheenaNgsheenaNg

I think it would be easier to read if a blank line is added between these two lines.

sheenaNgsheenaNg

I think it would be easier to read if a blank line is added between these two lines.

sheenaNgsheenaNg

Same comments here as the docstring for resolve_users.

daviddavid

graphql -> GraphQL

daviddavid

Please add a docstring.

daviddavid

Can you please add a blank line between them?

sheenaNgsheenaNg

This needs a module docstring, and imports should be broken up into three, alphabetically-sorted groups: Standard library (json) 3rd party …

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

graphql -> GraphQL

daviddavid

This decorator takes any "<URL>" in the docstring and replaces it with the value of sample_api_url. Since we're not listing …

daviddavid

I think a blank line is not needed here.

sheenaNgsheenaNg

Don't need this decorator.

daviddavid

Since you're using the @webapi_test_template, perhaps do something like """Testing the <URL> API for a specific user and url serialization"""? …

daviddavid

Let's rewrap: self.assertEqual( rsp['user']['links'], { 'self': { 'href': '...', 'method': 'GET', }, })

daviddavid

I think it would be easier to read if a blank line is added before calling the API endpoint. This …

sheenaNgsheenaNg

I wonder why you did not assign the returned data to the variable "rsp" and check the "stat" and "error" …

sheenaNgsheenaNg

A bunch of the test cases in here don't have docstrings.

daviddavid

A blank line is not needed here.

sheenaNgsheenaNg

Again, I wonder why you did not assign the returned data to the variable "rsp" and add assertions to check …

sheenaNgsheenaNg

A blank line is not needed here.

sheenaNgsheenaNg

Can you please add a blank line between these two just to be consistent with the other unit tests?

sheenaNgsheenaNg

A blank line is not needed here.

sheenaNgsheenaNg

A blank line is not needed here.

sheenaNgsheenaNg

A blank line is not needed here.

sheenaNgsheenaNg

Missing docstring that describes the test?

sheenaNgsheenaNg

W291 trailing whitespace

reviewbotreviewbot

A blank line is not needed here.

sheenaNgsheenaNg

Missing docstring that describes the test.

sheenaNgsheenaNg

Should be "GraphQL"

daviddavid

This stuff shouldn't be part of your change. It looks like you (or one of your tools) did a git …

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

DR
Review request changed
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
Experimantal GraphQL Endpoint
e9f1bae53b13c5fe1781fcf79ede9d35b49ba969
Experimantal GraphQL Endpoint
190da7a4bbff2a5d1737843a684c1233a9e63941

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
190da7a4bbff2a5d1737843a684c1233a9e63941
Experimantal GraphQL Endpoint
4e40484e77b69ac1ff5391b81111f05eea058b45

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
4e40484e77b69ac1ff5391b81111f05eea058b45
Experimantal GraphQL Endpoint
f834e918c4ebe05055b7349c21edecceaf6911d1

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Change Summary:
  • fix flake8 errors
Commits:
Summary ID
Experimantal GraphQL Endpoint
f834e918c4ebe05055b7349c21edecceaf6911d1
Experimantal GraphQL Endpoint
e108bff9cf59fb1eb6b3cad9398f5cc9078ad431

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    We'll need to add a dependency for this to reviewboard/dependencies.py

  3. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    variable names should always start lower-case. Here and elsewhere.

  4. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    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.

  5. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    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.

  6. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    Can be elif type in ('Choice', 'List'):

  7. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    We should build an array and then use '\n'.join(...) instead of concatenations.

  8. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
    Show all issues

    None of these are actually format strings.

  9. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
    Show all issues

    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.

  10. reviewboard/webapi/resources/graphql.py (Diff revision 5)
     
     
     
    Show all issues

    This seems very inefficient. Is there some built-in pagination structure to py_gql?

  11. 
      
DR
Review request changed
Change Summary:
  • addressing feedback
Commits:
Summary ID
Experimantal GraphQL Endpoint
e108bff9cf59fb1eb6b3cad9398f5cc9078ad431
Experimantal GraphQL Endpoint
a168dd304ec1e1b0a62e090e342d6cfab58b6891

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
a168dd304ec1e1b0a62e090e342d6cfab58b6891
Experimantal GraphQL Endpoint
898fc97862dbc9651e1c8018d0d367ac8e81969d

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
898fc97862dbc9651e1c8018d0d367ac8e81969d
Experimantal GraphQL Endpoint
46378a7521ac685e9a4eb32c1988b8adfd787bb5

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
46378a7521ac685e9a4eb32c1988b8adfd787bb5
Experimantal GraphQL Endpoint
6b5b73f8edb805f90ed680fe53960a2df323b896

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
6b5b73f8edb805f90ed680fe53960a2df323b896
Experimantal GraphQL Endpoint
48c25be4c8f3d6ffef54bdc444c1b902fdb5d8b6

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
48c25be4c8f3d6ffef54bdc444c1b902fdb5d8b6
Experimantal GraphQL Endpoint
ac6505249a885ae5b5de69089074592bc677c926

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 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.

  2. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    This needs a module docstring.

  3. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These should be sorted into two groups:

    All "third party" imports (including djblets)
    All reviewboard imports.

    Please sort them alphabetically within the groups.

  4. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    This should be in the imperative mood ("Return" instead of "Returns").

  5. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    This needs an explanation.

  6. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    Please change "String" to "str"

  7. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Please add a blank line between these two.

  8. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Please add a blank line between these two.

  9. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    Returns -> Return

  10. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    This needs an explanation.

  11. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    String -> str

  12. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Blank line between these two.

  13. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
     
    Show all issues

    This is a little funky. How about we define a schema_fmt and then do schema = schema_fmt.format(...)?

  14. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
    Show all issues

    This wrapping is awkward. How about:

    ```python
    def resolve_users(
    _root,
    ctx,
    _info,
    query_params={
    'page': 0,
    ...
    }):

  15. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Blank line in here. We also need entries for _root and _info, which at least explain why they're unused.

  16. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
    Show all issues

    We can use a list comprehension here:

    data = [
        serializer.serialize_object(user, request)
        for user in query
    ]
    
  17. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    Same comments here as the docstring for resolve_users.

  18. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Blank line between these two.

  19. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Blank line between these two.

  20. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
     
     
     
    Show all issues

    Same comments here as the docstring for resolve_users.

  21. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    Same comments here as the docstring for resolve_users.

  22. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    graphql -> GraphQL

  23. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    Please add a docstring.

  24. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This needs a module docstring, and imports should be broken up into three, alphabetically-sorted groups:

    1. Standard library (json)
    2. 3rd party (django, djblets)
    3. Current module (reviewboard)
  25. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    graphql -> GraphQL

  26. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    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.

  27. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    Don't need this decorator.

  28. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    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.

  29. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
     
     
    Show all issues

    Let's rewrap:

    self.assertEqual(
        rsp['user']['links'],
        {
            'self': {
                'href': '...',
                'method': 'GET',
            },
        })
    
  30. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A bunch of the test cases in here don't have docstrings.

  31. reviewboard/webapi/tests/urls.py (Diff revision 11)
     
     
    Show all issues

    Should be "GraphQL"

  32. setup.py (Diff revision 11)
     
     
     
     
     
    Show all issues

    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?

  33. 
      
DR
Review request changed
Commits:
Summary ID
Experimantal GraphQL Endpoint
ac6505249a885ae5b5de69089074592bc677c926
Experimantal GraphQL Endpoint
14f6c2abc3b12ce214ff80e013b57ea4ebbffa7c

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sheenaNg
  1. 
      
    1. I think you did a good job in creating the experimental GraphQL endpoint for the project! I learned quite a bit about GraphQL looking at your codes and the unit tests :) I just added some minor suggestions regarding the formatting issues to improve the styling consistency.

    2. Thanks for the review, Sheena!

  2. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    I believe that blank line is not needed after function docstring.

  3. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
    Show all issues

    I believe that blank line is not needed after function docstring.

  4. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    I think it would be easier to read if a blank line is added between these two lines.

  5. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    I think it would be easier to read if a blank line is added between these two lines.

  6. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    I think it would be easier to read if a blank line is added between these two lines.

  7. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    I think it would be easier to read if a blank line is added between these two lines.

  8. reviewboard/webapi/resources/graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Can you please add a blank line between them?

  9. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    I think a blank line is not needed here.

  10. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
     
    Show all issues

    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.

  11. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    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?

  12. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A blank line is not needed here.

  13. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    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?

    1. I think I must've just forgotten to go back and and add the assertions on these ones. Nice catch!

  14. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A blank line is not needed here.

  15. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
     
    Show all issues

    Can you please add a blank line between these two just to be consistent with the other unit tests?

  16. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A blank line is not needed here.

  17. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A blank line is not needed here.

  18. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A blank line is not needed here.

  19. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    Missing docstring that describes the test?

  20. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    A blank line is not needed here.

  21. reviewboard/webapi/tests/test_graphql.py (Diff revision 11)
     
     
    Show all issues

    Missing docstring that describes the test.

  22. 
      
DR
Review request changed
Change Summary:
  • addressing Sheena's review feedback
Commits:
Summary ID
Experimantal GraphQL Endpoint
14f6c2abc3b12ce214ff80e013b57ea4ebbffa7c
Experimantal GraphQL Endpoint
0706eca329e2bdfde38dc66f31240d625d517ae4

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

DR
Review request changed
Summary:
[WIP] Experimantal GraphQL Endpoint
Experimantal 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 existing WebAPIResource's into GraphQL types. This change also includes the query resolvers, which are called by the py-gql function graphql_blocking, that query, filter, and return data from the database. Currently, only the User and ReviewRequest 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 the review_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 the ResourceList and Resource types as a nested type, to take full advantage of the technology. For now, these are typed as objects with href, method, and title as they currently are in the other API resources.

Testing Done:
  +

Added unit tests for GraphQLResource and the User, Users, ReviewRequest, and ReviewRequests resolvers.

  + Ran all reviewboard tests.

Commits:
Summary ID
Experimantal GraphQL Endpoint
0706eca329e2bdfde38dc66f31240d625d517ae4
Experimantal GraphQL Endpoint
40a838e2c8bca9927c3aceeb8b0025031ca0f4e2

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed
Status:
Discarded