Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to define a new primary key. #347

Merged
merged 8 commits into from
Sep 8, 2022
Merged

Conversation

wiseaidev
Copy link
Contributor

@wiseaidev wiseaidev commented Aug 11, 2022

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 wiseaidev changed the title Make primary key programmable [WIP] Make primary key programmable Aug 11, 2022
@wiseaidev wiseaidev changed the title [WIP] Make primary key programmable [WIP] Allow users to define a new primary key. Aug 11, 2022
@wiseaidev wiseaidev changed the title [WIP] Allow users to define a new primary key. Allow users to define a new primary key. Aug 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022

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%) ⬆️

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%) ⬆️
aredis_om/model/migrations/migrator.py 82.75% <0.00%> (+6.89%) ⬆️

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

Signed-off-by: wiseaidev <[email protected]>
Signed-off-by: wiseaidev <[email protected]>
@chayim chayim self-requested a review September 1, 2022 10:11
if separator_char in value:
if isinstance(value, int):
# This if will hit only if the field is a primary key of type int
result = f"@{field_name}:[{value} {value}]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly (and painfully) enough, I've reach enough things recently to accept that we should probably start avoiding f-strings.

Let's not require it for this PR, but generally, we should remove them.

@chayim
Copy link
Contributor

chayim commented Sep 1, 2022

Super clean. Approved.

@chayim chayim added the enhancement New feature or request label Sep 1, 2022
@dvora-h dvora-h merged commit 551429c into redis:main Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants