fix(patch): leave unset fields untouched in update payloads by prasad-albert · Pull Request #561 · albert-labs/albert-python

prasad-albert · 2026-06-24T07:32:02Z

Problem

Patch-payload generators decided delete ops from value is None alone. A field the caller never set defaults to None (or is coerced to [] by getattr(updated, attr) or [] in the task special-attr loop), so omitted fields were read as deletions.

For partial update objects (the dominant Ask Albert agent pattern), this emitted bad delete ops that either:

the API rejects (e.g. "Delete operation not allowed for attribute-name"), or
silently wipe existing data (inventory rows, tags).

Fix

Gate the diff on attribute in updated.model_fields_set:

unset (not in model_fields_set) -> no-op, leave server value
explicit None -> delete (unchanged)
explicit [] / {} -> clear to empty (unchanged)

Non-breaking: the fetch-mutate-update pattern is unchanged (fetched objects have all server fields in model_fields_set); only erroneous deletes of omitted fields stop. Explicit clears still work.

claude · 2026-06-24T07:34:31Z

Code Review

Issues Found

[src/albert/collections/base.py:161] Guard condition if attribute not in updated.model_fields_set and new_value is None is weaker than intended. OPINIONS.md (added in this same PR) states "Gate the diff on attribute in updated.model_fields_set, never on value is None alone" — yet the condition does both, meaning an unset field whose type has a non-None default (e.g. default=[]) will not be skipped and will emit a spurious diff op. tasks.py:286 correctly uses just if attribute not in updated.model_fields_set: continue — base.py should match it and drop the and new_value is None clause.

Summary

1 issue found. The incomplete guard in the shared _generate_patch_payload path contradicts the policy OPINIONS.md introduces and leaves the same class of silent-deletion bug reachable for any future field with a non-None default.

prasad-albert · 2026-06-24T09:07:39Z

Code Review

Issues Found

[src/albert/collections/base.py:161] Guard condition if attribute not in updated.model_fields_set and new_value is None is weaker than intended. OPINIONS.md (added in this same PR) states "Gate the diff on attribute in updated.model_fields_set, never on value is None alone" — yet the condition does both, meaning an unset field whose type has a non-None default (e.g. default=[]) will not be skipped and will emit a spurious diff op. tasks.py:286 correctly uses just if attribute not in updated.model_fields_set: continue — base.py should match it and drop the and new_value is None clause.

Summary

1 issue found. The incomplete guard in the shared _generate_patch_payload path contradicts the policy OPINIONS.md introduces and leaves the same class of silent-deletion bug reachable for any future field with a non-None default.

fixed this issue -- re-review the PR

prasad-albert · 2026-06-24T13:34:15Z

Code Review

Issues Found

[src/albert/collections/base.py:161] Guard condition if attribute not in updated.model_fields_set and new_value is None is weaker than intended. OPINIONS.md (added in this same PR) states "Gate the diff on attribute in updated.model_fields_set, never on value is None alone" — yet the condition does both, meaning an unset field whose type has a non-None default (e.g. default=[]) will not be skipped and will emit a spurious diff op. tasks.py:286 correctly uses just if attribute not in updated.model_fields_set: continue — base.py should match it and drop the and new_value is None clause.

Summary

1 issue found. The incomplete guard in the shared _generate_patch_payload path contradicts the policy OPINIONS.md introduces and leaves the same class of silent-deletion bug reachable for any future field with a non-None default.

fixed this issue -- re-review the PR

@claude re-review this PR, addressed your comments

claude · 2026-06-24T13:34:30Z

Claude finished @prasad-albert's task in 1m 20s —— View job

Code Review

Read the changed files to verify the fix in base.py
Full re-review of all changed files
Post findings

Findings

Previous issue — FIXED. base.py:163 now reads:

if attribute not in updated.model_fields_set:
    continue

The and new_value is None clause is gone. The guard is now correct and consistent with tasks.py:286.

All four guard sites

File	Line	Guard	Verdict
`base.py`	163	`if attribute not in updated.model_fields_set: continue`	✅ Correct
`attachments.py`	129	`if "metadata" not in updated.model_fields_set: return …`	✅ Correct
`inventory.py`	787	`if attribute not in updated.model_fields_set: continue`	✅ Correct
`tasks.py`	286	`if attribute not in updated.model_fields_set: continue`	✅ Correct

All sites are consistent. Unset fields are skipped unconditionally, regardless of their default value.

Tests

test_update_partial_leaves_omitted_fields_untouched (lots) and test_update_partial_leaves_omitted_special_attrs_untouched (tasks) both follow good practice: they assert the precondition ("field" not in partial.model_fields_set) before calling update, then refetch and assert the server-side value was preserved. Clean.

Minor nit (non-blocking)

base.py:159–162 has a 4-line comment block. AGENTS.md calls for one short line max. Not worth a round-trip — the explanation is genuinely non-obvious — but could be trimmed to a single line in a follow-up.

Summary

No issues found. The fix is correct, all guard sites are consistent, and the new tests validate the key scenario. Good to merge.

sumedhb1995

Awesome!! This changes update semantics for anyone who relied on "construct a fresh partial object, call update, and have omitted fields be deleted." Could you please ensure that, at-least internally, no-one was relying on this paradigm? That pattern is unusual (the documented flow is fetch-modify-save), but it is a behavior change nonetheless. I would consider at the very least including this in the some breaking changes or similar change notes to let any external consumers of the SDK know

sumedhb1995

Just a heads up, I think smart_datasets.py defines its own _generate_patch_payload that does not delegate to base and has no model_fields_set guard. That will need its own separate fix

prasad-albert · 2026-06-25T05:50:33Z

Awesome!! This changes update semantics for anyone who relied on "construct a fresh partial object, call update, and have omitted fields be deleted." Could you please ensure that, at-least internally, no-one was relying on this paradigm? That pattern is unusual (the documented flow is fetch-modify-save), but it is a behavior change nonetheless. I would consider at the very least including this in the some breaking changes or similar change notes to let any external consumers of the SDK know

It is a behavior change -- I will be explicitly adding this in the release notes and communicate to internal users.

prasad-albert · 2026-06-25T06:00:01Z

Just a heads up, I think smart_datasets.py defines its own _generate_patch_payload that does not delegate to base and has no model_fields_set guard. That will need its own separate fix

added a fix for this in the same PR

Patch-payload generators decided delete ops from `value is None`, so a field the caller never set was read as a deletion. Partial update objects emitted bad `delete` ops that the API rejects or that silently wiped existing data (inventory rows, tags). Skip any attribute not in `updated.model_fields_set`: an unset field is a no-op, while an explicit `None`/`[]` still deletes/clears. Covers all collections using `_generate_patch_payload` plus task special attrs in `generate_adv_patch_payload`. Adds integration coverage for lots (base path) and tasks (special-attr path).

inventory._generate_inventory_patch_payload iterated tags/acls/cas/company without checking model_fields_set, coercing unset attrs to [] and emitting delete ops for all existing values. attachments._generate_attachment_patch_payload constructed an empty AttachmentMetadata() when metadata was unset, diffing it against the server state and emitting deletes for every existing metadata field. Same model_fields_set guard applied as in base._generate_patch_payload.

…e_patch_payload SmartDataset overrides _generate_patch_payload without delegating to base, so the model_fields_set guard added in base.py did not cover it. Unset fields emitted spurious UPDATE ops with null new_value. Same fix applied.

The API rejects multiple operations on the same list attribute (e.g. Symbols) in a single request with '1 entities duplicated'. Batch scalar ops together and send each list-attribute op (delete/add) individually, matching the pattern used for Units/Synonyms in PR #559.

prasad-albert mentioned this pull request Jun 24, 2026

docs(agents): record unset-vs-empty rule for update payloads #562

Draft

prasad-albert force-pushed the fix/patch-payload-unset-fields branch from f380097 to 3dd1711 Compare June 24, 2026 09:07

prasad-albert mentioned this pull request Jun 24, 2026

fix(units): patch synonyms as item-level add/delete operations #559

Merged

sumedhb1995 approved these changes Jun 25, 2026

View reviewed changes

sumedhb1995 reviewed Jun 25, 2026

View reviewed changes

prasad-albert added 4 commits June 26, 2026 11:07

prasad-albert force-pushed the fix/patch-payload-unset-fields branch from 45dd230 to 48a87bb Compare June 26, 2026 05:38

prasad-albert merged commit 0642e94 into main Jun 26, 2026
1 check passed

prasad-albert deleted the fix/patch-payload-unset-fields branch June 26, 2026 06:02

release-catalyst Bot mentioned this pull request Jun 26, 2026

chore(main): release 1.30.1 #571

Merged

This was referenced Jul 7, 2026

chore(next): release 1.28.0-beta0 #593

Closed

chore(main): release albert 1.33.0 #596

Closed

chore(main): release albert 1.33.0 #601

Closed

chore(main): release albert 1.33.0 #603

Closed

chore(next): release 1.35.0-beta0 #607

Merged

Navigation Menu

Uh oh!

fix(patch): leave unset fields untouched in update payloads - #561

fix(patch): leave unset fields untouched in update payloads#561
prasad-albert merged 4 commits into
mainfrom
fix/patch-payload-unset-fields

prasad-albert commented Jun 24, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jun 24, 2026

Uh oh!

prasad-albert commented Jun 24, 2026

Code Review

Issues Found

Summary

Uh oh!

prasad-albert commented Jun 24, 2026

Code Review

Issues Found

Summary

Uh oh!

claude Bot commented Jun 24, 2026 •

edited

Loading

Uh oh!

sumedhb1995 left a comment

Uh oh!

sumedhb1995 left a comment

Uh oh!

prasad-albert commented Jun 25, 2026

Uh oh!

prasad-albert commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

prasad-albert commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Uh oh!

claude Bot commented Jun 24, 2026

Code Review

Issues Found

Summary

Uh oh!

prasad-albert commented Jun 24, 2026

Code Review

Issues Found

Summary

Uh oh!

prasad-albert commented Jun 24, 2026

Code Review

Issues Found

Summary

Uh oh!

claude Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Findings

All four guard sites

Tests

Minor nit (non-blocking)

Summary

Uh oh!

sumedhb1995 left a comment

Choose a reason for hiding this comment

Uh oh!

sumedhb1995 left a comment

Choose a reason for hiding this comment

Uh oh!

prasad-albert commented Jun 25, 2026

Uh oh!

prasad-albert commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prasad-albert commented Jun 24, 2026 •

edited

Loading

claude Bot commented Jun 24, 2026 •

edited

Loading