redis-om-python icon indicating copy to clipboard operation
redis-om-python copied to clipboard

Allow users to define a new primary key.

Open wiseaidev opened this issue 1 year ago • 2 comments

Related to #251

Only 2 tests cases failed:

=========================== short test summary info ============================
FAILED tests/test_hash_model.py::test_exact_match_queries - TypeError: argume...
FAILED tests_sync/test_hash_model.py::test_exact_match_queries - TypeError: a...
======================== 2 failed, 140 passed in 3.36s =========================

There could be another bug in the code when querying a primary key field of type int:

actual = await m.Member.find(m.Member.id == 0).all()

               if separator_char in value: # a bug here, value is int.

@dvora-h, any idea?

FYI, this PR is an enhancement on the HashModel, after merging this PR when all tests pass, users can now define a new primary key like:

class Customer(HashModel):
    id: int = Field(primary_key=True)
    first_name: str
    last_name: str
    email: EmailStr
    join_date: datetime.date
    age: int
    bio: Optional[str]

If it is not defined, it will take the default one:

    pk: Optional[str] = Field(default=None, primary_key=True)

Otherwise, pk will be removed, and you can access the new primary key using the key method.

Edit:

For some reason, a primary key can't be indexed with an integer value, that's weird, Anyone knows why? Is this a thing in Redis?

Edit:

Found the bug, when the value is integer and the field is primary key, the field type become RediSearchFieldTypes.TAG rather than a RediSearchFieldTypes.NUMERIC. To solve this issue, we can add an additional if statement within the this if statement elif field_type is RediSearchFieldTypes.TAG::

                if isinstance(value, int):
                    result = f"@{field_name}:[{value} {value}]"

Let's see if the tests pass this way.

wiseaidev avatar Aug 11 '22 15:08 wiseaidev

Codecov Report

Merging #347 (996154c) into main (c6fb00b) will increase coverage by 0.76%. The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
+ Coverage   77.64%   78.41%   +0.76%     
==========================================
  Files          14       14              
  Lines        1154     1158       +4     
==========================================
+ Hits          896      908      +12     
+ Misses        258      250       -8     
Flag Coverage Δ
unit 78.41% <90.00%> (+0.76%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aredis_om/model/model.py 86.79% <90.00%> (+0.29%) :arrow_up:
aredis_om/model/migrations/migrator.py 82.75% <0.00%> (+6.89%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Aug 11 '22 20:08 codecov-commenter

Super clean. Approved.

chayim avatar Sep 01 '22 10:09 chayim