sopherapps / pydantic-redis

A simple Declarative ORM for Redis using pydantic Models
https://sopherapps.github.io/pydantic-redis
MIT License
43 stars 14 forks source link

Can't work with models that contain lists of other models #8

Closed Rictus closed 2 years ago

Rictus commented 2 years ago

Hello,

I think I have been too fast with my pull request, I'm realizing it does not solve the problem.

Here's the code to reproduce ``` py from typing import List from pydantic_redis import RedisConfig, Model, Store from uuid import uuid4 class Folder(Model): _primary_key_field: str = 'id' id: str name: str class FoldersList(Model): _primary_key_field: str = 'id' id: str elements: List[Folder] store = Store(name='some_name', redis_config=RedisConfig(db=0, host='localhost', port=6379)) store.register_model(Folder) store.register_model(FoldersList) example_folder = Folder(id=str(uuid4()), name='first folder') FoldersList.insert(FoldersList(id='test', elements=[example_folder])) ```

Edit: I'm actually not sure what's happening

Traceback (most recent call last):
  File "database/abladatabase/test.py", line 35, in <module>
    FoldersList.insert(FoldersList(id='test', a=[example_folder]))
  File "/mnt/e/miniconda/envs/ablabackend/lib/python3.8/site-packages/pydantic_redis/model.py", line 44, in insert
    cls.__insert_on_pipeline(_id=None, pipeline=pipeline, record=record, life_span=life_span)
  File "/mnt/e/miniconda/envs/ablabackend/lib/python3.8/site-packages/pydantic_redis/model.py", line 178, in __insert_on_pipeline
    mapping = cls.serialize_partially(data)
  File "/mnt/e/miniconda/envs/ablabackend/lib/python3.8/site-packages/pydantic_redis/abstract.py", line 35, in serialize_partially
    return {key: orjson.dumps(value) for key, value in data.items()}
  File "/mnt/e/miniconda/envs/ablabackend/lib/python3.8/site-packages/pydantic_redis/abstract.py", line 35, in <dictcomp>
    return {key: orjson.dumps(value) for key, value in data.items()}
TypeError: Type is not JSON serializable: Folder

I'm using

pydantic                   1.10.2
pydantic-redis             0.1.3
Tinitto commented 2 years ago

Let me have a look. It might be a version issue.

Tinitto commented 2 years ago

Oh...wow...my bad. There were some fixes I added a few weeks back in 0.1.4 and I forgot to deploy it to pypi. Let me clean it up before pushing.

Tinitto commented 2 years ago

0.1.4 has the same issue. Your earlier assumption seems to be right.

  File "/mnt/e/miniconda/envs/ablabackend/lib/python3.8/site-packages/pydantic_redis/abstract.py", line 35, in <dictcomp>
    return {key: orjson.dumps(value) for key, value in data.items()}
TypeError: Type is not JSON serializable: Folder

orjson.dumps(Folder(...)) will fail but Folder(...).json() should pass.

Tinitto commented 2 years ago

@Rictus, I would like to assign this to you. Is that okay?

Rictus commented 2 years ago

After some analysis, pydantic-redis is good for persisting simple models like:

class Folder(Model):
    _primary_key_field: str = 'id'
    id: str
    name: str

class ParentFolder(Model):
    _primary_key_field: str = 'id'
    id: str
    elements: Folder

ParentFolder have one child that is Folder

But current version of code does not allow for:

class Folder(Model):
    _primary_key_field: str = 'id'
    id: str
    name: str

class FoldersList(Model):
    _primary_key_field: str = 'id'
    id: str
    elements: List[Folder]

Where FoldersList have multiple childs that are of type Folder.

In Redis, I would see that one instance of FoldersList (hashmap) would have a property named elements which is a list of keys (str). Each key leading to a Folder persisted in redis like the lib already do.

Do you agree ? That's a lot of work from my PoV

Tinitto commented 2 years ago

Pretty much. Yeah it is a lot of work. Maybe the work around for now is convert the Folders to dict first (if your use-case allows for that)

Tinitto commented 2 years ago

Or use the reverse relationship i.e.

class Folder(Model):
    parent: FolderList
    # ...
Rictus commented 2 years ago

Maybe the work around for now is convert the Folders to dict first (if your use-case allows for that)

It does not

Or use the reverse relationship i.e.

That is a good suggestion but it would be great for pydantic-redis to support both ways. The PR currently goes on this way.

Rictus commented 2 years ago

Thank you @Tinitto