Closed polgfred closed 1 year ago
Base: 96.34% // Head: 96.36% // Increases project coverage by +0.02%
:tada:
Coverage data is based on head (
282cc69
) compared to base (2041835
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I really like this idea and I think it makes sense, given that the SQLAlchemy relationships (be it a vector or a scalar) always return the T
inside them, there's not really any None
's in those.
This really makes sense. But we need another option to check for nullability of :1 relationships first. The method you used isn't guaranteed to work (just like you mentioned in your comment).
Additionally, we should also change Relay's behavior (connectionFields and nodes) to keep everything nice and consistent.
Another concern I have is future support of the @defer
directive which is going to be released in graphql-js soon, meaning that graphql-python will follow. At some point, deferring was only supported on nullable fields. Since deferring makes a lot of sense on relationships to improve response times, we should see if the new spec might impact the design in graphene-sqlalchemy.
Here's a snapshot from the apollo-server
docs. They already support deferring results. Curious to see how the reference implementation will handle this.
https://github.com/apollographql/apollo-server/blob/defer-support/docs/source/defer-support.md#caveats-regarding-defer-usage
EDIT:
Looks like the reference implementation allows deferring NonNull
fragments, in spite of the points listed on the apollo server docs:
Will have a more detailed look at that this week!
I haven't found an elegant approach to the to-one
relationship nullability check so far. We could use the column containing the foreign key and check column.nullable
. However, I couldn't find a way to get that column name from the relationship property, other than manually parsing the A.primary_key == B.foreign_key
expression in the relationship prop. Any ideas?
Models that rely on the foreign on the other model still won't benefit from this, as a required foreign key on the joined model doesn't imply anything about the existance of a model to join. So design-wise it's hard to use a general rule for both sides here, and the only solution I see are user-declared ORMFields for each to-one
relationship with nullable=False
.
We should discuss if it makes sense to include the to-one relationships in this PR if there is no clearer approach for both sides of the relationship.
We should discuss if it makes sense to include the to-one relationships in this PR if there is no clearer approach for both sides of the relationship.
@erikwrede Yeah, the to-many relationship scenario is much more straightforward. I'd be ok punting on that. I haven't dug into it more than I did in the original PR, but what I came up with was unfortunately the most elegant approach I could come up with. :)
Okay, let's push back the to-one case and focus on to-many relationships here!
We're still missing
When that is done, I'll merge it for 3.0 π
When that is done, I'll merge it for 3.0
So in that case, should I remove the use_non_nullable_relationships
flag, and just make this behavior the default?
I believe we should make the NonNull
behavior the default but still give users the choice in case they want to be fully relay-spec compliant. The flag is a great compromise for that.
LGTM! Thanks for getting this started π
Thanks! I wasn't sure if there were some other more comprehensive unit tests you wanted, but I added ones to cover the changes.
The logic behind this change is quite simple and there shouldn't be any complex collisions, so making change the new behavior works and the legacy behavior should suffice in this case. π
Following on the heels of my first successful PR π, I wanted to float another suggestion for your consideration. It seems like it would be closer to ORM semantics if:
user_id
) is non-null, the relationship field should be alsoFor example:
The Schema should be:
This should probably be the default (version 3 maybe?), but in the interests of backward compatibility, we can make it an opt-in feature. This would help my UI team immensely by cleaning up the schema, and not having to deal with unnecessary
null
s all over the place.Obviously more tests and docs would be forthcoming, but I wanted to float the idea first.
cc: @erikwrede @flipbit03