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

Avoid erasing the model information from the wrapping paginated results type when defining a paginated @hasMany field after a field with @paginate #1149

Merged
merged 9 commits into from
Jan 17, 2020

Conversation

MilesWuCode
Copy link
Contributor

@MilesWuCode MilesWuCode commented Jan 16, 2020

  • Added or updated tests
  • Added Docs for all relevant versions
  • Updated CHANGELOG.md

Resolves #1144

tests/Integration/Schema/Directives/HasManyDirectiveTest.php

  • testCanQueryHasManyPaginatorBeforeQuery
  • testCanQueryHasManyPaginatorAfterQuery
  • testCanQueryHasManyNoTypePaginator

@spawnia
Copy link
Collaborator

spawnia commented Jan 16, 2020

Found a fix: Prepending the type name of the parent object to the generated paginated type does the trick.

Not sure if that is the optimal solution though - it's a breaking change in the schema. I will take a deeper look later.

@codeclem
Copy link
Contributor

codeclem commented Jan 17, 2020

I was having this issue as well (see #1147) and I stumbled upon a workaround by specifying the model in the Type schema. Ie:

type Query {
  series: [Series!]! @paginate(defaultCount: 10, model: "App\\Series")
  series(id: ID @eq): Series @find

  episodes: [Episode!]! @paginate(defaultCount: 10, model: "App\\Episodes")
  episode(id: ID @eq): Episode @find
}

That gets rid of the error, but I wouldn't call this a fix either. It seems odd that this would even have an effect when Lighthouse is already configured to search the App namespace and isn't having any issues finding models otherwise. But maybe this helps you find the problem.

@spawnia
Copy link
Collaborator

spawnia commented Jan 17, 2020

It seems like i found the proper fix.

What happened is that:

  1. The field with @paginate generated a type TaskPaginator that also has the @modelClass directive attached. The functionality in @paginate requires that to know the model class later.
  2. The field with the @hasMany paginator overwrote the type TaskPaginator but without the @modelClass. That by itself works, as the model class is not required here.
  3. Querying for the field with @paginate now fails, as the model class is no longer known.

I fixed it by always adding the @modelClass directive to the generated paginator type.

Edit: Oops, looks like that breaks one other bit of functionality - one where you would have a different type than a model in a paginated @hasMany. I will have to do something different.

@spawnia spawnia changed the title HasMany: 3 tests for type paginator Avoid erasing the model information from the wrapping paginated results type when defining a paginated @hasMany field after a field with @paginate Jan 17, 2020
@spawnia spawnia merged commit dba67b9 into nuwave:master Jan 17, 2020
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

Successfully merging this pull request may close these issues.

No class 'TodoPaginator' was found for directive 'paginate'
3 participants