Skip to content

Optimize FindQuery.update() with partial writes - #843

Open
lh0156 wants to merge 10 commits into
redis:mainfrom
lh0156:agent/find-query-update-777
Open

Optimize FindQuery.update() with partial writes#843
lh0156 wants to merge 10 commits into
redis:mainfrom
lh0156:agent/find-query-update-777

Conversation

@lh0156

@lh0156 lh0156 commented Jul 26, 2026

Copy link
Copy Markdown

Summary

Implements #777 by updating matching records without materializing full Pydantic models.

What changed

  • Uses FT.SEARCH ... NOCONTENT to collect matching keys page by page.
  • Validates and serializes update values once, including Pydantic constraints and Redis OM storage conversions.
  • Uses partial HSET mappings for HashModel.
  • Uses path-level JSON.SET commands for JsonModel, including nested __ paths.
  • Preserves the use_transaction flag and returns the number of updated records.
  • Keeps query projections out of the key-only search and preserves KNN state when copying queries.
  • Documents the new return value.

Testing

  • pytest -q tests --ignore tests/test_benchmarks.py: 265 passed.
  • pytest -q tests/test_hash_model.py tests/test_json_model.py: 147 passed.
  • Generated sync tests for the new coverage plus Hash/JSON model tests: 157 passed.
  • ruff check and ruff format --check on changed source/tests.
  • python -m compileall -q aredis_om tests.
  • bandit -q -r aredis_om/model/model.py -s B608.

Redis Stack was run locally via Docker Compose for the integration tests; benchmark tests were excluded from the full run.


Note

Medium Risk
Bulk updates now bypass per-instance save() and use direct Redis writes, so behavior depends on the new serialization/validation path and pipeline semantics; hash field TTL preservation adds extra Redis calls on supported servers.

Overview
FindQuery.update() no longer loads full models and calls save() per match. It validates and serializes kwargs once, pages FT.SEARCH with NOCONTENT to collect keys (ignoring .only() projections), then applies partial HSET mappings for HashModel or JSON.SET on $. paths (including __ nested paths) for JsonModel in a pipeline. use_transaction is honored, FindQuery.dict() now copies knn, and the method returns the number of updated records (0 when nothing matches or Hash updates would be empty).

Hash bulk updates re-apply HEXPIRE for updated fields when Redis 7.4+ field TTLs were set. Docs show the return value; new unit and integration tests cover validation, pagination, JSON paths, and TTL preservation.

Reviewed by Cursor Bugbot for commit 0363c8a. Bugbot is set up for automated code reviews on this repo. Configure here.

@lh0156
lh0156 marked this pull request as ready for review July 30, 2026 09:12
Comment thread aredis_om/model/model.py
Comment thread aredis_om/model/model.py Outdated
@abrookins

Copy link
Copy Markdown
Collaborator

Thanks for opening, @lh0156! I'll take a look ASAP.

Comment thread aredis_om/model/model.py
Comment thread aredis_om/model/model.py
return {
key: ("1" if value else "0") if isinstance(value, bool) else value
for key, value in document.items()
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bool-to-string conversion unreachable after jsonable_encoder processes integers

Medium Severity

In _serialize_update_values for HashModel, the jsonable_encoder call returns booleans as-is because isinstance(False, int) is True in its (str, int, float, type(None)) check. However, the subsequent boolean-to-string conversion on line 2108 checks isinstance(value, bool), which works correctly for Python bool objects. The real issue is that jsonable_encoder processes dict values recursively and can convert certain nested or complex typed booleans inconsistently depending on the Pydantic model's encoder configuration. More importantly, updating a boolean field to False produces "0", which after if value is not None passes through but could be confused with integer 0 by downstream code. This inconsistency between int(0) staying as 0 and bool(False) becoming "0" could cause subtle data type mismatches on read-back if fields are typed as int but store boolean-like values.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 46ec114. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 0363c8a. Configure here.

Comment thread aredis_om/model/model.py
for key in keys:
pipeline.hset(key, mapping=serialized_values)
for field_name, ttl in preserved_ttls.get(key, {}).items():
pipeline.hexpire(key, ttl, field_name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing Field expire on update

High Severity

FindQuery.update() only restores existing positive hash field TTLs after HSET and never applies Field(expire=N) defaults. The old path called save(), which falls back to those defaults when a field has no TTL. After a field expires and Redis deletes it, a later bulk update can recreate the field with no expiry, so values such as session tokens may never expire again.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0363c8a. Configure here.

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