Opened 4 years ago

Closed 4 years ago

#15696 closed New feature (wontfix)

Add pre_insert signal to loaddata command

Reported by: jonash 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 jonash 4 years ago.
with-tests.patch (2.7 KB) - added by jonash 4 years ago.
Fixed version with tests

Download all attachments as: .zip

Change History (12)

Changed 4 years ago by jonash

comment:1 Changed 4 years ago by jonash

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

comment:2 Changed 4 years ago by lukeplant

  • Type set to New feature

comment:3 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:4 Changed 4 years ago by jonash

Any way to get this into trunk soon?

comment:5 Changed 4 years ago by julien

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 4 years ago by jonash

Fixed version with tests

comment:6 Changed 4 years ago by jonash

  • Needs tests unset

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

comment:7 Changed 4 years ago by russellm

  • 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 4 years ago by russellm

  • Triage Stage changed from Accepted to Design decision needed

comment:9 Changed 4 years ago by jonash

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 4 years ago by jacob

  • Easy pickings unset
  • Resolution set to wontfix
  • Status changed from new to closed

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