[WIP]Allow to query Releases using GraphQL #4036

karmadolkar · 2020-05-25T08:12:36Z

ref #4024
Signed-off-by: Karma Dolkar karmadolkar29@gmail.com

lgtm-com · 2020-05-25T08:33:55Z

This pull request introduces 9 alerts when merging 3ca6391 into e0fcce5 - view on LGTM.com

new alerts:

6 for Unused import
2 for Module is imported with 'import' and 'import from'
1 for Unreachable code

StephenCoady

Great job so far! I left some small comments.

StephenCoady · 2020-05-25T13:44:55Z

bodhi/server/services/graphql.py

+@view_config()
+def graphql_view(request):
+    context = {'session': request.session}
+    return serve_graphql_request(request, schema, context_value=context)


this line can be deleted

StephenCoady · 2020-05-25T13:51:32Z

bodhi/server/services/graphql.py

+
+
+class Query(graphene.ObjectType):
+    release = graphene.List(Release)


if you call this allReleases or something similar then it will be more obvious to the user what this does

StephenCoady · 2020-05-25T14:30:00Z

bodhi/server/services/graphql.py

+    return serve_graphql_request(request, schema, context_value=context)
+
+    # Setting graphiql=True
+    return serve_graphql_request(request, schema, graphiql=True, context_value=context)


I also think the flag here should be graphiql_enabled=True.

It doesn't matter for now as the default is True but it will matter once we set this based on a variable :)

StephenCoady · 2020-05-25T14:32:52Z

Also, it looks like you have some unused imports in the schema and service file, so the CI will fail.

If you need to fix those, you can run flake8 . :)

cverna · 2020-06-01T08:33:41Z

bodhi/server/graphql_schemas.py

+
+class Release(SQLAlchemyObjectType):
+    """
+    Type object representing a distribution release, such as Fedora 27.


I think in the doc string here we should just reference that this is using the Release object from models.py so that we don't have to maintain the Attributes list and documentation here.

cverna · 2020-06-01T08:35:36Z

bodhi/server/services/graphql.py

 from cornice import Service
+from webob_graphql import serve_graphql_request
+import graphene


Let's move this import at the top of the import

Suggested change

from cornice import Service

from webob_graphql import serve_graphql_request

import graphene

import graphene

Suggested change

from cornice import Service

from webob_graphql import serve_graphql_request

import graphene

import graphene

from cornice import Service

from webob_graphql import serve_graphql_request

cverna · 2020-06-01T08:36:26Z

bodhi/server/services/graphql.py

+    Args:
+        request (pyramid.Request): The current request.
+    Returns:
+        The response from Bodhi to the request.


The graphql response to the query

cverna · 2020-06-01T08:36:47Z

bodhi/server/services/graphql.py

+    """
+    context = {'session': request.session}
+
+    # Setting graphiql_enabled=True


Let's remove this comment

cverna · 2020-06-01T08:38:07Z

bodhi/server/services/graphql.py

+    context = {'session': request.session}
+
+    # Setting graphiql_enabled=True
+    return serve_graphql_request(request, schema, graphiql_enabled=True, context_value=context)


I think we want to have the graphiql_enabled value to be configure in the configuration so that we can have it set to True in the development environment and False in production.

For adding this in the configuration you can look at an example here -->
import the configuration :
https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/models.py#L47
get the value of a configuration key:
https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/models.py#L1094
add the configuration key to the file :
https://github.com/fedora-infra/bodhi/blob/develop/production.ini#L322
and https://github.com/fedora-infra/bodhi/blob/develop/devel/development.ini.example

the configuration key could be something like enable_graphiql

cverna · 2020-06-01T08:40:05Z

bodhi/server/services/graphql.py

+
+
+@graphql.post()
+def graphql_post(request):


I am not 100% sure here (@StephenCoady might look at that), but I am not sure that we need 2 function here we could have only one with the following declaration

@graphql.post() @graphql.get() def graphql_endpoint(request): ....

This is correct, function with both post and get will work the same

cverna · 2020-06-01T08:44:34Z

We also need to update the tests 😄

cverna · 2020-06-01T08:47:36Z

devel/ansible/roles/bodhi/tasks/main.yml

@@ -109,6 +109,21 @@
      name: jinja2-cli
      executable: pip3

+- name: pip install WebOb-GraphQL


here you can use a loop in ansible instead of repeating the tasks 4 times. See https://docs.ansible.com/ansible/latest/user_guide/playbooks_loops.html#iterating-over-a-simple-list

cverna · 2020-06-01T09:28:43Z

When trying to use the endpoint with graphiql and using the following query

{
  allReleases {
    id
    name
    longName
  }
}

I don't get any releases in the response, I would have expected to see a json list of all the releases.

But I might be doing something wrong :-)

karmadolkar · 2020-06-01T16:04:13Z

I don't get any releases in the response, I would have expected to see a json list of all the releases.

Yes, I think a resolver function for each field is required to fetch the data.

StephenCoady · 2020-06-02T10:45:12Z

bodhi/server/services/graphql.py

+
+    allReleases = graphene.List(Release)
+
+    def resolve_releases(self, info):


Looks like @cverna was right, graphene handles the mapping under the hood for us, so resolve_allReleases will work here.

StephenCoady · 2020-06-15T11:18:47Z

bodhi/tests/server/services/test_graphql.py

+
+    def test_allReleases(self):
+        """Testing allReleases"""
+        client = Client(Release)


So Release here isn't actually your schema. The schema you want to pass to your test client here is the collection of queries and mutations you have created. Graphene sqlalchemy actually creates this for you when you call graphene.Schema(query=Query). If you check graphql.py you've already done that there so you can use the schema variable :)

karmadolkar · 2020-06-22T12:15:07Z

Riot isn't working on my end right now (Connectivity to server has been lost). :( I'll text there as soon as it works!

StephenCoady · 2020-06-25T13:11:22Z

bodhi/server/graphql_schemas.py

+
+
+class Release(SQLAlchemyObjectType):
+    """Type object representing a distribution release from bodhi.server.models, such as Fedora 27."""


this is the line that is breaking your lint, if you make it multi-line the flake8 run should pass

StephenCoady · 2020-06-25T13:17:56Z

bodhi/tests/server/services/test_graphql.py

+        self.db.add(release)
+        self.db.commit()
+
+        executed = client.execute("""{  allReleases{  name  }}""")


It would be nice to test for some of the trickier fields like state or package_manager (or both!). You could request it from allReleases and make sure it matched the value provided to db.add(). I think that could be a separate test.

StephenCoady · 2020-06-25T13:18:22Z

bodhi/server/graphql_schemas.py

+
+
+class Release(SQLAlchemyObjectType):
+    """Type object representing a distribution release from bodhi.server.models, like Fedora 27"""


this is the line that is breaking your lint, if you make it multi-line the flake8 run should pass

… query allReleases that allows to fetch all Release objects from the database using GraphQL. Fixes fedora-infra#4024. Signed-off-by: Karma Dolkar <karmadolkar29@gmail.com>

cverna · 2020-06-29T13:02:09Z

This lgtm, @StephenCoady if you want to give it another look :-)

StephenCoady

looks good to me. great work :)

karmadolkar requested a review from a team as a code owner May 25, 2020 08:12

karmadolkar changed the title ~~Allow to query Releases using GraphQL~~ [WIP]Allow to query Releases using GraphQL May 25, 2020

StephenCoady reviewed May 25, 2020

View reviewed changes

karmadolkar force-pushed the queryrelease branch from 3ca6391 to d569e42 Compare May 28, 2020 13:43

cverna reviewed Jun 1, 2020

View reviewed changes

StephenCoady reviewed Jun 2, 2020

View reviewed changes

karmadolkar force-pushed the queryrelease branch 4 times, most recently from 5ff3092 to 7237fd0 Compare June 11, 2020 11:30

karmadolkar force-pushed the queryrelease branch from 7237fd0 to a971713 Compare June 15, 2020 05:52

StephenCoady reviewed Jun 15, 2020

View reviewed changes

karmadolkar force-pushed the queryrelease branch from a971713 to 1be960b Compare June 17, 2020 05:49

karmadolkar force-pushed the queryrelease branch 2 times, most recently from 3867fe0 to 58a0f74 Compare June 25, 2020 13:14

StephenCoady suggested changes Jun 25, 2020

View reviewed changes

karmadolkar force-pushed the queryrelease branch from 58a0f74 to 2160a94 Compare June 26, 2020 06:26

Add allReleases query to the GraphQL endpoint. This commit adds a new…

9dd0590

… query allReleases that allows to fetch all Release objects from the database using GraphQL. Fixes fedora-infra#4024. Signed-off-by: Karma Dolkar <karmadolkar29@gmail.com>

karmadolkar force-pushed the queryrelease branch from 6021a0e to 9dd0590 Compare June 29, 2020 11:07

Merge branch 'develop' into queryrelease

f515821

cverna approved these changes Jun 29, 2020

View reviewed changes

StephenCoady approved these changes Jun 29, 2020

View reviewed changes

mergify bot merged commit 84fdcb4 into fedora-infra:develop Jun 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Allow to query Releases using GraphQL #4036

[WIP]Allow to query Releases using GraphQL #4036

karmadolkar commented May 25, 2020

lgtm-com bot commented May 25, 2020

StephenCoady left a comment

StephenCoady May 25, 2020

StephenCoady May 25, 2020

StephenCoady May 25, 2020

StephenCoady commented May 25, 2020

cverna Jun 1, 2020

cverna Jun 1, 2020

cverna Jun 1, 2020

cverna Jun 1, 2020

cverna Jun 1, 2020

cverna Jun 2, 2020

cverna Jun 2, 2020

cverna Jun 1, 2020

StephenCoady Jun 2, 2020

cverna commented Jun 1, 2020

cverna Jun 1, 2020

cverna commented Jun 1, 2020 •

edited

karmadolkar commented Jun 1, 2020

StephenCoady Jun 2, 2020

StephenCoady Jun 15, 2020

karmadolkar commented Jun 22, 2020

StephenCoady Jun 25, 2020

StephenCoady Jun 25, 2020

StephenCoady Jun 25, 2020

cverna commented Jun 29, 2020

StephenCoady left a comment



		class Query(graphene.ObjectType):
		release = graphene.List(Release)


		allReleases = graphene.List(Release)

		def resolve_releases(self, info):



		class Release(SQLAlchemyObjectType):
		"""Type object representing a distribution release from bodhi.server.models, such as Fedora 27."""

[WIP]Allow to query Releases using GraphQL #4036

[WIP]Allow to query Releases using GraphQL #4036

Conversation

karmadolkar commented May 25, 2020

lgtm-com bot commented May 25, 2020

StephenCoady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenCoady commented May 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cverna commented Jun 1, 2020

Choose a reason for hiding this comment

cverna commented Jun 1, 2020 • edited

karmadolkar commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmadolkar commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cverna commented Jun 29, 2020

StephenCoady left a comment

Choose a reason for hiding this comment

cverna commented Jun 1, 2020 •

edited