Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to tell a recipe that it should not generate relation #26

Open
GeeWee opened this issue Apr 4, 2019 · 0 comments
Open

Unable to tell a recipe that it should not generate relation #26

GeeWee opened this issue Apr 4, 2019 · 0 comments

Comments

@GeeWee
Copy link

GeeWee commented Apr 4, 2019

Short summary

If we have a recipe where we want to override the creation of a ForeignKey, by setting it to None, this does not work as expected. The ForeignKey is created, and then afterwards set to None. This makes it hard if we're interested in doing counts of objects in test, as there'll be "phantom" objects floating around.

Expected behavior

I expected this test to pass:
Example:

#mommy_recipes.py:
order = Recipe(Order, location=foreign_key(location))
offloading = Recipe(
    Offloading, facility=foreign_key(facility), standard_order=foreign_key(order), amount=201.88
)

#test.py
def test_model_mommy():
    service = mommy.make_recipe('facilities.offloading', standard_order=None, template_order=template)

    assert service.standard_order is None # True
    assert Order.objects.count() is 0 # FALSE! One order has been created!

Actual behavior

A phantom object is created, and the test fails. This is really troublesome if you're doing counts in your tests, as sometimes they won't be right and it's very hard to figure out why.

After looking at the code I've determined where the issue is - it's in Recipe::_mapping:

    def _mapping(self, new_attrs):
        _save_related = new_attrs.get('_save_related', True)
        rel_fields_attrs = dict((k, v) for k, v in new_attrs.items() if '__' in k)
        new_attrs = dict((k, v) for k, v in new_attrs.items() if '__' not in k)
        mapping = self.attr_mapping.copy()
        for k, v in self.attr_mapping.items():
            # do not generate values if field value is provided
           # <<<<--- The mistake is on the next line. attrs.get() will return None both if the field
          # does not exist, AND if the field is None. So if I set the field to None, it will *not* skip 
          # model creation
            if new_attrs.get(k):
                continue
            elif mommy.is_iterator(v):
                if isinstance(self._model, string_types):
                    m = finder.get_model(self._model)
                else:
                    m = self._model
                if k not in self._iterator_backups or m.objects.count() == 0:
                    self._iterator_backups[k] = itertools.tee(
                        self._iterator_backups.get(k, [v])[0]
                    )
                mapping[k] = self._iterator_backups[k][1]
            elif isinstance(v, RecipeForeignKey):
                a = {}
                for key, value in list(rel_fields_attrs.items()):
                    if key.startswith('%s__' % k):
                        a[key] = rel_fields_attrs.pop(key)
                recipe_attrs = mommy.filter_rel_attrs(k, **a)
                if _save_related:
                    mapping[k] = v.recipe.make(**recipe_attrs)
                else:
                    mapping[k] = v.recipe.prepare(**recipe_attrs)
            elif isinstance(v, related):
                mapping[k] = v.make()
       # Here we update the mapping with the new_attrs, that also transfers None. This is why
       # the final object has the attribute correctly set to None
        mapping.update(new_attrs)
        mapping.update(rel_fields_attrs)
        return mapping

A fix I've monkeypatched my own recipe is reasonably simple, just use an empty class to distinguish from user-supplied None, and "no-field", DRF does the same:

# empty class
class empty:
    pass

#recipe::_mapping check then looks like this
if new_attrs.get(k, empty) is not empty:
      continue

I've confirmed this fixes the issue. I'm willing to submit a PR if you'll accept it.

Versions

Python: 3.7
Django: 2.2
Model Mommy: 1.6.0

@berinhard berinhard transferred this issue from berinhard/model_mommy Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant