redis / redis-om-python

Object mapping, and more, for Redis and Python
MIT License
1.09k stars 108 forks source link

`all_pks` returns only part of complex primary keys #470

Closed YaraslauZhylko closed 1 year ago

YaraslauZhylko commented 1 year ago

I have the following setup (minimized for simplicity):

class Place(JsonModel, ABC):
    id: str = Field(primary_key=True)

    class Meta:
        global_key_prefix = "places"

class Country(Place):
    some_field: int = 1

    class Meta:
        model_key_prefix = "country"

class Province(Place):
    another_field: bool = True

    class Meta:
        model_key_prefix = "province"

class City(Place):
    yet_another_field: str = "bar"

    class Meta:
        model_key_prefix = "city"

And I want id fields (primary keys) to represent some hierarchy n order to be easily identifieable from the first glance. For that I'd like to use the same : separator that is used for namespacing:

country = Country(id="ca")
province = Province(id="ca:qc")
city = City(id="ca:qc:montreal")

So that the actual Redis keys look like that:

places:country:ca
places:province:ca:qc
places:city:ca:qc:montreal

This setup works for most actions. For example, I can easily get the city like that:

>>> City.get("ca:qc:montreal")
{"id": "ca:qc:montreal", "yet_another_field": "bar"}

But it does not work for .all_pks().

Expected behaviour:

>>> list(Country.all_pks())
["ca"]
>>> list(Province.all_pks())
["ca:qc"]
>>> list(City.all_pks())
["ca:qc:montreal"]

Actual behaviour:

>>> list(Country.all_pks())
["ca"]
>>> list(Province.all_pks())
["qc"]
>>> list(City.all_pks())
["montreal"]

This behaviour is caused by this patch of code: https://github.com/redis/redis-om-python/blob/721f734aff770f56c54701629f578fe175d9ddda/aredis_om/model/model.py#L1530-L1535

Upon finding the key, it splits the key by : and takes the last part of it. So places:city:ca:qc:montreal becomes montreal.

While it should just remove the key_prefix part. So that places:city:ca:qc:montreal becomes ca:qc:montreal.

YaraslauZhylko commented 1 year ago

Will create a PR to fix that soon.

YaraslauZhylko commented 1 year ago

Ready for review: #471