Stricter non-null fields for relationships by polgfred · Pull Request #367 · graphql-python/graphene-sqlalchemy · GitHub
Skip to content

Stricter non-null fields for relationships#367

Merged
erikwrede merged 2 commits into
graphql-python:masterfrom
polgfred:non-null-relationships
Jan 13, 2023
Merged

Stricter non-null fields for relationships#367
erikwrede merged 2 commits into
graphql-python:masterfrom
polgfred:non-null-relationships

Conversation

@polgfred

@polgfred polgfred commented Nov 28, 2022

Copy link
Copy Markdown
Contributor

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 Bot commented Nov 28, 2022 •
edited
Loading

Copy link
Copy Markdown

@flipbit03

flipbit03 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor

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

erikwrede commented Nov 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 erikwrede added this to the 3.0 milestone Nov 29, 2022
@erikwrede

erikwrede commented Dec 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

polgfred commented Dec 31, 2022

Copy link
Copy Markdown
Contributor Author

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

erikwrede commented Jan 2, 2023

Copy link
Copy Markdown
Member

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

polgfred commented Jan 6, 2023

Copy link
Copy Markdown
Contributor Author

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

erikwrede commented Jan 6, 2023

Copy link
Copy Markdown
Member

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
erikwrede approved these changes Jan 13, 2023
View reviewed changes

@erikwrede erikwrede left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for getting this started 🙂

@erikwrede erikwrede merged commit d3a4320 into graphql-python:master Jan 13, 2023
@polgfred

polgfred commented Jan 13, 2023

Copy link
Copy Markdown
Contributor Author

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

erikwrede commented Jan 13, 2023

Copy link
Copy Markdown
Member

@erikwrede erikwrede mentioned this pull request Feb 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erikwrede erikwrede erikwrede approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@polgfred @flipbit03 @erikwrede