cleanerversion icon indicating copy to clipboard operation
cleanerversion copied to clipboard

Don't raise errors when loading old objects from fixtures

Open ezheidtmann opened this issue 9 years ago • 8 comments

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.

ezheidtmann avatar May 21 '15 19:05 ezheidtmann

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

maennel avatar May 22 '15 08:05 maennel

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?

ezheidtmann avatar May 23 '15 18:05 ezheidtmann

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.

maennel avatar May 27 '15 06:05 maennel

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

ezheidtmann avatar Jun 02 '15 23:06 ezheidtmann

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

peterfarrell avatar Jul 16 '15 20:07 peterfarrell

@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.

ezheidtmann avatar Jul 16 '15 21:07 ezheidtmann

@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 )

peterfarrell avatar Jul 16 '15 21:07 peterfarrell

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.

ezheidtmann avatar Jul 16 '15 23:07 ezheidtmann