Objection.js: Model insert calling twice in GraphQL

[zefexdeveloper]

I'm trying to use Objection.js in GraphQL (Apollo) and I have a mutation like this:

import User from '../models/user'

export default {
  Mutation: {
    createUser: (parent, args) => User.query().insert(args)
  }
}

When I run this mutation the insert is being called twice (the mutation itself is called only once, I even put a console.log there and the only thing that is called twice is the insert).

I enabled the debug: true and that is the return on the console:

{ method: 'insert',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ '5d7ee514-cb73-4ef5-b880-96053a551726', 'John Doe' ],
  __knexQueryUid: 'fe07743f-4e04-458a-b533-19635f8fc7b8',
  sql:
   'insert into "users" ("id", "name") values (?, ?) returning "id"',
  returning: 'id' }
{ method: 'insert',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ '5d7ee514-cb73-4ef5-b880-96053a551726', 'John Doe' ],
  __knexQueryUid: '7fdc03f8-6f58-4673-ba6b-b20c88af43ab',
  sql:
   'insert into "users" ("id", "name") values (?, ?) returning "id"',
  returning: 'id' }

And the return of that is:

"errors": [
    {
      "message": "insert into \"users\" (\"id\", \"name\") values ($1, $2) returning \"id\" - duplicate key value violates unique constraint \"users_pkey\"",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],

What is happening?

[zefexdeveloper]

It was my mistake, the mutation should be created with the async keyword.

[falkenhawk]

I encountered the same issue. I am trying to understand why this is happening. Returning a QueryBuilder instance from a resolver (which is a promise itself, a bluebird-promise to be specific) should just work fine, right? It shouldn't be necessary to add an async keyword (to return Promise) if await is not used in the function body. @koskimas may I ask for your insight here, please?

[koskimas]

You aren't really giving me anything to work with. How could I possibly help here? Yes, no need to mark a function async if you return a promise from it.

[koskimas]

Just a quess: something stores the return value from that mutation, which is actually a query builder, not a promise. QueryBuilder is a thenable, not a promise, so calling then twice on that query builder will insert twice.

const builder = Person.query().insert(stuff)
// Starts a query
builder.then()
// Starts another query.
builder.then()
// This only starts one query, because `.then()` returns an actual promise.
builder.then().then()

Something storing a promise and calling .then twice on the same object seems like a poor design that isn't compatible with thenables.

As a remedy you can do one of the following:

1. Explicitly call execute() for the builder. execute() returns a real promise.
2. Convert the builder to a real promise Promise.resolve(Person.query().insert(stuff)
3. Mark the function async, which will wrap the return value into a native promise.

[falkenhawk]

Thank you @koskimas that helps a lot. I know there is not much info here, a reproduction scenario would be definitely helpful. Anyway, the issue is outside the objection's scope. But with your hints I may suspect something fishy is going on in the graphql server itself. I will dig into that.

[falkenhawk]

while trying to shoot the issue blindly from different angles, I noticed that adding then at the end: createUser: (parent, args) => User.query().insert(args).then(r => r) prevents the query to be executed twice, but that's probably logical, since the graphql resolves an already then-ed query 🤔

[koskimas]

Yeah, QueryBuilder.then executes the query and returns a promise. As I mentioned earlier, query builders are not promises. They are objects that have a then function that returns a promise.

[falkenhawk]

@koskimas I've found the culprit in graphql apollo server...
https://github.com/apollographql/apollo-server/blob/master/packages/graphql-extensions/src/index.ts#L273
https://github.com/apollographql/apollo-server/blob/master/packages/graphql-extensions/src/index.ts#L297
there result.then is called but it's not returned (where result is a QueryBuilder instance returned from a graphql resolver function)...

ps. this has been introduced in apollo server 2.x, the issue was not present in apollo server 1.4.0

[falkenhawk]

Just for the reference: my colleague has reported the issue on apollo-server. https://github.com/apollographql/apollo-server/issues/2501

[koskimas]

:+1: The answer from the apollo-server people can easily be we don't support this kind of objects or that objection is doing it wrong. Those are both valid opinions. Thenable isn't really a well defined thing. I don't think there's a specification anywhere that says what should happen if then of a thenable is called multiple times.

https://promisesaplus.com/

I think the right solution is to just use async in the function definition. The performance drop from adding another promise wrap is a fraction of a microsecond per function call.

[falkenhawk]

@koskimas they replied, just FYI ;) https://github.com/apollographql/apollo-server/issues/2501#issuecomment-509404448

[falkenhawk]

btw, this is what we did in the meantime, as a workaround:

import { GraphQLField, GraphQLObjectType, GraphQLSchema } from 'graphql';
import { QueryBuilder } from 'objection';

const wrapResolver = (field: GraphQLField<any, any>): void => {
  if (typeof field.resolve !== 'function') {
    return;
  }

  const fieldResolver = field.resolve;
  field.resolve = (source, args, context, info) => {
    const res = fieldResolver(source, args, context, info);
    if (res instanceof QueryBuilder) {
      return res.execute();
    }
    return res;
  };
};

/**
 * Workaround for executing a query twice if an instance of objection.js' QueryBuilder is returned from a resolver.
 * A QueryBuilder is executed when .then() is called. An arbitrary execution is caused by "whenResultIsFinished"
 * function in "graphql-extensions" (by apollo - introduced in apollo-server 2.x) - handling "willResolveField" hook.
 *
 * The solution would be to either always remember about executing the query already in a resolver, or mark all resolvers
 * returning a QueryBuilder with "async" - which would return a Promise instead of a QueryBuilder itself, but
 * it would be prone to human errors. Better ensure it by wrapping all resolvers, at least until apollo guys fix the issue, if ever...
 *
 * https://github.com/Vincit/objection.js/issues/1143
 * https://github.com/apollographql/apollo-server/issues/2501
 */
const wrapResolversReturningObjectionQueryBuilders = (schema: GraphQLSchema): void => {
  const typeMap = schema.getTypeMap();
  Object.keys(typeMap).forEach(typeName => {
    const type = typeMap[typeName];

    if (!typeName.startsWith('__') && type instanceof GraphQLObjectType) {
      const fields = type.getFields();
      Object.keys(fields).forEach(fieldName => {
        wrapResolver(fields[fieldName]);
      });
    }
  });
};

export default wrapResolversReturningObjectionQueryBuilders;

this is to be used on a GraphQLSchema instance:

wrapResolversReturningObjectionQueryBuilders(schema);