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.

Sign up for GitHub

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

Jump to bottom

Store JSON objects instead of pickle objects #1051

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Store JSON objects instead of pickle objects #1051

wants to merge 9 commits into from
+438 −18

Conversation

masipcat
Copy link
Contributor

@masipcat masipcat commented Nov 27, 2020 •
edited
Loading

#1026

I'll rebase this branch with master when master is G7

@masipcat masipcat added this to the G7 milestone Nov 27, 2020
@masipcat
Copy link
Contributor Author

Almost everything works. Only need to fix tests that manipulate files

@masipcat
Copy link
Contributor Author

All tests green! But there are some things to consider/discuss:

  1. I think the current implementation is vulnerable. If someone sends a dict like {"__class__": ...} on a JSONField, we'll resolve the dotted name and create an instance of the __class__ when reading the object from db. The first thing that comes to my mind to mitigate this is appending a secret key like this __class__y2o930874h5f8764598. This requires each guillotina installation to generate this key and put in in the config. What do you think? Any other idea on how to solve it?

  2. orjson is strict and doesn't serialize keys that aren't strings and I found some objects like Blob and BucketList that have dicts with numeric keys. I solved these cases doing a pickle of the object :P This is something that the developer of the application can solve for their own types, but needs to implement custom IStorageSerializer/Deserializer and might be a little inconvenient.

masipcat
masipcat commented Nov 27, 2020
View reviewed changes
guillotina/db/serializers/json/value_deserializers.py
Comment on lines +98 to +100
# from zope.interface import Provides # type: ignore
# obj = Provides(*[resolve_dotted_name(iface) for iface in self.data["__ifaces__"]])
# return obj
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone know how to make it work?

@masipcat masipcat marked this pull request as ready for review May 11, 2021 16:53
@masipcat masipcat changed the title [PoC] Store JSON objects instead of pickle objects Store JSON objects instead of pickle objects May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordic jordic Awaiting requested review from jordic

@bloodbare bloodbare Awaiting requested review from bloodbare bloodbare is a code owner

@vangheem vangheem Awaiting requested review from vangheem vangheem is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
G7
Development

Successfully merging this pull request may close these issues.
1 participant
@masipcat