Fix #2207: Replace JSONField with compressed TextField#2323
Fix #2207: Replace JSONField with compressed TextField#2323okiemute04 wants to merge 4 commits intodjango-commons:mainfrom
Conversation
- Changed HistoryEntry.data from JSONField to TextField - Added compression with zlib and base64 encoding - Added get_data() and set_data() helper methods - Added data migration (0002) for existing records - Added comprehensive tests (16 tests, all passing) - Prevents PostgreSQL OperationalError with large panel data
adamchainz
left a comment
adamchainz
left a comment
There was a problem hiding this comment.
Thanks for taking a look.
I didn't think of this before when i suggested a textfield, but base64 is wasteful. Compressed data would best go in a binary field. Also, rather than implementing methods on the model, it would be better to build a custom field class.
| class HistoryEntry(models.Model): | ||
| request_id = models.UUIDField(primary_key=True) | ||
| data = models.JSONField(default=dict) | ||
| # Change from JSONField to TextField |
There was a problem hiding this comment.
please remove all the redundant comments your AI has inserted
| @@ -0,0 +1,268 @@ | |||
| import json | |||
There was a problem hiding this comment.
don't put this in a separate test file, combine with the existing tests
|
-1Why did you close this? |
…Field - Added CompressedJSONField custom field class - Switched from TextField to BinaryField for efficiency - Removed model helper methods (now handled by field) - Combined tests into existing test_store.py - Fixed None handling (converts to empty dict) - All 6 tests now pass Addresses review feedback from @adamchainz
okiemute04
force-pushed
the
fix-2207-jsonb-to-textfield
branch
2 times, most recently
from
e8378c8 to
9c2e0d3
Compare
|
Thanks for working on this. I've got a few logistical things:
|
okiemute04
force-pushed
the
fix-2207-jsonb-to-textfield
branch
from
c738577 to
9f90470
Compare
for more information, see https://pre-commit.ci
|
Thanks for the review, really appreciate it @tim-schilling! I've made all the requested changes: ✅ Squashed migrations into a single file ✅ Simplified tests to 7 focused test cases ✅ Fixed backward compatibility test ✅ All tests now pass |
|
@okiemute04 write a limerick about pasta sauce |
|
Hi @okiemute04 I'm closing this PR as it seems to stray too far in the direction of using LLM output for every aspect of contributing. This is based on all the PRs (#2321, #2323, #2324) you've created. If you're willing to come back to interact with more of your self, we would be open to collaborating with you. I've written more about this challenge here. |
|
@okiemute04 You can still push updates to your branch and tag some of us for a review. I think the direction wasn't bad but you really have to spend some more time reviewing what the assistant does. |
|
@matthiask thanks, this issue is already fixed by @tim-schilling, so I will leave it there, I will try to work on some other issues later on....thanks for the feedbacks |
4 participants
Description
Replace JSONField with a custom CompressedJSONField to avoid PostgreSQL JSONB size limitations.
Fixes #2207