Skip to content
This repository has been archived by the owner on Feb 7, 2019. It is now read-only.

Don't raise errors when loading old objects from fixtures #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ezheidtmann
Copy link
Contributor

As described in #73, I can't load fixtures for a model with a M2M field in the current version of cleanerversion. This PR allows loaddata to succeed, but I'm not sure about the consequences. And I don't know that I fully understand the need for the SuspiciousOperation exceptions in the first place.

So this is a piece for discussion. Feedback welcome, please! If this isn't obviously a bad idea, I can write some tests for it.

This seems to work with a VersionedManyToManyField, but needs more testing.
@maennel
Copy link
Contributor

maennel commented May 22, 2015

Hi @ezheidtmann,
Thanks for your contribution. The SuspiciousOperation exception is mainly intended to avoid someone adding related objects on non-current objects.
As you may have guessed already, we didn't implement this exception with dumpdata and loaddata in mind. So your input is welcome.

One question: what other scenarios require the base Serializer from django/core/serializer/base.py? Is there a possibility for conflicts? I currently can't think of any...

Thanks

@ezheidtmann
Copy link
Contributor Author

OK, so CleanerVersion is capable of storing old versions on m2m fields on old versions, but you added the exception to avoid user errors? That's great, cause that means my patch probably doesn't break huge things. :-)

The only other place I've used the Serializer is a place in my app where I'm loading initial or demo data into a new customer's account. We're storing the data in a fixture, loading it almost like how loaddata does, but in the middle of a view rather than as a command. I don't know of any other way that the Serializer might be used. Maybe caching?

@maennel
Copy link
Contributor

maennel commented May 27, 2015

From what I can find, serialization for caching is done using the Pickler class. So, no danger here...
Do you think you will be able to provide some tests, before merging this code?
Thanks.

@ezheidtmann
Copy link
Contributor Author

I've put this on my list for the week.

@peterfarrell
Copy link
Contributor

Just wanted to ping @ezheidtmann about the status of getting some tests in the PR before merging code. Do you have an ETA?

@ezheidtmann
Copy link
Contributor Author

@peterfarrell I'm sorry, I didn't get to it last month and I currently don't have an ETA for tests. I wouldn't mind if you or someone else wrote them.

@peterfarrell
Copy link
Contributor

@ezheidtmann - Just to confirm -- the load data issues you describe only occur on M2M in which the model has changed? I don't understand the issue enough just based on the above comments to write tests for it. Could you share an outline what you intended to do and I can get one of my team members to do it. (cc: @boydjohnson )

@ezheidtmann
Copy link
Contributor Author

I really wish I had written those tests immediately ... I'm having trouble remembering what the problem was. I know that I was able to load a fixture full of "current" models, but then I had trouble when I added an M2M field. It may have been that the problem was just that my fixture had some non-current intermediate models; I'm not sure there's anything about M2M in particular that this fixes.

I guess the test would be to generate some models, make some changes so there's some history, serialize them, delete the originals, and then deserialize them back into the database. And put an M2M field in there for good measure.

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.

3 participants