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

Allow relation _id fields to use a sequence #253

Merged
merged 5 commits into from Oct 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
### Changed
- Validate `increment_by` parameter of `seq` helper when `value` is an instance of `datetime` [PR #247](https://github.com/model-bakers/model_bakery/pull/247)
- Fix a simple typo in `bulk_create` disclaimer in docs
- Allow relation `_id` fields to use sequences [PR #253](https://github.com/model-bakers/model_bakery/pull/253/)
- Fix bulk_create not working with multi-database setup [PR #252](https://github.com/model-bakers/model_bakery/pull/252)
- Conditionally support NullBooleanField, it's under deprecation and will be removed in Django 4.0 [PR #25](https://github.com/model-bakers/model_bakery/pull/250)
- Fix Django max version pin in requirements file [PR #251](https://github.com/model-bakers/model_bakery/pull/251)
Expand Down
20 changes: 19 additions & 1 deletion model_bakery/baker.py
Expand Up @@ -363,10 +363,20 @@ def _make(
]
else:
self.m2m_dict[field.name] = self.model_attrs.pop(field.name)
# is an _id relation that has a sequence defined
elif (
(isinstance(field, OneToOneField) or isinstance(field, ForeignKey))
and hasattr(field, "attname")
and field.attname in self.iterator_attrs
):
self.model_attrs[field.attname] = next(
self.iterator_attrs[field.attname]
)
elif field.name not in self.model_attrs:
if (
not isinstance(field, ForeignKey)
or "{0}_id".format(field.name) not in self.model_attrs
or hasattr(field, "attname")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swapped this to checking "attname" as it mirrors the same technique that Django uses internally.

and field.attname not in self.model_attrs
):
self.model_attrs[field.name] = self.generate_value(
field, commit_related
Expand Down Expand Up @@ -476,6 +486,14 @@ def _skip_field(self, field: Field) -> bool:
if isinstance(field, FileField) and not self.create_files:
return True

# Don't Skip related _id fields defined in the iterator attributes
if (
(isinstance(field, OneToOneField) or isinstance(field, ForeignKey))
and hasattr(field, "attname")
and field.attname in self.iterator_attrs
):
return False
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of returning a False here rather than running through the conditions and falling back to the False at the end. The last 2 conditions catch this scenario and it was getting complicated to isolate the right case. Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you here @ashiazed, but I think this is a result of a major problem that is the fact that there's practically no abstractions under this major for field in fields loop.

I mean, this is done like that since the very beginning of the project and never was refactored. But today, it's more clear to me that we should have a piece of code to receive a field and generate a value/skip it. Baker class would only trust that everything happened as expected. If we had such object, I feel that these validations wouldn't be all grouped together like that.


# Skip links to parent so parent is not created twice.
if isinstance(field, OneToOneField) and self._remote_field(field).parent_link:
return True
Expand Down
27 changes: 27 additions & 0 deletions tests/test_recipes.py
Expand Up @@ -17,7 +17,10 @@
Dog,
DummyBlankFieldsModel,
DummyNumbersModel,
LonelyPerson,
Person,
Profile,
User,
)

recipe_attrs = {
Expand All @@ -32,6 +35,8 @@
"birth_time": now(),
}
person_recipe = Recipe(Person, **recipe_attrs)
user_recipe = Recipe(User)
lonely_person_recipe = Recipe(LonelyPerson)


def test_import_seq_from_recipe():
Expand Down Expand Up @@ -450,6 +455,28 @@ def test_increment_for_strings_with_suffix(self):
person = fred_person.make()
assert person.email == "fred3@example.com"

def test_increment_for_fks(self):
from model_bakery.recipe import seq # NoQA

profiles = baker.make(Profile, _quantity=3)
start_id = profiles[0].id
seq_user = user_recipe.extend(username="name", profile_id=seq(start_id))
user = seq_user.make()
assert user.profile_id == start_id + 1
user = seq_user.make()
assert user.profile_id == start_id + 2

def test_increment_for_one_to_one(self):
from model_bakery.recipe import seq # NoQA

people = baker.make(Person, _quantity=3)
start_id = people[0].id
seq_lonely_person = lonely_person_recipe.extend(only_friend_id=seq(start_id))
person = seq_lonely_person.make()
assert person.only_friend_id == start_id + 1
user = seq_lonely_person.make()
assert user.only_friend_id == start_id + 2

def test_increment_for_strings_with_bad_suffix(self):
from model_bakery.recipe import seq # NoQA

Expand Down