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
Natural key definitions for Token and various extras models #4131
Conversation
…rmission, Token, and User models
…nable and fix relationship tests
…eAttachment, ImageAttachment
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.
It looks like RequiredRelationshipTestMixin
is not inheriting from RelationshipBaseTest
. That might be causing the test failure. But otherwise, it looks good.
@classmethod | ||
def setUpTestData(cls): | ||
cls.location_ct = ContentType.objects.get_for_model(Location) | ||
cls.rack_ct = ContentType.objects.get_for_model(Rack) | ||
cls.vlan_ct = ContentType.objects.get_for_model(VLAN) | ||
|
||
cls.locations = Location.objects.all()[:5] | ||
cls.locations = Location.objects.get_for_model(Rack).get_for_model(VLAN)[:5] |
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.
Should we assert something like len(cls.locations) == 5
or something after this line?
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.
Yeah, we should. Since this is a classmethod
we can't do self.assertEqual()
but I can maybe just add a plain assert
?
Co-authored-by: Hanlin Miao <46973263+HanlinMiao@users.noreply.github.com>
…cstring to CompositeKeyQuerySetMixin
In addressing Ken's feedback, I uncovered that we didn't have a working implementation for |
Thanks!!! I appreciate your thoroughness and this really helps me understand what is going on. |
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.
🚢
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.
Looking pretty sane. Only the one question for that TODO comment. Good to go either way.
in the `get()` case (only) to *not* automatically convert `get(prefix="10.1.1.1/8")` to | ||
`get(network="10.0.0.0", prefix_length=8)', i.e. discard the host bits. | ||
|
||
TODO: why *shouldn't* we have similar enforcement on `filter()` and `exclude()`? |
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.
It looks like this TODO was addressed in the code in the places merge_dicts_without_collision
is being called? Is that accurate?
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'm referring to the check at lines 265-266 in this function:
if str(_prefix) != str(prefix):
raise self.model.DoesNotExist()
There's no equivalent check in the filter()
method, so I didn't add it in the exclude()
implementation either.
Closes: #3974
What's Changed
ComputedField
,CustomField
,FileAttachment
,ImageAttachment
,ObjectChange
,Relationship
,RelationshipAssociation
, andToken
models.model.objects.get(composite_key=...)
andmodel.objects.filter(composite_key=...)
, we didn't have code in place formodel.objects.exclude(composite_key=...)
.Prefix.objects.exclude(prefix=...)
andIPAddress.objects.exclude(address=...)
.TODO