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
fix: batching should work with non unique scalar filters #3328
Conversation
r#"query {findUniqueArtist(where:{firstName_lastName_birth:{firstName:"Michael",netWorth:"236600000.12409"}}) {firstName netWorth}}"#.to_string(), | ||
r#"query {findUniqueArtist(where:{firstName_lastName_birth:{firstName:"George",netWorth:"-0.23660010012409"}}) {firstName netWorth}}"#.to_string(), | ||
r#"query {findUniqueArtist(where:{firstName_netWorth:{firstName:"Michael",netWorth:"236600000.12409"}}) {firstName netWorth}}"#.to_string(), | ||
r#"query {findUniqueArtist(where:{firstName_netWorth:{firstName:"George",netWorth:"-0.23660010012409"}}) {firstName netWorth}}"#.to_string(), |
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.
This test was added by #3210. It started failing in my PR. I'm not sure how it's ever been able to work. The compound field name was clearly wrong since day 1, I don't get it 😅...
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.
Ohhh, I get it now. The previous algorithm would not validate the fields against the schema. It would simply check if any of the filters were objects and if so, assume it's a compound unique (because no other kind of filters were possible). In this case, it would take every nested filter individually to put it back in a findMany query. eg:
before:
findUnique(where: { firstName_lastName_bith: { firstName: "A", netWorth: 12 } })
after:
findMany(where: { firstName: "A", netWorth: 12 })
This essentially means the compound unique field name was never sent to the schema validation, which is why it worked.
With this new PR though, we use the query schema to find out what kind of filters we're dealing with more safely.
Hence why it's now failing, because it cannot find firstName_lastName_birth
in the query schema as a scalar or compound unique field, so it assumes we cannot compact the queries (because it thinks it's a relation filter), so it sends both queries individually and the schema validation kicks in, saying that firstName_lastName_birth
doesn't exist.
Shows the new logic is safer, that's cool.
Self::List(list) => { | ||
let mut values = vec![]; | ||
|
||
for item in list { | ||
if let Some(pv) = item.into_value() { | ||
values.push(pv) | ||
} else { | ||
return None; | ||
} | ||
} | ||
|
||
Some(PrismaValue::List(values)) | ||
} |
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.
This is needed for 👇, line 55.
prisma-engines/query-engine/core/src/response_ir/mod.rs
Lines 47 to 63 in eaaec29
pub fn index_by(self, keys: &[String]) -> Vec<(Vec<QueryValue>, Map)> { | |
let mut map = Vec::with_capacity(self.len()); | |
for item in self.into_iter() { | |
let inner = item.into_map().unwrap(); | |
let key: Vec<QueryValue> = keys | |
.iter() | |
.map(|key| inner.get(key).unwrap().clone().into_value().unwrap()) | |
.map(QueryValue::from) | |
.collect(); | |
map.push((key, inner)); | |
} | |
map | |
} |
One of the new tests showed that scalar lists were failing because it couldn't convert serialized scalar lists back to prisma values.
This piece of code handles that case.
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.
Thanks for the clarification, makes sense!
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.
nice fix
/// Invalid filters are: | ||
/// - non scalar filters (ie: relation filters, boolean operators...) | ||
/// - any scalar filters that is not `EQUALS` | ||
fn operation_contain_invalid_filter(op: &Operation, schema: &QuerySchemaRef) -> bool { |
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 name for this function isn't quite right. I think it needs to be something like invalid_compact_filter
or can_use_filter_with_compact
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.
This was a great learning opportunity, code looks good 👍, I only left some actionable feedback in one of the comments about reordering instructions.
/// Returns `true` if the batch document is [`Compact`]. | ||
#[must_use] | ||
pub fn is_compact(&self) -> bool { | ||
matches!(self, Self::Compact(..)) | ||
} |
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.
Out of curiosity, nothing actionable.
As far as I can see, this function is used in tests. Is there a reason why we prefer to define these type of helpers in the code rather than tests, when they use public modules like BatchDocument::Compact
?
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.
Tbh, there's no real reason. It's true that I could've used assert!(matches!(doc, BatchDocument::Compact(_)))
. On the other hand, I don't feel like leaking this kind of this API should ever hurt us. If you think otherwise, please let me know
// we do not compact the queries. | ||
let has_invalid_filters = operations | ||
.iter() | ||
.any(|op| Self::operation_contain_invalid_filter(op, schema)); |
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.
To avoid unnecessary computation, would it make sense to move this above has_identical_selection_sets
return false if it has invalid filters?
/// Takes in a unique filter, extract the scalar filters and return a simple list of field/filter. | ||
/// This list is used to build a findMany query from multiple findUnique queries. | ||
/// Therefore, compound unique filters walked and each individual field is added. eg: | ||
/// { field1_field2: { field1: 1, field2: 2 } } -> [(field1, 1), (field2, 2)] | ||
/// This is because findMany filters don't have the special compound unique syntax. | ||
/// | ||
/// Furthermore, this list is used to match the results of the findMany query back to the original findUnique queries. | ||
/// Because of that, we only extract EQUALS filters or else we would have to manually implement other filters. | ||
/// This is a limitation that _could_ technically be lifted but that's not worth it for now. | ||
fn extract_filter(where_obj: IndexMap<String, QueryValue>, model: &ModelRef) -> Vec<(String, QueryValue)> { |
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.
Thanks for adding good documentation! ✨ 🎩
Self::List(list) => { | ||
let mut values = vec![]; | ||
|
||
for item in list { | ||
if let Some(pv) = item.into_value() { | ||
values.push(pv) | ||
} else { | ||
return None; | ||
} | ||
} | ||
|
||
Some(PrismaValue::List(values)) | ||
} |
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.
Thanks for the clarification, makes sense!
585578d
to
f8789c2
Compare
Overview
fixes prisma/prisma#15934
With the extension of where unique inputs, which now allow non-unique filters, the batching logic was broken. Here are the new rules:
A batched query is compacted, only if:
{ field: <val> }
or{ field: { equals: <val> } }
{ field1_field2: { field1: <val>, field2: <val> } }
If the condition above is not fulfilled, then we fall back to a simple batch, which executes all findUnique queries separately.
Concretely, this means:
{ deletedAt: null }