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

MySQL date attributes can not be read from paginated HasMany relations #776

Open
quangdac opened this issue May 9, 2019 · 14 comments

Comments

@quangdac
Copy link

commented May 9, 2019

Type

type User {
    id: ID! @rename(attribute: "user_id")
    post: [Post!]! @hasMany(type: "paginator")
}
type Post {
    id: ID! @rename(attribute: "post_id")
    userId: Int @rename(attribute: "user_id")
    fromDate: Date! @rename(attribute: "from_date")
    toDate: Date! @rename(attribute: "to_date")
}

schema.graphql

type Query {
    users: [User!]! @paginate(builder: "App\\GraphQL\\Queries\\UserQuery@resolve")
}

This is mysql table

CREATE TABLE `post` (
  `post_id` bigint(20) NOT NULL AUTO_INCREMENT COMMENT 'ID',
  `user_id` bigint(20) NOT NULL COMMENT 'キャンペーンID',
  `from_date` date NOT NULL DEFAULT '0000-00-00' COMMENT '開始日',
  `to_date` date NOT NULL DEFAULT '0000-00-00' COMMENT '終了日',
  PRIMARY KEY (`t_flight_id`),
  KEY `t_campaign_id` (`user_id`),
) ENGINE=InnoDB AUTO_INCREMENT=147454 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci COMMENT='comment';

Query

{
"query":"{users(count: 4, page: 1){data {id, post(count: 5, page: 1){ data {id, fromDate, toDate}, paginatorInfo{currentPage, lastPage}}}, paginatorInfo{currentPage, lastPage}}}"
}

The message error is The Response content must be a string or object implementing __toString(), "boolean" given.
The condition for an error are type of hasMany directive is paginator and the data type in mysql is Date (not DateTime)

Lighthouse Version: "nuwave/lighthouse": "^3.2",
Laravel Version: 5.6
PHP Version: 7.1.3
Thanks!

@spawnia

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

A quick google search indicates to me that this error message originates from within a Laravel controller. Seems like this issue is not within Lighthouse per se.

Closing this for now, if you find a bug within Lighthouse, feel free to reopen.

@spawnia spawnia closed this May 9, 2019
@spawnia spawnia added the question label May 9, 2019
@quangdac

This comment has been minimized.

Copy link
Author

commented May 10, 2019

@spawnia If I remove 'fromDate' and 'toDate' from client query , the api works normal. Or If I don't set paginator type for hasMany directive, the api also works normal.
I think this bug relate to sql date type and paginator type of hasMany directive. If both conditions occur, there is an error as you know.

@quangdac quangdac changed the title hasMany relationship error occurred when set type = paginator error occurred May 10, 2019
@spawnia

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

If you can come up with a failing unit test and create a PR or pinpoint the exact issue within Lighthouse, we can work on it.

@quangdac

This comment has been minimized.

Copy link
Author

commented May 13, 2019

@spawnia I don't understand why if I don't use hasMany (type = "paginator") then the data returns '2000-22-12', but if using hasMany (type = "paginator"), the returned data is b "à x07 x03 x17". The type of data above is mySql date. I try write the unit test but seem this lib is using SQL for test. So I can't recreate it. My project use MySql. Hope to get your help 🙇

@quangdac

This comment has been minimized.

Copy link
Author

commented May 13, 2019

@spawnia 0

PHP does not have "binary" and "non-binary" strings. It just has strings, and they're always "binary", as they're just acting like byte arrays. The b prefix is added by the Symfony VarDumper component as a sign that the string is not valid UTF-8. Arguably UTF-8 should be the one and only sensible encoding in use today, and apparently Symfony goes so far as to declare anything else as "binary", i.e. atypical text.

@spawnia spawnia reopened this May 13, 2019
@spawnia

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

@quangdac yeah SQLite is pretty essential to keep our test suite running fast.

I think the root cause might be somewhere within \Nuwave\Lighthouse\Execution\DataLoader\ModelRelationFetcher, as there is a different mechanism used for matching paginated relations rather then simple eager loaded relations.

We should be able to implement a database-agnostic test for this:

  • Setup a relation A -> @hasMany(type: "paginator") -> B
  • Add some custom getter to B
  • Assert the getter is called
    If setters are getting called, i am pretty sure that date casts should also run.
@spawnia spawnia added bug and removed question labels May 13, 2019
@spawnia spawnia changed the title error occurred MySQL date attributes can not be read from paginated HasMany relations May 13, 2019
@quangdac

This comment has been minimized.

Copy link
Author

commented May 14, 2019

@spawnia yes i did the steps as you suggested in my project and the getter were called. What should I do next?

@spawnia

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

I don't have any more ideas right now, hopefully you can figure out the root cause of this issue.

@atomita

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I also faced this issue.

The same result was obtained when executing SQL including "union all" like this to mysql using tinker.

>>> \DB::select('(select * from `user_education_histories` where `user_education_histories`.`user_id` in (3) limit 1 offset 0) union all (select * from `user_education_histories` where `user_education_histories`.`user_id` in (10) limit 1 offset 0)')
=> [
     {#3241
       +"id": 5,
       +"user_id": 3,
       +"school": "秋田県西区立学校",
       +"graduated_at": b"Í\x07\x07\x1C",
       +"created_at": "2019-09-05 09:07:59",
       +"updated_at": "2019-09-05 09:07:59",
     },
     {#3242
       +"id": 18,
       +"user_id": 10,
       +"school": "山口県西区立学校",
       +"graduated_at": b"Î\x07\v\x06",
       +"created_at": "2019-09-05 09:07:59",
       +"updated_at": "2019-09-05 09:07:59",
     },
   ]
@spawnia

This comment has been minimized.

Copy link
Collaborator

commented Sep 11, 2019

We are actually running Travis CI on MySQL now, can you add a PR with a failing test case in https://github.com/nuwave/lighthouse/blob/master/tests/Integration/Schema/Directives/HasManyDirectiveTest.php ?

@atomita

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

@spawnia
I added a test case, but the test finished without errors...

What I know now is that in the test environment for this package, the following script got the correct date:
(php v7.2.0)

$pdo = new \PDO('mysql:host=mysql;dbname=test', 'root', '', [\PDO::ATTR_EMULATE_PREPARES => false]);
$stmt = $pdo->query('(select * from `task_target_days` where `task_target_days`.`task_id` in (1) limit 2 offset 0) union all (select * from `task_target_days` where `task_target_days`.`task_id` in (2) limit 2 offset 0)');
var_dump($stmt->fetch());

But in my environment, a similar script broke the date value.
(php v7.2.0)

atomita added a commit to atomita/lighthouse that referenced this issue Sep 13, 2019
@spawnia

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2019

Maybe the MySQL version plays a role?

@quangdac

This comment has been minimized.

Copy link
Author

commented Oct 4, 2019

@spawnia , @atomita
About the bugs, we have just found the root cause as below, if you have any question, please ask us:
Cause:
When multi-pagination is performed, lighthoust lib combines queries with the "union all" command.
Originally the 'date' field is in PHP NEWDATE format. This format is handled by Hexadecimal.
How to respond:
config:
PDO :: ATTR_EMULATE_PREPARES = true
the purpose:
The 'date' field is processed as a String. Please refer to the following link.
laravel/framework#19467 (comment)
risk:
Other 'date' fields querying 'union all' can be affected

@spawnia

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2019

Hey, great job for digging into it. Can you detail the proposed fix? Where would that config go?

Is there something we can do from Lighthouse's side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.