fix(dashboard-tiles): migrate doubly stringified layouts correctly #9497
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mariusandra
reviewed
Apr 22, 2022
There was a problem hiding this comment.
- Could the original migration also be fixed? So that new users won't have to convert all their data twice. Once to tiles, and once to correct tiles? This second migration would just be a no-op for them.
- Might it be easier to just have one SQL query that sets
layouts = layouts::jsonor something like that, in caselayoutsis a string? Totally unsure about how the query would look like... This is fine though. - I think it's fine to keep the get_items
strfix around for a while.
|
+1 to what @mariusandra wrote, overall LGTM |
OMG you can edit already applied migrations?! 🤯 |
mariusandra
approved these changes
Apr 22, 2022
| @@ -34,7 +35,10 @@ def migrate_dashboard_insight_relations(apps, _) -> None: | |||
| if not page: | |||
| break | |||
| DashboardTile.objects.bulk_create( | |||
| [DashboardTile(insight_id=row[0], dashboard_id=row[1], layouts=row[2], color=row[3]) for row in page], | |||
| [ | |||
| DashboardTile(insight_id=row[0], dashboard_id=row[1], layouts=json.loads(row[2]), color=row[3]) | |||
There was a problem hiding this comment.
Is there a chance this might also be None somehow... causing json.loads(None) to throw? The default is a dict, so probably not... 🤔
pauldambra
added a commit
that referenced
this pull request
Apr 25, 2022
pauldambra
added a commit
that referenced
this pull request
May 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
In #9416 the migration loaded layouts from the insight table and saved them to new items in the dashboard tiles table
It used raw sql to load them, and django models to save them
Both fields are JSONField
However, django models stringify the json when saving to DB and destringify when loading.
As they were loaded by raw sql they weren't destringified when loading but, since they were saved using a django model, they were stringified when saving to the new location
This meant they were doubly stringified i.e.
json.dumps(json.dumps(field_data))Changes
Adds a follow-up migration which loads all dashboard tiles. Pages through them 500 at a time, for each it loads the tile, destringifies the layout once or twice, and adds it back to the tile. Which should mean that the tile's layout is set to a dict (not a string encoded dict)
The amended tiles from the page are then updated in a single operation
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?
Added unit tests and ran it locally