Skip to content

refactor(migrations): reduce instance attributes across migration helpers#835

Open
renansuaris wants to merge 2 commits into
redis:mainfrom
renansuaris:refator/too-many-instance-attributes
Open

refactor(migrations): reduce instance attributes across migration helpers#835
renansuaris wants to merge 2 commits into
redis:mainfrom
renansuaris:refator/too-many-instance-attributes

Conversation

@renansuaris
Copy link
Copy Markdown

@renansuaris renansuaris commented May 29, 2026

What does this PR do?

Refactors the migration-related classes to reduce the number of instance attributes and satisfy Pylint's R0902 warning ("too-many-instance-attributes"). The changes extract or remove redundant state so each class keeps only the data it actually needs.

Why is this change needed?

Some migration classes were accumulating execution state, progress tracking, and metrics in the same object, which made them harder to read, test, and maintain. Reducing instance attributes improves cohesion, simplifies the class responsibilities, and keeps the code within the project's linting rules without changing external behavior.

How was it tested?

Existing tests were run and continue to pass.

Notes for reviewers

This refactor is intentionally minimal and isolated to the instances flagged by Pylint. The public behavior of the migrations should remain unchanged.


Note

Medium Risk
Large structural refactors in FindQuery, query execution, and index schema generation touch core query paths; migration hash processing was rewritten into new helpers but should be behavior-equivalent if tests pass.

Overview
This PR restructures migration and ORM code to cut per-class instance attribute counts (Pylint R0902) by grouping state and extracting helpers, without changing intended public behavior.

Datetime hash migration drops redundant counters (enable_resume on the instance, legacy _processed_keys / _converted_fields) and moves hash scanning, byte normalization, and per-key conversion into _collect_hash_keys, _normalize_hash_data, and _process_hash_key, which _process_hash_model now calls.

Data migrator monitoring pulls verbose logging and result dict assembly into _log_pending_migrations, _build_monitoring_result, and _log_monitoring_summary so run_migrations_with_monitoring is thinner.

model.py is the bulk of the diff: FindQuery splits query inputs and caches into _FindQueryState / _FindQueryCache (properties preserve the old attribute API); execute splits into _build_execute_args, _execute_command, and _parse_execute_results; RediSearch resolve_value delegates to type-specific static helpers; FieldInfo Redis metadata lives in _redis_options; VectorFieldOptions optional params move to VectorFieldParameters; DefaultMeta uses ClassVar; ModelMeta.__new__ is decomposed into static steps; JSON index schema_for_type recursion is split into _schema_for_container_type, _schema_for_embedded_model, and _schema_for_leaf_type.

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

Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 and found 2 potential issues.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 1c3a4b2. Configure here.

Comment thread aredis_om/model/model.py
model=model,
knn=knn,
offset=offset,
limit=limit if limit is not None else (knn.k if knn else DEFAULT_PAGE_SIZE),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semantics of limit falsy check changed subtly

Low Severity

The old code used limit or (self.knn.k if self.knn else DEFAULT_PAGE_SIZE) which treated any falsy limit (including 0) as "use default." The new code uses limit if limit is not None else (knn.k if knn else DEFAULT_PAGE_SIZE) which only falls back for None, allowing limit=0 to pass through. This changes the LIMIT clause sent to RediSearch from the default page size to 0, potentially returning no results where the old code would have returned results.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1c3a4b2. Configure here.

Comment thread aredis_om/model/model.py

@property
def _model_cache(self):
return self._cache.model_cache
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unnecessary indirection wrapping attributes in dataclass containers

Low Severity

The FindQuery refactoring introduces _FindQueryState and _FindQueryCache dataclasses plus ~15 property definitions (getters and setters) that add ~120 lines of boilerplate indirection. The attributes still live on the same object conceptually — they're just accessed through an extra layer. This doesn't improve cohesion or testability; it only suppresses the lint warning while making the class harder to read and maintain.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1c3a4b2. 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.

1 participant