Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Consistently use db_to_json to convert from database values to JSON objects. #7849

Merged
merged 5 commits into from
Jul 16, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented Jul 14, 2020

This removes the last usages of importing json from canonicaljson in order to get the simplejson package.

This consistently uses the db_to_json helper in our storage code. So a follow-on to #7836 / #7802 and related to #7674.

@clokep
Copy link
Member Author

clokep commented Jul 14, 2020

There are other instances of json.loads being used directly in synapse.storage -- should we be converting all of those to db_to_json? It is unclear to me in what situations postgresql returns a memoryview / in what situations we get bytes back instead of strings.

@clokep
Copy link
Member Author

clokep commented Jul 14, 2020

There are other instances of json.loads being used directly in synapse.storage -- should we be converting all of those to db_to_json? It is unclear to me in what situations postgresql returns a memoryview / in what situations we get bytes back instead of strings.

I decided to make this change, see 281d4de29eb776c28ce23d1c72bdeb4f1dc10e0b

@clokep clokep force-pushed the clokep/json-unicode-storage branch from 281d4de to 212e79d Compare July 15, 2020 16:18
@clokep clokep force-pushed the clokep/json-unicode-storage branch from 212e79d to fdd65df Compare July 15, 2020 17:51
@clokep clokep changed the title Remove last usages of simplejson to handle converting bytes to json Consistently use db_to_json to convert from database values to JSON objects. Jul 15, 2020
@clokep clokep marked this pull request as ready for review July 15, 2020 19:02
@clokep clokep requested a review from a team July 15, 2020 19:02
@clokep
Copy link
Member Author

clokep commented Jul 15, 2020

I suspect it would be nice to have a json_to_db helper as well that removes whitespace and such as #7372 suggests. Out of scope for this specific PR though.

Comment on lines +252 to +253
# Avoid a circular import.
from synapse.storage._base import db_to_json
Copy link
Member

Choose a reason for hiding this comment

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

hrm. It's unsatisfactory if synapse.storage._base has a dependency on this file somehow. This is fine as a temporary solution though.

Copy link
Member Author

Choose a reason for hiding this comment

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

synapse.storage._base imports from synapse.storage.database, which imports from synapse.storage.background_updater.

@clokep clokep merged commit f460da6 into develop Jul 16, 2020
@clokep clokep deleted the clokep/json-unicode-storage branch July 16, 2020 15:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants