• 
      

    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