Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Graphql #2186

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Graphql #2186

merged 1 commit into from
Aug 26, 2020

Conversation

jgrund
Copy link
Member

@jgrund jgrund commented Aug 25, 2020

Implement Experimental GraphQL support

This change is Reviewable

@jgrund jgrund self-assigned this Aug 25, 2020
@jgrund jgrund changed the base branch from master to corosync_daemon August 25, 2020 12:29
@jgrund jgrund force-pushed the graphql branch 3 times, most recently from 3f401e0 to a30c51f Compare August 25, 2020 14:46
Base automatically changed from corosync_daemon to master August 25, 2020 16:05
@jgrund jgrund force-pushed the graphql branch 2 times, most recently from 358d07b to c080ea7 Compare August 25, 2020 20:00
Signed-off-by: Joe Grund <jgrund@whamcloud.io>
@jgrund jgrund marked this pull request as ready for review August 25, 2020 21:58
@jgrund jgrund requested a review from a team August 25, 2020 21:58
@jgrund
Copy link
Member Author

jgrund commented Aug 25, 2020

This patch adds two new endpoints:

/graphql - Accepts a POST and data as described in https://graphql.org/

/graphiql - Loads an interactive query visualizer in the browser.

Screen Shot 2020-08-25 at 5 54 24 PM


#[juniper::graphql_object(Context = Context)]
impl QueryRoot {
#[graphql(arguments(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This effectively gives a default it looks like. Seems like a good solution to having to explicitly specify a limit/direction on every call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just docs for the query browser. The default is due to the unwrap_or calls within this fn.

@jgrund jgrund requested a review from a team August 26, 2020 09:49
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jgrund and @johnsonw)


chroma-manager.conf.template, line 226 at r1 (raw file):

        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_pass {{IML_API_PROXY_PASS}}/graphiql;
    }

Why no compression here?


iml-api/src/graphql.rs, line 62 at r1 (raw file):

Previously, jgrund (Joe Grund) wrote…

This is just docs for the query browser. The default is due to the unwrap_or calls within this fn.

In theory, could we just accept same JSON with limit, offset and direction and not use GraphQL here? Just trying to understand what does GraphQL bring in specifically.

@jgrund
Copy link
Member Author

jgrund commented Aug 26, 2020


chroma-manager.conf.template, line 226 at r1 (raw file):

Previously, mkpankov (Michael Pankov) wrote…

Why no compression here?

I'll add in a follow-on

@jgrund
Copy link
Member Author

jgrund commented Aug 26, 2020


iml-api/src/graphql.rs, line 62 at r1 (raw file):

Previously, mkpankov (Michael Pankov) wrote…

In theory, could we just accept same JSON with limit, offset and direction and not use GraphQL here? Just trying to understand what does GraphQL bring in specifically.

Yes, we could do the same with query string params.

currently, the biggest draws are:

  1. API Documentation generated from code (which is very important for external consumers)
  2. A query explorer
  3. A consistently enforced way to structure endpoints
  4. The ability to request just the data that is needed from clients

Of all of those, the ability to generate extensive documentation (which we will need to stay on top of) is the most important imo.

Copy link
Member Author

@jgrund jgrund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnsonw)


iml-api/src/graphql.rs, line 62 at r1 (raw file):

Previously, jgrund (Joe Grund) wrote…

Yes, we could do the same with query string params.

currently, the biggest draws are:

  1. API Documentation generated from code (which is very important for external consumers)
  2. A query explorer
  3. A consistently enforced way to structure endpoints
  4. The ability to request just the data that is needed from clients

Of all of those, the ability to generate extensive documentation (which we will need to stay on top of) is the most important imo.

Done.

@jgrund jgrund merged commit 56df47e into master Aug 26, 2020
@jgrund jgrund deleted the graphql branch August 26, 2020 11:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants