-
Notifications
You must be signed in to change notification settings - Fork 213
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
feat: extend WhereUnique for non unique filters #3281
Conversation
f544312
to
a60059b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small feedback. But this is looking really good.
Once the tests are passing I think we good to go.
@@ -11,7 +11,7 @@ mod where_unique { | |||
&runner, | |||
"query { findUniqueUser(where: {}){ id }}", | |||
2009, | |||
"Expected exactly one field to be present, got 0." | |||
"Expected a minimum of 1 fields of (id, email, first_name_last_name) to be present, got 0." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you rename this test file to extended_where_unique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "extended where" is a temporary name that we use to qualify the feature we're building. But those tests are for a where unique. Once we remove the feature flag, we'll refer to this as where unique again. Why do you think we should rename the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of extended where because these sets of tests are around that concept. yes its just normal where but if I came to the code base and looked at a where_unique
test I would think its a little strange to be there. But if we have the extended_where_unique
it gives me context. There is something related to a new feature that was added later to the where_unique
functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if I came to the code base and looked at a where_unique test I would think its a little strange to be there
I'm sorry but I don't understand why you'd find that strange. This where_unique.rs
test file already existed. All I did was add a couple more tests to it. In 2 months, we'll GA and remove this feature flag and extended_where_unique won't make any sense anymore. It won't be easier either from a visibility/searchability perspective because we'll have where unique tests spread in two different files for no obvious reasons because extendedWhereUnique won't be a concept anymore. It does make sense right now but think about onboarding developers 2 years from now without any of this context.
query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/filters/where_unique.rs
Show resolved
Hide resolved
de8140e
to
76eca78
Compare
Overview
extendedWhereUnique
WhereUniqueInput
sWhereInput
s to nested delete/disconnect/update for to-one relationSchema changes
Considering the following datamodel,
Here are the additions:
This new WhereUniqueInput affect all reads and writes that uses it, such as:
Furthermore, on one-to-one relation, it adds the ability to add additional filters via a
WhereInput
(since the record is pinned by the QE). All examples below aren't possible without theextendedWhereUnique
feature flag: