Skip to content

Conversation

@Xyedo
Copy link

@Xyedo Xyedo commented Aug 4, 2025

Fixes: #1146

Add #[allow_missing] in the field attribute for DeserializeRow macro both on enforce_order flavor and match_by_name flavor

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@Xyedo Xyedo force-pushed the partial-DeserializeValue branch from 67e73b9 to 820386a Compare August 4, 2025 10:34
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Aug 4, 2025
@github-actions
Copy link

github-actions bot commented Aug 4, 2025

cargo semver-checks found no API-breaking changes in this PR.
Checked commit: fd9db3f

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds an allow_missing attribute to struct fields for both row and value deserialization, allowing fields to be initialized with Default::default() when missing from the database instead of failing. This feature provides compatibility with sqlx's #[sqlx(default)] attribute for smoother migration to scylla-rust-driver.

  • Implements #[scylla(allow_missing)] attribute for both struct-level and field-level usage
  • Adds support for the attribute in both row and value deserialization contexts
  • Includes comprehensive test coverage for the new functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
scylla-macros/src/lib.rs Adds documentation for the new allow_missing attribute
scylla-macros/src/serialize/value.rs Adds ignored allow_missing attribute parsing for value serialization
scylla-macros/src/serialize/row.rs Adds ignored allow_missing attribute parsing for row serialization
scylla-macros/src/deserialize/value.rs Implements allow_missing logic for value deserialization
scylla-macros/src/deserialize/row.rs Implements allow_missing logic for row deserialization
scylla/tests/integration/macros/hygiene.rs Adds hygiene tests for the new attribute
scylla-cql/src/deserialize/row_tests.rs Adds comprehensive tests for allow_missing functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Xyedo Xyedo force-pushed the partial-DeserializeValue branch from 820386a to 7091295 Compare September 1, 2025 09:52
@Xyedo Xyedo force-pushed the partial-DeserializeValue branch from 7091295 to 74184aa Compare September 9, 2025 02:56
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Please rebase on main to make semver checks work again.

Comment on lines 185 to 189
#[test]
fn test_struct_deserialization_loose_ordering_allow_missing() {
#[derive(DeserializeRow, PartialEq, Eq, Debug)]
#[scylla(crate = "crate")]
#[scylla(allow_missing)]
Copy link
Collaborator

@Lorak-mmk Lorak-mmk Oct 16, 2025

Choose a reason for hiding this comment

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

Please let's do one thing in this PR. Other field attributes can't be used on a struct, so let's remove this ability from this one as well, at least for now. It will also simplify this PR a lot.

Copy link
Author

Choose a reason for hiding this comment

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

sorry, i don't understand what your intention is, is it start with scylla[allow_missing] field attributes in this PR, and put scylla[allow_missing] in struct attributes in another (next) PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is how understand this. Also, if we choose to allow this attribute to be put on a struct, then we want to allow others as well. Therefore, such another PR would necessarily by larger.
For now, not to impede the per-field attribute, let's strip this PR and get it merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, i don't understand what your intention is, is it start with scylla[allow_missing] field attributes in this PR, and put scylla[allow_missing] in struct attributes in another (next) PR?

In another PR, or maybe not at all - we will have to decide if we want to include such functionality in the driver.
I see "using field attributes on a struct" as a totally separate matter from "using allow_missing in DeserializeRow", so I think it should not be put in a single commit or PR.

Copy link
Author

Choose a reason for hiding this comment

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

Hi! sorry for the delay, removing struct attribute and add enforce_order functionalities based on DeserializeValue code is done in my latest commit

@Lorak-mmk Lorak-mmk added the waiting-on-author Waiting for a response from issue/PR author label Oct 16, 2025
@Xyedo Xyedo force-pushed the partial-DeserializeValue branch from 74184aa to 27a4fa3 Compare October 31, 2025 03:15
@github-actions github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Oct 31, 2025
@Xyedo Xyedo force-pushed the partial-DeserializeValue branch from 27a4fa3 to 4c8defe Compare October 31, 2025 03:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

///
/// #[scylla(allow_missing)]
///
/// if set, implementation will not fail if some columns are missing.
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

Inconsistent capitalization: sentence should start with a capital letter. Change 'if set' to 'If set'.

Suggested change
/// if set, implementation will not fail if some columns are missing.
/// If set, implementation will not fail if some columns are missing.

Copilot uses AI. Check for mistakes.
///
/// #[scylla(allow_missing)]
///
/// if set, implementation will not fail if some columns are missing.
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

Inconsistent capitalization: sentence should start with a capital letter. Change 'if set' to 'If set'.

Suggested change
/// if set, implementation will not fail if some columns are missing.
/// If set, implementation will not fail if some columns are missing.

Copilot uses AI. Check for mistakes.
Comment on lines 516 to 517
/// If the value of the field received from DB is null, the field will be
/// initialized with `Default::default()`.
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The documentation is incorrect. For DeserializeValue, the allow_missing attribute should handle missing fields in the UDT definition, not null values. The description should say 'If the UDT definition does not contain this field' similar to the field-level documentation at line 528.

Suggested change
/// If the value of the field received from DB is null, the field will be
/// initialized with `Default::default()`.
/// If the UDT definition does not contain this field, it will be initialized
/// with `Default::default()`.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🌱 Hmm, perhaps Copilot is right? Perhaps a better name would be allow_null? cc @Lorak-mmk

Copy link
Author

Choose a reason for hiding this comment

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

this is allow_missing struct attributes docs that i forgot to delete, mb!

add allow_missing field attribute with macro DeserializeRow to handle partial deserialization
@Xyedo Xyedo force-pushed the partial-DeserializeValue branch from 4c8defe to fd9db3f Compare October 31, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-author Waiting for a response from issue/PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to do partial deserialization

3 participants