React-admin: [RFC] GraphQL adaptators improvements

[Weakky]

Hey there,

As of now, most GraphQL adaptators aren't taking full advantage of one of GraphQL's biggest benefit: being able to query nested fields in one request, as deep as the schema allow us to.
Instead, they're more or less imitating the way REST works, by making X request for X reference needed (although some great optimizations were already done in that regard https://marmelab.com/blog/2016/10/18/using-redux-saga-to-deduplicate-and-group-actions.html)

I think there are ways we could improve that, and I suggest we use this place as a central thread to discuss about what could be done.

Suggestion n°1

Include all scalar fields of linked types by default

As of now, we're only fetching ids from linked types, which forces us to use <ReferenceField /> on every linked type.

const linkedResource = introspectionResults.resources.find(
  r => r.type.name === type.name
);

if (linkedResource) {
  return { ...acc, [field.name]: { fields: { id: {} } } };
}

One quick-fix that could be done, is to automatically fetch all scalar fields of linked types:

const linkedResource = introspectionResults.resources.find(r => r.type.name === type.name);

if (linkedResource) {
  // Fetch all linked resource scalar fields
  const linkedScalarFields = linkedResource.type.fields
    .filter(field => getFinalType(field.type).kind !== TypeKind.OBJECT)
    .reduce((acc, field) => ({ ...acc, [field.name]: {} }), {});

  return { ...acc, [field.name]: { fields: linkedScalarFields } };
}

This way, we're able to use <TextField source="linkedType.scalarField" />, which already cover lots of cases and greatly improve the amount of request made.
I think the few more bytes that those additional fields will take on every requests are more than worth the amount of request that it will save.

Suggestion n°2

Make this overridable

After thinking about it, @fzaninotto, I don't think there's a need for a function to transform the introspectionResults. The introspection query already fetches all the existing types (and their fields) anyway.

If I'm not mistaking, overriding the behavior described above is actually already possible and that's what you've done here, by overriding the query that fetches the Command.

I think we could provide a simpler and more concise API to do that, for two reasons:

• The query name and the params are not needed for what we're trying to achieve. The params will always have to be declared anyway, and it can become heavy when overriding GET_LIST actions (having filters, sorting and pagination as input).

• Users have to be careful about the aliases (data and sometimes total)

If you agree with me, I see two ways of enhancing this:

• By providing the same API GraphQL Bindings offers: a simple string.

const COMMAND_FIELDS = `{
  id
  reference
  product { id name }
}`

• By providing the same API as graphqlify expects (more consistent, but heavier IMO)

That's all for now, tell me what you guys think 🙌

[djhi]

Hi @Weakky, thanks a lot for your feedback. The truth is, we don't have much hindsights on the graphql providers yet.

Instead, they're more or less imitating the way REST works, by making X request for X reference needed

Indeed. We opted for this in order to get a proof of concept for graphql providers. There is room for improvements!

Include all scalar fields of linked types by default

I don't think we should as it would defeat another benefit of graphql, only fetching what you need. We can't make that decision for you

Make this overridable

It is already. In fact, you can disable the introspection altogether and provide queries for each requests of each resources. One of our partners is actually doing it. Overly simplifying, they use a dataProvider such as:

import buildGraphQLProvider from 'ra-data-graphql'

const buildQuery = (raFetchType, resourceName, params) => {
  switch (raFetchType) {
    case GET_LIST: {
      switch (resourceName) {
        case 'Comments': {
          return {
            gql: gql`query Comments ($pagination: PaginationInput!, $sort: SortInput!, $filter: ContenuFilterInput){
              data: comments (pagination: $pagination, sort: $sort, filter: $filter){
                data {
                  id
                  body
                }
                total
              }
            }`,
            variables: params,
            parseResponse: response => response.data
          }
        }
      }
    }
  }
};

buildGraphQLProvider({
  client: apolloClient,
  buildQuery,
  introspection: false
});

[djhi]

The way I see it, you should use the introspection enabled dataProvider for prototyping, switching progressively and selectively to hand crafted queries when needed

[fzaninotto]

I agree with @Weakky that overriding the entire query just to add a related resource might come in costly. We have to find something easier.

By the way, the override option doesn't seem to be documented.

[djhi]

What override option ?

[fzaninotto]

https://github.com/marmelab/react-admin/blob/b36bc9dd8eb23dad3289f3e3c3e540e92939b6e9/examples/graphcool-demo/src/dataProvider.js#L35-L49

[Weakky]

Thanks for the fast response !

I don't think we should as it would defeat another benefit of graphql, only fetching what you need. We can't make that decision for you

I agree with you, but unless we find a way to generically detect which linked fields have to be fetched (although according to @fzaninotto, that'd be really hard because of react-admin's current implementation), we will have to make tradeoffs.

And I think it's a fair tradeoff to batch potentially perPage requests per <ReferenceField />. (25 resources with 2 <ReferenceField /> = 50 potential requests made), by adding a few more fields to every requests. I understand it's all about preference though here 😃

Invalid argument, you're batching all CRUD_GET_ONE_REFERENCE to one single GET_MANY using the trick linked above

The way I see it, you should use the introspection enabled dataProvider for prototyping, switching progressively and selectively to hand crafted queries when needed

Totally agree with you here. Detect the hot-spots, and override these queries by handcrafted one when needed. Hence my suggestion to simplify the current API to override the queries, so that it can be done in a more concise way !

[djhi]

Propositions:

1. Remove the override option which I totally forgot and is redundant with decorating the dataProvider to bypass its features when needed
2. Allow the function returned by the queryBuilder to return any of the following:
   
   
   
   • String: We'll pass it to the gql string literal. I think it's not so nice as our users will loose syntax highlighting provided by many IDE for calls to gql but we could support it. We should then passes the params as variables and provide a default parseResponse for them
   
   
   • a query object (result of a call to gql). Same as before, we should then passes the params as variables and provide a default parseResponse for them
   
   
   • For full control because they may need to tranform params or parse the response, they may provide the same object as before
   
   

What do you think ?

[Weakky]

Apologies but I'm not sure to understand how we'd be able to decorate the dataProvider to override some queries here.

Let's assume we're talking about ra-data-graphcool. From a 'provider consumer's point of view', what would it look like if I wanted to override the same query as you did below, using the more concise option among the three that you're proposing ?

https://github.com/marmelab/react-admin/blob/b36bc9dd8eb23dad3289f3e3c3e540e92939b6e9/examples/graphcool-demo/src/dataProvider.js#L4-L49

[djhi]

No problem, I should have taken more time to think about it in the first place :)

There are two ways I think:

1. Keep the override option and make it usable as I described. It would look like the example posted by @fzaninotto but simplified

// in src/buildDataProvider.js
import buildApolloClient from 'ra-data-graphcool';
export default () => 
     buildApolloClient({ 
         clientOptions: { 
             uri: 'https://api.graph.cool/simple/v1/cj2kl5gbc8w7a0130p3n4eg78', 
         }, 
         override: { 
             Command: { 
                 GET_ONE: (params) => ({ 
                     query: gql`...`,  // You could get syntax highlighting here depending on your IDE
                     // Optionally provide variables if you need to work on them for this specific resource/fetchType
                     variables: params,
                     // Optionally provide parseResponse if you need to work on it for this specific resource/fetchType
                     parseResponse: response => response.data
                 }), 
                 // Or
                 GET_ONE: gql`...`,  // You could get syntax highlighting here depending on your IDE
                 // Or
                 GET_ONE: `...`,  // No syntax highlighting here as this is a simple string :(
             }, 
         }, 
});

1. Export the buildQuery function from the dialect aware dataProviders such as ra-data-graphcool so that you can decorate it:

// in src/buildDataProvider.js
import buildApolloClient, { buildQuery } from 'ra-data-graphcool';

const myBuildQuery = buildQuery => (aorFetchType, resource, params) => {
    const builtQuery = buildQuery(aorFetchType, resource, params);

    if (resource === 'Category' && aorFetchType === 'GET_ONE') {
        return {
            ...builtQuery,
            query: gql`...`, // Or a string directly
        };
    }
};

export default () => 
     buildApolloClient({ 
         clientOptions: { 
             uri: 'https://api.graph.cool/simple/v1/cj2kl5gbc8w7a0130p3n4eg78', 
         }, 
         buildQuery: myBuildQuery(buildQuery), 
    });

I personally prefer the second option

[Weakky]

Thanks for the examples, it's much clearer now. I prefer the second option as well. It'd allow us to mimic the override object anyway if we wanted to, and is much permissive regarding what consumers can do with it.

One thing though, unless I missed something, I don't get how the final query will be crafted in case we pass a simple string ?
What I mean is that on your example, assuming ra-data-graphql still passes the query as-is to apollo-client, it means that the apolloArgs and the args will be missing on the query ?

Besides, even if we add some kind of query processing in ra-data-graphql (which I think is not its job anyway), it will not be aware of how the query should be crafted.

The way I saw it, although I'd prefer decorating the dataProvider as well, was to pass the override option directly to ra-data-graphcool (for example), and let it process the overriden queries from within ra-data-graphcool. This way, it was just a matter of overriding the buildGqlQuery::buildFields() function while preserving how buildApolloArgs() and buildArgs() worked.

https://github.com/marmelab/react-admin/blob/b36bc9dd8eb23dad3289f3e3c3e540e92939b6e9/packages/ra-data-graphcool/src/buildGqlQuery.js#L96-L104

I may be wrong here though 🤔

[djhi]

What I mean is that on your example, assuming ra-data-graphql still passes the query as-is to apollo-client, it means that the apolloArgs and the args will be missing on the query ?

No, you missed this part (I added comments to make it clearer):

        return {
            ...builtQuery, // Merge the default builtQuery (variables and parseResponse)
            query: gql`...`, // Or a string directly
        };

It also means that ra-data-graphql will check for the query type and passes to the gql function if it's a string. I still think passing a string is not that great as you'll loose syntax highlighting in your IDE when not using the gql function

[Weakky]

I think you missed what I meant as well, let me rephrase it 😛. I get that the only param overriden on your example will be the query, but my issue is precisely on the query.

Here's what a valid apollo-client query might look like:

query shops(
  $where: ShopWhereInput
  $orderBy: ShopOrderByInput
  $skip: Int
  $first: Int
) {
  items: shops(where: $where, orderBy: $orderBy, skip: $skip, first: $first) {
    id
    name
    address
  }
  total: shopsConnection(
    where: $where
    orderBy: $orderBy
    skip: $skip
    first: $first
  ) {
    aggregate {
      count
    }
  }
}

If we override the query param by something like this:

return {
  ...builtQuery,
  query: `{ id name address }` //Same API as GraphQL Bindings
}

All the apolloArgs will be missing here. Even though we still pass the variables, apollo-client will have nothing to match those params ($where, $orderBy etc..).

Now, I totally get your point regarding the loose of syntax highlight.
My solution is a trade-off between the pain it is to have to always declare those apollo args and be careful about the aliases to match the parseResponse() function, and the loose the syntax highlight.

[Weakky]

I think we were just not on the same line from the beginning.

If the goal of using a simple string is only to remove the use of gql, then there's no point, I agree with you.

Here's what you might be able to achieve using GraphQL Bindings):

// Retrieve `firstName` and `lastName` of a specific user
binding.query.user({ where: { id: 'user_id' } }, '{ firstName lastName }');

(Bindings either accept the query AST passed in all resolvers (known as the info param) OR such a string if you need to customize what fields you want to query)

This would produce the following GraphQL query:

{
  user(where: { id: 'user_id' }) {
    firstName
    lastName
  }
}

Think about it as a GraphQL Fragment. If you look closely to graphql-bindings implementation, this is actually how they're treating that param.

In the example above, { firstName lastName } is equivalent to fragment tmp on User { firstName lastName } (which is a valid GraphQL input)

In a nutshell, the 'simple string' option would be a way to only express the fields user wants to fetch, removing all the apollo-client boilerplate that we don't need anyway.

If you do agree with me now (and I'd understand if you didn't like it at all 😞), best-case scenario would be to allow all three options like you proposed before.
But then again, your original proposition would not allow to do what's shown above 👐

[djhi]

All the apolloArgs will be missing here. Even though we still pass the variables, apollo-client will have nothing to match those params ($where, $orderBy etc..).

Those are the standard parameters of the query matching the LIST fetch type for prisma ? If yes, the ra-data-prisma should have translated them already. If not, you could override the variables as we did for the query. Have I missed something ?

As much as I personally like the graphql bindings syntax, I fail to see what you'd like ra-data-graphql to support here. Could you describe the API of your dream for ra-data-graphql?

[Weakky]

Those are the standard parameters of the query matching the LIST fetch type for prisma ?

Yes they're, but prisma isn't relevant here. I just wanted to highlight apolloArgs in general.

If yes, the ra-data-prisma should have translated them already. If not, you could override the variables as we did for the query. Have I missed something ?

Indeed, but on your proposition, we're overriding the whole query, meaning passing a simple GraphQL fragment as shown above won't work because the apolloArgs will be missing from the query. Even overriding the variables won't change anything, the only valid query that we could pass using your solution would be a full query, including the apolloArgs.
My original suggestion was about providing a more succint API to override queries, hence our disagreement here because decorating the provider would not provide any more concise API.

As much as I personally like the graphql bindings syntax, I fail to see what you'd like ra-data-graphql to support here. Could you describe the API of your dream for ra-data-graphql?

My initial statement is about GraphQL providers in general, and not specifically about ra-data-graphql, that's where I think we lost ourselves, apologies.
Although I understand ra-data-graphcool and ra-data-graphql-simple are only examples, I think it'd be great to improve their API as well to provide better starting point for future GraphQL providers.

Proposition

1. Remove the override params from ra-data-graphql as you suggested, we can indeed override the queries by decorating the buildQuery function.
   Only expect query to be of type gql object, I don't think there's a need for allowing raw strings.
   IMO, decorating the provider should only be used when a very custom behavior is needed for some reasons. It should be considered as a 'low-level API' to customize providers

2. Add a new overrideQueriesByFragment option to ra-data-graphcool and ra-data-graphql-simple, allowing to override queries using simple fragments, in the same way that the current override option works:

   import buildApolloClient from 'ra-data-graphcool';
   export default () => 
    buildApolloClient({ 
        clientOptions: { 
            uri: 'https://api.graph.cool/simple/v1/cj2kl5gbc8w7a0130p3n4eg78', 
        }, 
        overrideQueriesByFragment: { 
            Command: { 
                // Not sure yet whether the params could be of any use here.
                GET_ONE: (params) => '{ id firstName }',
                // If not, this could be enough
                GET_ONE: '{ id firstName }',
            }, 
        }, 
   });
   

   Below is what could be a straightforward implementation to convert such a fragment into something that graphlify understands:

   

3. (Optional) Inject the whole schema into the introspectionResults. We already have one use-case: being able to validate the fragments against the schema

   import { validate } from 'graphql'
   
   const document = parse('fragment tmp on User { firstName lastName }');
   
   validate(schema, document);
   

   graphql-js reference implementation is a treasure-chest when it comes to utils functions, there might be tons of other usecases that people could use thanks to the schema.

What do you think ?

[djhi]

I think I'm really lost now :laughing:

1. We at least agree on that :smile:, except that decorating the provider is actually not low level. As for the REST providers, this can be used to add features around the provider (like the addUploadFeature in the documentation) but that's not important

2. This is where I got lost. What is achieved by this overrideQueriesByFragment that cannot be achieved by decorating the buildQuery? Is your example correct?

3. Make sense, we could add the whole schema to the introspection results

[Weakky]

I just roughly implemented the query overriding by fragments on a branch on ra-data-prisma for you to see how I'd implement what I'm suggesting https://github.com/Weakky/ra-data-prisma/pull/7 😃

[djhi]

Ok, I think I get what you're doing now. Basically, you want a way to override a query without having to write the query boilerplate with parameters, etc.

I think the overrideQueriesByFragment is cumbersome to use though. Taking your repository product as an example, I suggest doing something like the following:

// in src/buildDataProvider.js
import buildApolloClient, { buildQuery } from 'ra-data-graphcool';

const productGetListQuery = gql`
    fragment product on Product {
        id
        name
        description
        brand {
            id
            name
        }
        category {
            id
            name
        }
        shop {
            id
            name
        }
        attributes {
            id
            value
        }
    }
`; 
const myBuildQuery = buildQuery => (aorFetchType, resource, params) => {
    if (resource === 'Category' && aorFetchType === 'GET_ONE') {
        // buildQuery accepts an extra parameter which is the query/fragment to use as an override
        // The rest of your logic still applies but you wont have to find the override
        return buildQuery(aorFetchType, resource, params, productGetListQuery);
    }

    return buildQuery(aorFetchType, resource, params);
};

export default () => 
     buildApolloClient({ 
         clientOptions: { 
             uri: 'https://api.graph.cool/simple/v1/cj2kl5gbc8w7a0130p3n4eg78', 
         }, 
         buildQuery: myBuildQuery(buildQuery), 
    });

Besides, I assume the only thing we have to do on our side is to include the full schema in the introspectionResults so that you can validate fragments, etc. Right?
We may then steal your code for our own providers :stuck_out_tongue:

[Weakky]

Ok, I think I get what you're doing now

You should've seen my smile reading those sweet words 😂

I think the overrideQueriesByFragment is cumbersome to use though. Taking your repository product as an example, I suggest doing something like the following

This is fine for me. Your way of doing it is more consistent as you're planning to remove the override option. Everything will have to be done by decorating the provider, offering more freedom.
I'll be able to use my cumbersome data structure anyway to avoid those ugly if statements 😄

Besides, I assume the only thing we have to do on our side is to include the full schema in the introspectionResults so that you can validate fragments, etc. Right?
We may then steal your code for our own providers

Exactly ! Let me recap everything:

1. Remove the override option from ra-data-graphql
2. Include the schema into the introspectionResults (other usecases might be found in the future)
3. Adapt the current examples to showcase how queries can be overriden using fragments

I might find some time to send a few PR's 🎉

[djhi]

Closing this issue as the first part has been merged and will be available in 2.3.0