Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using with dataloader-php throws "Could not resolve promise" Exception #150

Closed
n1ru4l opened this issue Aug 17, 2017 · 18 comments
Closed

Using with dataloader-php throws "Could not resolve promise" Exception #150

n1ru4l opened this issue Aug 17, 2017 · 18 comments

Comments

@n1ru4l
Copy link
Contributor

n1ru4l commented Aug 17, 2017

I have three types

type Questionnaire {
  id: Int!
  blocks: [Int]!
}

type Block {
  id: Int!
  wrappers: [Wrapper]!
}

type Wrapper {
  id: Int!
  questions: [Question]!
}

type Question {
  id: Int!
}

type Query {
  questionnaire(questionnaireId: Int!): Questionnaire
  wrapper(wrapperId: Int!): Wrapper
}

Every field that returns a list uses a DataLoader (like in https://github.com/overblog/dataloader-php#using-with-webonyxgraphql). Each DataLoader is identical, despite the tables from which the dataloaders fetch their data.

The following query works fine:

query questionnaire {
  questionnaire(questionnaireId: 1) { # no dataloader
    blocks { # resolver uses dataloader
      wrappers { # resolvers uses dataloader
        id
      }
    }
  }
}

However going one level deeper results in an exception.

query questionnaire {
  questionnaire(questionnaireId: 1) { # no dataloader
    blocks { # resolver uses dataloader
      wrappers { # resolvers uses dataloader
        id
        questions { # resolver uses dataloader
          id
        }
      }
    }
  }
}
InvariantViolation in SyncPromiseAdapter.php line 151:
Could not resolve promise

When I log the status of the promise I get pending.

According to the code this should not happen. https://github.com/webonyx/graphql-php/blob/master/src/Executor/Promise/Adapter/SyncPromiseAdapter.php#L132

When I use the following query (only 2 dataloader) everything is behaving like excpected:

query wrapper {
  wrapper(wrapperId: 1) { # resolvers uses dataloader
    id
    questions { # resolver uses dataloader
      id
    }
  }
}

Edit: This also happens in this resolver function where I chain multiple promises (I am using laravel):

public function resolveIsValidField($questionnaire, $args, $context) {
  $questionnaireId = is_array($questionnaire) ? $questionnaire['id'] : $questionnaire->id;
  /** @var BlockRepositoryInterface $blockRepository */
  $blockRepository = $context['repositories']['blocks'];
  /** @var WrapperRepositoryInterface $wrapperRepository */
  $wrapperRepository = $context['repositories']['wrappers'];
  /** @var QuestionRepositoryInterface $questionRepository */
  $questionRepository = $context['repositories']['questions'];

  $blocks = null;
  $wrappers = null;

  return $blockRepository->findWhereQuestionnaireId($questionnaireId)
    ->then(function (Collection $blockRecords) use (&$blocks, $wrapperRepository) {
      $blocks = $blockRecords;
      $bockIds = $blocks->map(function (Block $block) {
        return $block->id;
      })->toArray();

      return $wrapperRepository->getWhereBlockIds($bockIds);
    })
    ->then(function ($wrapperRecords) use (&$wrappers, $questionRepository) {
      $wrappers = collect($wrapperRecords)->flatten();
      $wrapperIds = $wrappers->pluck('id');
      return $questionRepository->getWhereWrapperIds($wrapperIds->toArray());
    })
    ->then(function ($questionRecords) use (&$blocks, &$wrappers) {
      $questions = collect($questionRecords)->flatten();

      return isQuestionnaireValid($blocks, $wrappers, $questions);
    });
}
@vladar
Copy link
Member

vladar commented Aug 17, 2017

I am curious, have you tried the same with just GraphQL\Deferred (without DataLoader)?

Looks like DataLoader project wraps original adapter, so it's hard to say without digging deeper if this is a bug in this library or in DataLoader.

@mcg-web any thoughts?

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Aug 18, 2017

@vladar Could you give me an example on how I could achieve something like the code I added in my last edit with GraphQL\Deferred?

I will try to setup a reproduction repository using laravel-graphql and dataloader-php.

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

@vladar without a complete use case it will be difficult to give my help on this... @n1ru4l with a complete example I can maybe get what going wrong :)

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Aug 18, 2017

@mcg-web Here is my reproduction repository: https://github.com/n1ru4l/laravel-graphql-dataloader-php-exception-reproduction

The relevant files are the Service Provider (app/Providers/AppServiceProvider.php), the types and queries app/GraphQL/* and the DataLoaders app/DataLoader/*.php

You will not need any database setup just use php artisan serve.

You can navigate to graphiql on /graphiql.

This query fails:

query type1 {
  type1 {
    __typename
    id
    items {
      __typename
      id
      items {
        __typename
        id
        items {
          __typename
          id
        }
      }
    }
  }

This query works:

query type1 {
  type1 {
    __typename
    id
    items {
      __typename
      id
    }
  }
}

@vladar
Copy link
Member

vladar commented Aug 18, 2017

I forked your repo and replaced DataLoaders with regular GraphQL\Deferred calls. It works as expected.

I guess the bug is somewhere on DataLoader side.

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Aug 18, 2017

@vladar Thank you for the fast response :) Is there any option to move this issue to dataloader-php repository?

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

I am trying to get why @n1ru4l . if I don't get i'll create the issue on dataloader repository :)

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Aug 18, 2017

OK cool, also thank you for the fast responses :)

@vladar
Copy link
Member

vladar commented Aug 18, 2017

@mcg-web If I add DataLoader::await() to queue progression, it works, e.g.:

    /**
     * Synchronously wait when promise completes
     *
     * @param Promise $promise
     * @return mixed
     */
    public function wait(Promise $promise)
    {
        $dfdQueue = Deferred::getQueue();
        $promiseQueue = SyncPromise::getQueue();

        while (
            $promise->adoptedPromise->state === SyncPromise::PENDING &&
            !($dfdQueue->isEmpty() && $promiseQueue->isEmpty())
        ) {
            Deferred::runQueue();
            SyncPromise::runQueue();
            DataLoader::await();
        }

        /** @var SyncPromise $syncPromise */
        $syncPromise = $promise->adoptedPromise;

        if ($syncPromise->state === SyncPromise::FULFILLED) {
            return $syncPromise->result;
        } else if ($syncPromise->state === SyncPromise::REJECTED) {
            throw $syncPromise->result;
        }

        throw new InvariantViolation("Could not resolve promise");
    }

So the whole queue must be progressed together. I guess you guys will have to write your own wait implementation to make it behave as expected.

Obviously, I am not 100% sure that this is enough to fix it (as I am not aware of DataLoader::await implementation details).

But let me know if I can make it easier for you somehow (e.g. by changing an interface of SyncPromiseAdapter or maybe changing how Executor calls wait).

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

Yes that where the bug is coming from, thank you. it works if we add the dataloader::await but only if we keep the wait of the override method

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

here a solution that works:

  • we add two hooks to GraphQL lib
// src/Executor/Promise/Adapter/SyncPromiseAdapter.php

    /**
     * Synchronously wait when promise completes
     *
     * @param Promise $promise
     * @return mixed
     */
    public function wait(Promise $promise)
    {
        $this->beforeWait($promise);

        $dfdQueue = Deferred::getQueue();
        $promiseQueue = SyncPromise::getQueue();

        while (
            $promise->adoptedPromise->state === SyncPromise::PENDING &&
            !($dfdQueue->isEmpty() && $promiseQueue->isEmpty())
        ) {
            Deferred::runQueue();
            SyncPromise::runQueue();
            $this->onWait($promise);
        }

        /** @var SyncPromise $syncPromise */
        $syncPromise = $promise->adoptedPromise;

        if ($syncPromise->state === SyncPromise::FULFILLED) {
            return $syncPromise->result;
        } else if ($syncPromise->state === SyncPromise::REJECTED) {
            throw $syncPromise->result;
        }

        throw new InvariantViolation("Could not resolve promise");
    }

    /**
     * Execute just before starting to run promise completion
     *
     * @param Promise $promise
     */
    protected function beforeWait(Promise $promise)
    {
    }

    /**
     * While running promise completion
     *
     * @param Promise $promise
     */
    protected function onWait(Promise $promise)
    {
    }
  • we modify DataLoader custom implementation to do the work
    protected function beforeWait(Promise $promise)
    {
        DataLoader::await();
    }

    protected function onWait(Promise $promise)
    {
        DataLoader::await();
    }

@vladar tell me what do you think of this solution please?

@vladar
Copy link
Member

vladar commented Aug 18, 2017

Works for me, pragmatically %) Can anyone prepare PR for this? I'll merge it.

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

I'll submit it, thank you @vladar ;)

@vladar
Copy link
Member

vladar commented Aug 18, 2017

Thanks! Merged and released 0.9.14

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

thanks again ;)

@mcg-web
Copy link
Collaborator

mcg-web commented Aug 18, 2017

@n1ru4l new release here, also require to update graphql to 0.9.14 (graphql is not a requirement of the lib)...

@n1ru4l
Copy link
Contributor Author

n1ru4l commented Aug 18, 2017 via email

@vladar
Copy link
Member

vladar commented Aug 20, 2017

Closing this. Feel free to re-open if it is still broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants