[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]

react-admin/examples/graphcool-demo/src/dataProvider.js

Lines 35 to 49 in b36bc9d

	export default () =>
	buildApolloClient({
	clientOptions: {
	uri: 'https://api.graph.cool/simple/v1/cj2kl5gbc8w7a0130p3n4eg78',
	},
	// We need to override the default Command query because we
	// to get the deeply nested products inside basket
	override: {
	Command: {
	GET_ONE: () => ({
	query: getOneCommandQuery,
	}),
	},
	},
	});

[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 ?

react-admin/examples/graphcool-demo/src/dataProvider.js

Lines 4 to 49 in b36bc9d

	const getOneCommandQuery = gql`
	query Command($id: ID!) {
	data: Command(id: $id) {
	id
	reference
	date
	status
	returned
	taxRate
	total
	deliveryFees
	totalExTaxes
	customer {
	id
	firstName
	lastName
	}
	basket {
	id
	product {
	id
	reference
	price
	stock
	}
	quantity
	}
	}
	}
	`;
	export default () =>
	buildApolloClient({
	clientOptions: {
	uri: 'https://api.graph.cool/simple/v1/cj2kl5gbc8w7a0130p3n4eg78',
	},
	// We need to override the default Command query because we
	// to get the deeply nested products inside basket
	override: {
	Command: {
	GET_ONE: () => ({
	query: getOneCommandQuery,
	}),
	},
	},
	});

[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 :(
             }, 
         }, 
});

2. 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.

react-admin/packages/ra-data-graphcool/src/buildGqlQuery.js

Lines 96 to 104 in b36bc9d

	export default introspectionResults => (
	resource,
	aorFetchType,
	queryType,
	variables
	) => {
	const apolloArgs = buildApolloArgs(queryType, variables);
	const args = buildArgs(queryType, variables);
	const fields = buildFields(introspectionResults)(resource.type.fields);

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?