redis / redis-om-python

Object mapping, and more, for Redis and Python
MIT License
1.12k stars 112 forks source link

Allow users to define a new primary key. #347

Closed wiseaidev closed 2 years ago

wiseaidev commented 2 years ago

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.

codecov-commenter commented 2 years ago

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.

chayim commented 2 years ago

Super clean. Approved.