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

Update operation allows insertion of Redis entries for keys not in the model #279

Closed
Tracked by #290
david-wahlstedt opened this issue Sep 16, 2022 · 6 comments · Fixed by #269 or #287
Closed
Tracked by #290

Update operation allows insertion of Redis entries for keys not in the model #279

david-wahlstedt opened this issue Sep 16, 2022 · 6 comments · Fixed by #269 or #287
Assignees

Comments

@david-wahlstedt
Copy link

david-wahlstedt commented Sep 16, 2022

Describe the bug
If I use update with a key in the 'data' dict which is not defined in the model, Redis gets updated with the "unknown" key, and it's not visible in the output of select.

To Reproduce
Steps to reproduce the behavior:

I added the following line to the example found in README.md: (line 58 in the copy-pasted program example)

    # Update any book or library
    await Book.update(_id="Oliver Twist", data={"author": "John Doe"})
    await Book.update(_id="Oliver Twist", data={"author_aaa": "John aaaaa"}) # added extra line here, with undefined field!

Looking into Redis with redis-cli gives the new binding

127.0.0.1:6379[5]> select 5 # because of the db=5 in the example
OK
127.0.0.1:6379[5]> hgetall "book_%&_Oliver Twist"
 1) "title"
 2) "Oliver Twist"
 3) "author"
 4) "John Doe"
 5) "published_on"
 6) "\"1215-04-04\""
 7) "in_stock"
 8) "false"
 9) "author_aaa"
10) "John aaaaa"

This new entry does not appear in the in the program's output.

Shouldn't there be a check that prevents this?

Python version

  • 3.8
@david-wahlstedt
Copy link
Author

It also doesn't type-check completely. I can update the author to be a number, for instance. But for a more involved example, with a class as a field value, it required it to have the right fields. So it does some checking sometimes.

@david-wahlstedt
Copy link
Author

Maybe this is due to pydantic, but I also found that you can create new books with badly typed and even nonsense fields, like

    Book(title="Great Expectations", author=0, grass=100000000000, published_on=date(year=1220, month=4, day=4)),

The author=0 goes into Redis(True is also allowed as author), but the grass=100000000000 field is completely ignored. No warning or error message, and no change in Redis.

@andrewthetechie
Copy link
Owner

This looks like this could be a bug in update https://github.com/andrewthetechie/pydantic-aioredis/blob/main/pydantic_aioredis/model.py#L92-L103

The update function does not convert the passed-in data dictionary to the model, so it does not validate. It probably should do that. Fixing that might mean converting update from a classmethod to a normal method.

For the second comment (creating with bad data), pydantic ignores fields that are not defined in the model so grass will just get thrown out.

andrewthetechie added a commit that referenced this issue Sep 17, 2022
#279
BREAKING CHANGE: This is a breaking change to how updates for model
attributes are saved to Redis. It removes the update classmethod and
replaces with with a save method on each model.

Additionally, if _auto_sync is set to true on a model (the default), it
will save to redis on any attribute change
andrewthetechie added a commit that referenced this issue Sep 17, 2022
#279
BREAKING CHANGE: This is a breaking change to how updates for model
attributes are saved to Redis. It removes the update classmethod and
replaces with with a save method on each model.

Additionally, if _auto_sync is set to true on a model (the default), it
will save to redis on any attribute change
@andrewthetechie
Copy link
Owner

The fix for the update issue will be in #269 in version 1.0.0

It will be a breaking change, the update() classmethod has been removed. In its place, a method on each instance of a model has a save() function that you can call to save that instance to redis after updating attributes on it. By doing updates via attributes on an instance, pydantics validation system is called to prevent adding invalid values.

Models have a new field _auto_sync: bool that defaults to True. If this is true, any update to attributes will automatically sync to Redis.

See the readme and examples for more examples. Let me know if you have any questions.

I hope to have this PR out sometime in the next 7 days.

@andrewthetechie andrewthetechie self-assigned this Sep 17, 2022
This was linked to pull requests Sep 17, 2022
@andrewthetechie
Copy link
Owner

This fix is included in https://github.com/andrewthetechie/pydantic-aioredis/releases/tag/v1.0.0 which is available in pypi now

@david-wahlstedt
Copy link
Author

This fix is included in https://github.com/andrewthetechie/pydantic-aioredis/releases/tag/v1.0.0 which is available in pypi now

Wow! Thanks a lot :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants