Skip to content

Allow injection of preserialized JSON using the default API#95

Merged
lelit merged 12 commits into
python-rapidjson:masterfrom
enquos:preserialized
Mar 25, 2018
Merged

Allow injection of preserialized JSON using the default API#95
lelit merged 12 commits into
python-rapidjson:masterfrom
enquos:preserialized

Conversation

@silviot

@silviot silviot commented Mar 6, 2018

Copy link
Copy Markdown
Contributor

For speed reason we're storing some preserialized JSON in our database.

It's still very useful to be able to treat preserialized objects with the same toolchain as the not serialized ones, so we amended python-rapidjson to support our use case.

This PR proposes a change to the default function API, so that when it returns a bytes string, it's not further serialized, but instead used verbatim.

Risks:

  • existing implementations using default and returning a bytes string should no longer expect it to be serialized. We expect all users would return a regular unicode string.
  • If client code provides an invalid JSON string, the output of rapidjson will in turn be invalid. This looks acceptable and unavoidable, given the goal is to improve speed

silviot added 3 commits March 2, 2018 15:11
When the `default` function returns a bytes object, include it
verbatim into the serializaion.
This allows users to store preserialized versions of their objects in a database
and use rapidjson to generate structures that include them.
If the `default` function returns an invalid JSON the resulting string returned by
rapidjson will be in turn invalid.
@lelit

lelit commented Mar 7, 2018

Copy link
Copy Markdown
Contributor

Thank you for the proposal.

While I can understand your use case and thus how this would be handy, I'm not sure I like the automatic assumption that any bytestrings contains raw JSON: for example, I remember at least one case where what I was dumping contained a slot with a base64-encoded-bytestring, and IIUC this would break that case...

I wonder if a better way would be providing a RawJSON marker class, with which we can be explicit.

I will try to have a closer look at your commits this weekend.

@silviot

silviot commented Mar 7, 2018

Copy link
Copy Markdown
Contributor Author

I understand your concern and agree that a more explicit RawJSON class would be desirable.

But in the interest of clarity, let me point out that this PR does not affect serialization of byte strings by rapidjson when not using the default customization. Nor does it interfere with nested byte strings returned by a default function.

It would only affect byte strings directly returned by the default function.

>>> import rapidjson as rj
>>> rj.dumps({'foo': b'bar'})
'{"foo":"bar"}'
>>> rj.dumps({'foo': rj}, default=lambda x: {'contains_a_nested': b'byte string'})
'{"foo":{"contains_a_nested":"byte string"}}'
>>> rj.dumps({'foo': rj}, default=lambda x: b'{"Pre": "serialized"}')
'{"foo":{"Pre": "serialized"}}'

Anyway, I can refactor the PR to use a custom RawJSON class, when you give me the green light.

@silviot

silviot commented Mar 22, 2018

Copy link
Copy Markdown
Contributor Author

@lelit I updated the pull request.
Now a RawJSON class is used to tell rapidjson we have a preserialized object.

I took the liberty to add running doctests in travis.

@lelit

lelit commented Mar 22, 2018

Copy link
Copy Markdown
Contributor

Great! I'm sorry I could not focus on PyRJ, but I'll do my best to merge this in a short time.
Thank you!

@lelit lelit merged commit 8fe1f1f into python-rapidjson:master Mar 25, 2018
@lelit

lelit commented Mar 25, 2018

Copy link
Copy Markdown
Contributor

Thank you Silvio, I merged your changes, with some mostly stylistic tweaks.

@silviot

silviot commented Mar 26, 2018

Copy link
Copy Markdown
Contributor Author

@lelit Thanks!

I realized failing doctests didn't really make the build fail, so I'm opening a different PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants