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
Changes from 1 commit
0b4625a
7543a3c
37385eb
0a3cb72
3f3ddc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
and field.attname not in self.model_attrs | ||
): | ||
self.model_attrs[field.name] = self.generate_value( | ||
field, commit_related | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,10 @@ | |
Dog, | ||
DummyBlankFieldsModel, | ||
DummyNumbersModel, | ||
LonelyPerson, | ||
Person, | ||
Profile, | ||
User, | ||
) | ||
|
||
recipe_attrs = { | ||
|
@@ -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(): | ||
|
@@ -450,6 +455,26 @@ 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 | ||
|
||
baker.make(Profile, _quantity=3) | ||
seq_user = user_recipe.extend(username="name", profile_id=seq(1)) | ||
user = seq_user.make() | ||
assert user.profile_id == 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just want to triple check here as I thought it was a tad unintuitive, the first generated number is 2 not 1? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. As you can see here, the iteration starts at I am not the original author, so I cannot speak to the intent of this code, however, it seems like a case of just trying to accommodate too many uses without a clear interface for each specific one. The code could probably be improved, however, most uses I have seen come in the form of |
||
user = seq_user.make() | ||
assert user.profile_id == 3 | ||
|
||
def test_increment_for_one_to_one(self): | ||
from model_bakery.recipe import seq # NoQA | ||
|
||
baker.make(Person, _quantity=3) | ||
seq_lonely_person = lonely_person_recipe.extend(only_friend_id=seq(1)) | ||
person = seq_lonely_person.make() | ||
assert person.only_friend_id == 2 | ||
user = seq_lonely_person.make() | ||
assert user.only_friend_id == 3 | ||
|
||
def test_increment_for_strings_with_bad_suffix(self): | ||
from model_bakery.recipe import seq # NoQA | ||
|
||
|
There was a problem hiding this comment.
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.