Skip to content

[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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mssalemi
Copy link
Contributor

@mssalemi mssalemi commented Jun 27, 2025

Addresses: https://github.com/shop/issues-shopifyvm/issues/318

Summary

  • Rust functions were outputting null fields like "attributes": null, "image": null, "price": null in JSON responses, making them larger than necessary

What this PR does?

  • Skips null fields: Optional fields with None values are no longer included in the serialized output
  • Keeps required fields: Non-optional fields are always serialized as before
  • Dynamic field counting: Adjusts msgpack field count based on actual fields being serialized

Before:

  {
    "attributes": null,
    "image": null,
    "price": null,
    "title": "Something that is wrapped"
  }

After:

  {
    "title": "Something that is wrapped"
  }

Approach

  • Analyzes GraphQL schema at compile time to detect nullable vs required fields
  • conditionally serialization for optional fields (if Some(value) { serialize })
  • Two-pass msgpack serialization (count fields, then serialize) to maintain format

Testing

  • All existing tests pass (no breaking changes)
  • New test added to verify null field skipping behavior
  • Backwards compatible with existing function implementations

Pro's

  • Smaller JSON output size
  • Cleaner, more predictable function responses

@mssalemi mssalemi force-pushed the ms.handle-null-fields-wasm-api branch from 254911c to 269b04c Compare June 27, 2025 14:33
/// 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());
Copy link
Contributor Author

@mssalemi mssalemi Jun 27, 2025

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.

Copy link
Contributor

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

@mssalemi mssalemi force-pushed the ms.handle-null-fields-wasm-api branch from 269b04c to 1b4199b Compare June 27, 2025 15:18
Comment on lines +545 to 566
// 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();
Copy link
Contributor

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();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants