-
Notifications
You must be signed in to change notification settings - Fork 10
[Draft] Skip null values when serializing in rust #149
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
base: main
Are you sure you want to change the base?
Conversation
254911c
to
269b04c
Compare
/// Uses conservative detection to identify Optional fields from GraphQL schema | ||
fn is_input_field_nullable(ivd: &impl InputValueDefinition) -> bool { | ||
// Use std::any::type_name to get type information as a string | ||
let type_name = std::any::type_name_of_val(&ivd.r#type()); |
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.
Not super familiar with bluejay, but I think if we have something like:
ivd.r#type().is_nullable()
Then we could avoid using this approach. We need to get the type information so we can match correctly. I ran into issues when trying to use matching because some types were required (aka not optional), and some were optional.
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.
Yeah we should be using bluejay
here for the type information
269b04c
to
1b4199b
Compare
// Generate field counting statements for dynamic field count calculation | ||
let field_count_statements: Vec<syn::Stmt> = input_object_type_definition | ||
.input_field_definitions() | ||
.iter() | ||
.map(|ivd| { | ||
let field_name_ident = names::field_ident(ivd.name()); | ||
|
||
if is_input_field_nullable(ivd) { | ||
// For nullable fields, count only if Some(_) | ||
parse_quote! { | ||
context.write_utf8_str(#field_name_lit_str)?; | ||
}, | ||
if let ::std::option::Option::Some(_) = self.#field_name_ident { | ||
field_count += 1; | ||
} | ||
} | ||
} else { | ||
// For required fields, always count | ||
parse_quote! { | ||
self.#field_name_ident.serialize(context)?; | ||
}, | ||
] | ||
field_count += 1; | ||
} | ||
} | ||
}) | ||
.collect(); |
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.
Can we just consolidate this into one line where we pre-compute the number of non-nullable fields and add the rest based on value, e.g. we might end up with:
let field_count: usize = 5 + self.foo.is_some().into() + self.bar.is_some().into();
Addresses: https://github.com/shop/issues-shopifyvm/issues/318
Summary
What this PR does?
Before:
After:
Approach
Testing
Pro's