Error creating required foreign key objects on obj_create #418

Open
wants to merge 1 commit into from

4 participants

@ipmb

See the associated test for an example of how to trigger the error. I made a small change to get all the tests to pass, but I don't think it's the right one.

It seems like reverse relations (ToManyFields with related_name) should be handled in save_m2m when you are sure the bundle object has a pk, instead of in save_related when the bundle object might not have been saved yet.

@ipmb ipmb Don't save before saving related on create
The initial object save will fail if it has required FKs. The tests
currently pass, but there's probably still an issue if you use
`RelatedField.related_name` and a required ForeignKey on the same
resource.
1839b11
@toastdriven

Thanks for the pull req. Surprised I didn't run into this sooner. :/ Added in SHA: 42e5c8f

I'm a little confused by """...instead of in save_related when the bundle object might not have been saved yet.""", since they're not handled there (https://github.com/toastdriven/django-tastypie/blob/master/tastypie/resources.py#L1944 - any M2M field should get ignored here). Can you explain what you mean?

@ipmb

I'm not sure I fully understand all the implications of this change, but I'll try to explain my thinking...

This line should not be necessary, but you'll see that some validation tests fail if you remove it. save_related should not be handling any relations which need the main resource to have a primary key. I noticed that the ones that failed are resources using related_name.

I think related_name fields should get handled in the save_m2m method which guarantees the main resource will have a primary key. The failing tests tell me they aren't, but maybe there's something else going on there. Whatever it is, my concern is that bundle.obj.pk should not ever be needed inside save_related and only in save_m2m.

I think a test case could be created now that would cause a failure by adding a required ForeignKeyField (similar to what I've done in this pull request) and add it to the end of one of the validation resources with a related_name field. I expect the following to happen:

  1. Save resource and enter save_related method
  2. Loop to the related_name field and hit the bundle.obj.save() line
  3. Save will fail because the required foreign key we added hasn't been saved yet.

I hope that clarifies my previous statement a little and thanks for all your work on tastypie!

@dgerzo

might be closed now 42e5c8f

@ipmb

I don't think this should be closed yet. From my perspective, anything that hits this code branch should be moved to save_m2m. See details in my previous comment.

@ianawilson

Have there been any other thoughts on this? I just hit that case where you have a required FK relationship that hasn't been hooked up yet (saving the parent before the required child) and you get an error about the key constraint.

In addition, 42e5c8f#L0R1962 is causing a problem where it seems to be passing an instance of the model into the related name attribute, rather than an iterable containing the model. Commenting out the entire block mentioned above (42e5c8f#L0R1959), fixes the problem.

EDIT:

Just realized that the assumption here is that you're not creating the related model on the fly. This works fine if you're passing in a url to an already created model, but if you're creating two instances on the fly (by nesting the child inside the parent), then you'll have this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment