Stricter non-null fields for relationships

[polgfred]

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:

• For to-one relationships, if the ID field (e.g. user_id) is non-null, the relationship field should be also
• For to-many relationships, both the list element type and the list field itself should be non-null, so that there is always a list for a many relationship (even if it is empty), and it never contains None

For example:

    class User(Base):
        comments = relationship("Comment")

    class Comment(Base):
        user_id = Column(Integer(), nullable=False)
        user = relationship("User")

The Schema should be:

    type User {
        comments: [Comment!]!
    }

    type Comment {
        user_id: Int!
        user: User!
    }

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 nulls all over the place.

Obviously more tests and docs would be forthcoming, but I wanted to float the idea first.

cc: @erikwrede @flipbit03

[codecov]

Codecov Report

Base: 96.34% // Head: 96.36% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (282cc69) compared to base (2041835).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   96.34%   96.36%   +0.02%     
==========================================
  Files           9        9              
  Lines         903      908       +5     
==========================================
+ Hits          870      875       +5     
  Misses         33       33              

Impacted Files	Coverage Δ
graphene_sqlalchemy/converter.py	95.81% <100.00%> (+0.08%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[flipbit03]

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.

[erikwrede]

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:

https://github.com/graphql/graphql-js/blob/ee0ea6c35be30fa6f0ddd7a2740014fd1cc83d9e/src/execution/__tests__/defer-test.ts#L460-L496

Will have a more detailed look at that this week!

[erikwrede]

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.

[polgfred]

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. :)

[erikwrede]

Okay, let's push back the to-one case and focus on to-many relationships here!

We're still missing

• Test Coverage for the newly added lines
• Additional tests to check the generated types in both cases (NonNull, Nullable)

When that is done, I'll merge it for 3.0 🙂

[polgfred]

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?

[erikwrede]

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.

[erikwrede]

LGTM! Thanks for getting this started 🙂

[polgfred]

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.

[erikwrede]

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. 😊