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

Nested Sorting: interface by non-derived child entity #4058

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

kamilkisiela
Copy link
Contributor

@kamilkisiela kamilkisiela commented Oct 12, 2022

Sorting an interface on a child basis.

  • Fix: Kamil, please fix me :(
  • Replace use_sort_key_alias boolean with something

@lutter
Copy link
Collaborator

lutter commented Oct 14, 2022

Is this ready for review or should I wait until you've done your refactor?

@kamilkisiela
Copy link
Contributor Author

@lutter wait until refactor, I will @mention you

@kamilkisiela kamilkisiela force-pushed the kamil-sorting-interface-by-child-type branch from dfc9ca9 to 570c71c Compare November 4, 2022 10:19
@kamilkisiela kamilkisiela force-pushed the kamil-sorting branch 2 times, most recently from 4b0a0f5 to 1d343e1 Compare January 12, 2023 12:24
@neysofu neysofu mentioned this pull request Feb 13, 2023
2 tasks
Base automatically changed from kamil-sorting to master February 14, 2023 01:32
@kamilkisiela kamilkisiela force-pushed the kamil-sorting-interface-by-child-type branch 5 times, most recently from aefeff1 to e96099f Compare March 21, 2023 08:34
@neysofu neysofu self-assigned this Jun 1, 2023
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I like how you avoided passing that somewhat mysterious boolean flag around.

To me, the most important thing is that this won't change the behavior of queries that work today - I think that is true, and I don't have a great idea how we could test that more rigorously. The existing query tests should ensure that.

This is good to be merged after a rebase, and addressing the comment about making sorting by an interface where the field has mixed storage in implementers an error. I think it would lead to a SQL error anyway, but an explicit error would be clearer.

}
// Finds the field that connects the parent entity with the child entity.
// In the case of an interface, we need to find the field on one of the types that implement the interface,
// as the `@derivedFrom` directive is only allowed on object types.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crazy thing here is that AFAIK it's possible for different object types that implement the same interface to store that attribute directly or derived. I have no idea how well that actually works in practice, but I don't think we enforce anything there.

I don't think we should hold this PR up any longer because of that, but it's likely that this will lead to issues if anybody uses that somewhat crazy mix of derived and direct storage. I have no idea if anybody does that, and we should have a look at the specific subgraph if that ever leads to issues. It might be a good idea though to require that all implementers of the interface use the same storage and return an error otherwise just to make sure we don't inadvertently return incorrect data.

@kamilkisiela kamilkisiela force-pushed the kamil-sorting-interface-by-child-type branch from c803b11 to ea762af Compare June 27, 2023 13:57
@dotansimha dotansimha requested a review from leoyvens June 28, 2023 06:24
@leoyvens leoyvens merged commit ba5f12f into master Jul 19, 2023
@leoyvens leoyvens deleted the kamil-sorting-interface-by-child-type branch July 19, 2023 19:21
Copy link

@gatleas17 gatleas17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi

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.

6 participants