Opened 6 years ago

Closed 5 years ago

#15696 closed New feature (wontfix)

Add pre_insert signal to loaddata command

Reported by: Jonas H. Owned by: nobody
Component: Core (Serialization) Version: master
Severity: Normal Keywords: nonrel
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX:

Description

Here's a patch that adds a pre_insert signal to the loaddata command, allowing to do extra modifications on deserialized objects before saving them to the database.

Here's my use case: SQL databases use integer primary keys, MongoDB uses ObjectIds, and I want to re-use SQL-style fixtures (as found in the Django test suite) to test Django MongoDB Engine (the Django MongoDB backend). I think this is a painless way to solve this problem: Very little code changes and doesn't require any ugly hacks.

Would be great if this could be applied!

Attachments (2)

serializer-pre-insert-signal.patch (1.4 KB) - added by Jonas H. 6 years ago.
with-tests.patch (2.7 KB) - added by Jonas H. 5 years ago.
Fixed version with tests

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by Jonas H.

comment:1 Changed 6 years ago by Jonas H.

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Version: 1.2SVN

comment:2 Changed 6 years ago by Luke Plant

Type: New feature

comment:3 Changed 6 years ago by Luke Plant

Severity: Normal

comment:4 Changed 5 years ago by Jonas H.

Any way to get this into trunk soon?

comment:5 Changed 5 years ago by Julien Phalip

Has patch: set
Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

This sounds like a useful feature, but to stand any chance of inclusion the patch would need some tests and documentation.

Changed 5 years ago by Jonas H.

Attachment: with-tests.patch added

Fixed version with tests

comment:6 Changed 5 years ago by Jonas H.

Needs tests: unset

Where should the documentation go to? The 'loaddata' section in the commands reference? The testing docs?

comment:7 Changed 5 years ago by Russell Keith-Magee

Patch needs improvement: set

I disagree that a signal is the right way to handle this. Signals, even when unconnected, have a cost, and this would impose a cost on every fixture load (and, by extension, every test in every test run) in order to support what seems to be to be an extreme edge case requirement.

My gut tells me that a better approach would be a custom serializer. That would isolate the extra cost to the specific deserializer that needs the extra data conversion features.

Answering your question about tests: the tests should go in the regressiontests/fixtures_regress test suite; the loaddata command is tested there as a side effect of testing fixtures themselves.

comment:8 Changed 5 years ago by Russell Keith-Magee

Triage Stage: AcceptedDesign decision needed

comment:9 Changed 5 years ago by Jonas H.

Russell, that's true, although I think that's negligible - running all model tests, I get a difference of half a second (48.3 vs. 47.9 seconds), for Django's ~3500 tests that's an addition of about 3 seconds.

(btw, my question was about documentation, not tests.)

comment:10 Changed 5 years ago by Jacob

Easy pickings: unset
Resolution: wontfix
Status: newclosed

I find myself agreeing with Russ that a custom serializer is a better approach here. I just can't find a compelling use case for this signal beyond the very specific one you've posted. Wary of feature creep, I'm marking this wontfix.

Note: See TracTickets for help on using tickets.
Back to Top