Nested async queries messes up the order of keys in arrays producing unwanted responses #80

lordthorzonus · 2016-12-20T14:59:20Z

When batch loading multiple nested relations in an async environment the values of the arrays aren't resolved in a sync order anymore which produces php arrays whose indexes aren't ordered properly. This causes json_encode to evaluate them as associative arrays and add the keys to the json response.

For example a query:

{
  eventParticipantsForAnEvent(eventId: 2006) {
    id
    contactInfo{
      id
      companyInfo{
        id
      }
    }
  }
}

Where all the relations are one -> one and batch loaded it produces following json:

{
  "data": {
    "eventParticipantsForAnEvent": {
      "0": {
        "id": 89906,
        "contactInfo": {
          "id": 112218,
          "companyInfo": {
            "id": 25623
          }
        }
      },
      "1": {
        "id": 89919,
        "contactInfo": {
          "id": 112570,
          "companyInfo": {
            "id": 25696
          }
        }
      }
    }
  }
}

When the expected result would be:

{
  "data": {
    "eventParticipantsForAnEvent": [
      {
        "id": 89906,
        "contactInfo": {
          "id": 112218,
          "companyInfo": {
            "id": 25623
          }
        }
      },
      {
        "id": 89919,
        "contactInfo": {
          "id": 112570,
          "companyInfo": {
            "id": 25696
          }
        }
      }
    ]
  }
}

I'm not sure if this is the right approach to solve the problem. But at least it removes it :). If we want to add tests we probably need to add the event loop as dev dependency?

coveralls · 2016-12-20T15:06:29Z

Coverage increased (+0.006%) to 91.889% when pulling 54ece56 on lordthorzonus:nested-async-queries-mess-the-indexes-of-arrays into ff3a40d on webonyx:master.

mcg-web · 2016-12-20T15:08:01Z

Hi, can you tell me what promise adapter are you using?

vladar · 2016-12-20T15:09:48Z

Hey, thanks for reporting. Looks like it's not the order that is wrong - it's just that keys are set as strings vs int.

I don't think ksort is a proper fix for the problem, so need a bit more info on how to reproduce it:

Do you use ReactPHP or some other async environment?
Also I am curious if you return "Promise for list" or "List of promises" in resolver?

As for adding event loop to tests - I wanted to leverage HHVM in Travis CI to test it, but couldn't find decent promise library for HHVM. Any hints how to integrate async stuff with Travis are welcome!

lordthorzonus · 2016-12-20T17:01:50Z

I'm using ReactPromiseAdapter in a React event loop environment.

The batch loader I'm using returns a promise which resolves into a value (the relation between entities in this case is one eventParticipant has one contactInfo and contactInfo has one companyInfo).

I think the problem is the wrong order of index keys in array. For my examples I shortened the responses (the real one would contain 600 entities), Sorry for being unclear :). I tried var dumping the execution result before json_decoding and the keys are integers like they should.

The print_r looks following where the un-ordering first occurs:

As for the unit testing I think we just need to new up the event loop and run that explicitly before asserting the test results.

Please let me know if you need some more info! I can for example try to replicate this in a dummy repo with some mock data :).

vladar · 2016-12-20T18:15:59Z

Sorry I am a bit slow today, so just to clarify...

The question is who actually creates this array with gaps? Is it produced by your resolver/promise or produced by graphql?

In the former case the result is expected. Here is a simple test which produces the same result as you describe:

$a = [0 => 'a', 1 => 'b', 100 => 'z'];
echo json_encode($a);

If your app returns array with such keys - it should be probably fixed in the user-land (but we can think about forcing arrays with valid keys). But if you return array with ordered integer keys without gaps and graphql re-orders them - this is a bug.

So which is the case?

lordthorzonus · 2016-12-20T18:25:31Z

Basically resolving promises async makes the array unordered. Here is a messy test to reproduce the problem. We are expecting an array with of [1,2,3] (keys: 0,1,2) for the test field but we are actually getting it in order [1,3,2] (keys:0,2,1) as the second promise is resolved async later than the others. I think the problem happens somewhere when we are doing $promise->all(); The arrays keys are somehow preserved but the order changes as the values are resolved in different order.

    public function testPromiseListsPreserveTheirArrayKeyOrder()
    {
        $loop = \React\EventLoop\Factory::create();
        Executor::setPromiseAdapter(new ReactPromiseAdapter());

        $testData = [
            new \React\Promise\Promise(
                function ($resolve) use ($loop) {
                    $loop->nextTick(
                        function () use ($resolve) {
                            $resolve(1);
                        }
                    );
                }
            ),
            new \React\Promise\Promise(
                function ($resolve) use ($loop) {
                    $loop->nextTick(
                        function () use ($resolve, $loop) {
                            \React\Promise\resolve()->then( function() use ($resolve, $loop) {
                                $loop->nextTick(function () use ($resolve) {
                                    $resolve(2);
                                });
                            });
                        }
                    );
                }
            ),
            new \React\Promise\Promise(
                function ($resolve) use ($loop) {
                    $loop->nextTick(
                        function () use ($resolve) {
                            $resolve(3);
                        }
                    );
                }
            )
        ];

        $expected =  [ 'data' => [ 'nest' => [ 'test' => [ 1, 2, 3 ] ] ] ];
        $expectedJson = json_encode($expected);


        $data = ['test' => $testData];
        $dataType = null;
        $testType = Type::nonNull(Type::listOf(Type::nonNull(Type::int())));

        $dataType = new ObjectType(
            [
                'name' => 'DataType',
                'fields' => function () use (&$testType, &$dataType, $data) {
                    return [
                        'test' => [
                            'type' => $testType
                        ],
                        'nest' => [
                            'type' => $dataType,
                            'resolve' => function () use ($data) {
                                return $data;
                            }
                        ]
                    ];
                }
            ]
        );

        $schema = new Schema(['query' => $dataType]);

        $ast = Parser::parse('{ nest { test } }');
        $result = Executor::execute($schema, $ast, $data);

        $response = null;
        $loop->run();

        $result->then(function($value) use (&$response) {
            $response = $value;
        });

        $this->assertEquals($expectedJson, json_encode($response->toArray()));
    }

vladar · 2016-12-20T18:28:51Z

Thanks a lot for the test case! Will check it as soon as I have a chance.

lordthorzonus · 2016-12-20T18:58:22Z

I investigated bit further and this could be a problem (or indented behavior) in ReactPHP itself. I opened a issue there : reactphp/promise#74 .

If it's indented the question is should we ever expect a unordered array as a list? As it should be a list by GraphQl spec. Javascript doesn't have this problem as arrays are always in order and arrays with gaps are converted to json arrays correctly.

Thanks a thousand for help! :)

vladar · 2016-12-20T19:14:05Z

I suspected that it could be caused by ReactPHP all, but you are right - we should probably force proper keys in array when expected return type is List. I'll create separate issue to track this.

vladar · 2016-12-21T10:12:19Z

@lordthorzonus I think we should add ksort to ReactPromiseAdapter::all anyway. Can you change this PR accordingly (move ksort from Executor to ReactPromiseAdapter)? I'll merge it then.

mcg-web · 2016-12-21T10:30:41Z

Hi @vladar, I think that ksort is not a reliable solution, why not implement something similar to this ?

vladar · 2016-12-21T10:46:41Z

@mcg-web You are right. ksort will mess keys of associative arrays. I guess what we need in ReactPromiseAdapter::all is something like this:

function all(array $promisesOrValues)
{
    $result = \React\Promise\all($promisesOrValues);
    return $result->then(function($values) use ($promisesOrValues) {
        $newResult = [];
        foreach ($promisesOrValues as $key => $value) {
            $newResult[$key] = $values[$key];
        }
        return $newResult;
    });
}

Is it what you mean?

mcg-web · 2016-12-21T10:49:01Z

Yes @vladar, you got it 👍

lordthorzonus · 2016-12-21T10:50:48Z

@mcg-web @vladar 👍 I was thinking the same. I'll update the PR. Do we want tests as well for this? Then we probably need to add react/event-loop and react/promise as a dev-dependency. Is it acceptable?

vladar · 2016-12-21T10:59:15Z

@lordthorzonus I don't think we should add it as dev dependency. I don't want to create impression that ReactPHP is somwhow required for development when using this lib. It is 100% optional stuff and composer suggest is appropriate for it.

What we could do with reagrds to tests:

Add tests for ReactPHP promise adapter, but exclude them in default phpunit.xml.dist
Add composer require react/event-loop to .travis.yml (similar to how we do it with coveralls) + add ReactPHP tests for Travis builds only.

If you do not want to bother with this - just add tests for ReactPHP that you find appropriate - and exclude them in default phpunit.xml.dist. We'll manage Travis stuff when we find time.

P.S. Glad that you are using this async stuff! %)

…added tests

coveralls · 2016-12-22T13:30:54Z

Coverage increased (+0.5%) to 92.418% when pulling e3a864f on lordthorzonus:nested-async-queries-mess-the-indexes-of-arrays into ff3a40d on webonyx:master.

coveralls · 2016-12-22T13:38:17Z

Coverage increased (+0.5%) to 92.418% when pulling 8626e0b on lordthorzonus:nested-async-queries-mess-the-indexes-of-arrays into ff3a40d on webonyx:master.

coveralls · 2016-12-22T13:43:54Z

Coverage increased (+0.5%) to 92.418% when pulling 8626e0b on lordthorzonus:nested-async-queries-mess-the-indexes-of-arrays into ff3a40d on webonyx:master.

lordthorzonus · 2016-12-22T13:52:00Z

@vladar PR updated. Added tests and fixed the all() behaviour.

ReactPromiseAdapterTest is now excluded with phpunit's group feature from the default run (and travis runs them by using the group flag --group default,RectPromise. It's also marked as skipped if somebody tries to run it and hasn't got the react/promise package installed.

vladar · 2016-12-23T08:30:09Z

@lordthorzonus Great work. Thanks!

vladar mentioned this pull request Dec 20, 2016

Normalize array keys when expected return type is List #81

Closed

Juuso Leinonen added 2 commits December 22, 2016 15:08

Changed ReactPromiseAdapter::all to preserve the array key order and …

90c4b5d

…added tests

Make travis run also the ReactPromiseAdapter tests

e3a864f

lordthorzonus force-pushed the nested-async-queries-mess-the-indexes-of-arrays branch from 54ece56 to e3a864f Compare December 22, 2016 13:25

Make tests run on php 5.4

8626e0b

lordthorzonus force-pushed the nested-async-queries-mess-the-indexes-of-arrays branch from b3cf2cd to 8626e0b Compare December 22, 2016 13:35

vladar merged commit 595ae52 into webonyx:master Dec 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested async queries messes up the order of keys in arrays producing unwanted responses #80

Nested async queries messes up the order of keys in arrays producing unwanted responses #80

lordthorzonus commented Dec 20, 2016

coveralls commented Dec 20, 2016

mcg-web commented Dec 20, 2016

vladar commented Dec 20, 2016

lordthorzonus commented Dec 20, 2016 •

edited

Loading

vladar commented Dec 20, 2016

lordthorzonus commented Dec 20, 2016 •

edited

Loading

vladar commented Dec 20, 2016

lordthorzonus commented Dec 20, 2016

vladar commented Dec 20, 2016

vladar commented Dec 21, 2016 •

edited

Loading

mcg-web commented Dec 21, 2016 •

edited

Loading

vladar commented Dec 21, 2016

mcg-web commented Dec 21, 2016

lordthorzonus commented Dec 21, 2016

vladar commented Dec 21, 2016 •

edited

Loading

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

lordthorzonus commented Dec 22, 2016

vladar commented Dec 23, 2016

Nested async queries messes up the order of keys in arrays producing unwanted responses #80

Nested async queries messes up the order of keys in arrays producing unwanted responses #80

Conversation

lordthorzonus commented Dec 20, 2016

coveralls commented Dec 20, 2016

mcg-web commented Dec 20, 2016

vladar commented Dec 20, 2016

lordthorzonus commented Dec 20, 2016 • edited Loading

vladar commented Dec 20, 2016

lordthorzonus commented Dec 20, 2016 • edited Loading

vladar commented Dec 20, 2016

lordthorzonus commented Dec 20, 2016

vladar commented Dec 20, 2016

vladar commented Dec 21, 2016 • edited Loading

mcg-web commented Dec 21, 2016 • edited Loading

vladar commented Dec 21, 2016

mcg-web commented Dec 21, 2016

lordthorzonus commented Dec 21, 2016

vladar commented Dec 21, 2016 • edited Loading

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

coveralls commented Dec 22, 2016

lordthorzonus commented Dec 22, 2016

vladar commented Dec 23, 2016

lordthorzonus commented Dec 20, 2016 •

edited

Loading

lordthorzonus commented Dec 20, 2016 •

edited

Loading

vladar commented Dec 21, 2016 •

edited

Loading

mcg-web commented Dec 21, 2016 •

edited

Loading

vladar commented Dec 21, 2016 •

edited

Loading