Associations that just retrieve belongsTo id should not execute unnecessary query

[mschipperheyn]

I initially posted this issue on dataloader-sequelize (https://github.com/mickhansen/dataloader-sequelize/issues/87) but it prob belongs here:

I noticed an inefficiency that I think warrants a small optimization:

Let's assume the following type definition

type User {
   id: ID!
   bestFriend: User # based on belongsTo
}

resolver

User: {
    bestFriend: resolver(() => models.User.associations.bestFriend),
}

If on the client side I have the following fragment:

fragment userFragment on User {
     id
     bestFriend {
         id
    }
}

I see a separate query being executed for bestFriend. Since the bestFriend id is available on the root and it's the only key we're asking for, this query should not be executed.

An alternative would be to add bestFriendId on the User type, but I would consider that hackish.

[mickhansen]

It does belong here. However i'm not sure it's a practical change to make. It's generally easier just to reason about objects and them being there or not.

You also end up with a bunch of fragment related edge cases once you start reasoning about what attributes were actually requested (graphl-sequelize had this once but it was more complicated than what it was worth).

[mschipperheyn]

Wow, that was fast! it may be just my performance fetish. However, in this particular case, I think fragment edge case should not be considered. Bc, if you're just requesting an id, it would not make sense to request a fragment imho. Although I do understand that as an edge case that might happen.

[mickhansen]

If you use Relay you end up with fragments all over the place. I suppose we could apply the optimization only if there are no fragments.

However this can also only be applied if we for a fact know that the requested attribute maps to a preloaded value. As the field might actually have a resolve function itself this could be tricky.

Not saying it's impossible, and a PR would be welcome, but it's not necessarily simple :)

[mschipperheyn]

Right, yeah, I get it. There are lot of real world scenarios to consider that make this harder than the simple scenario I'm thinking of. Thinking of it differently. Could this be handled in a customized fashion on the graphql-sequelize resolver function itself?

The way you might reference a parent on a graphql resolver for a specific value and short circuit a return value directly, preventing a database hit?

[mickhansen]

The best way i could think of is having resolvers on the fields of the best friend type itself that could then have some sort of logic - but that would require that the field resolver knows the original source, rather than just the source of the values for the type.

[mschipperheyn]

Hmm, yeah, I guess this workaround might also be feasible

bestFriend: (parent, args, context, info) => {
    if (!parent.bestFriendId){
        return null;
    }
    const isIdOnly = idOnly(info);
    if(isIdOnly) {
         return { id: parent.bestFriendId };
    }
    return resolver(() => models.User.associations.bestFriend)(parent, args, context, info);
}

I guess I could make this a generic wrapper idOnlyResolver. Since all my types follow the same pattern.

[mschipperheyn]

Something like this. Currently, oriented towards belongsTo style relations

import invariant from 'invariant';
import { resolver } from 'graphql-sequelize';

const defaultOptions = {
    idScalar: 'id',
};

/**
 * options.idField: The name of the sequelize field that represents the foreign key
 * options.idScalar: The name of the graphql id field (default: 'id')
 */
const idOnlyResolver = (options, ...rest) => (parent, args, context, info) => {
    const opts = { ...defaultOptions, ...options };
    const { idField, idScalar } = opts;
    invariant(idField, 'Missing sequelize idField');
    if (!parent[idField]) {
        return null;
    }
    const fieldArray = info.fieldNodes[0].selectionSet.selections;
    // assume one of the two fields is the __typename
    if (
        fieldArray.length === 2 &&
        fieldArray.some(f => f.name.value === idScalar)
    ) {
        return { id: parent[idField] };
    }
    return resolver(...rest)(parent, args, context, info);
};

export default idOnlyResolver;

WDYT?

[mickhansen]

Yeah something like that could definitely work. Another option could be to modify the before hook of the graphql-sequelize to support short circuiting and returning results.

[mschipperheyn]

Yeah, that would be interesting.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.