-
Notifications
You must be signed in to change notification settings - Fork 107
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
Remove non-model-fields from Meta.fields of filters #773
Conversation
Attached issue: https://pulp.plan.io/issues/6915 |
Mongo docs? |
Oh geez. Lemme fix and repush, thanks for the catch. |
Changes include: * Meta.fields list may only reference fields that exist in Meta.model * 'exact' is redundant in Meta.fields qualifiers (per django-filter docs) * Upload.completed was removed in cf5bad, remove referencing filter closes #6915
2c2b859
to
28ac107
Compare
@@ -0,0 +1 @@ | |||
Corrected a number of filters to be django-filter-2.3.0-compliant. |
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 conflicted about whether this should be a misc or a bugfix. You think bugfix?
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's a bugfix - it was harmless, but Worng, and in the next release of django-filters it will stop working at all. We should never have been doing this.
Tested this out and it seems the filters still work so 👍 |
@@ -122,8 +122,6 @@ class Meta: | |||
fields = { | |||
"name": NAME_FILTER_OPTIONS, | |||
"last_heartbeat": DATETIME_FILTER_OPTIONS, | |||
"online": ["exact"], | |||
"missing": ["exact"], |
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 don't think this is semantically equivalent. Does this not tell django-filters that you cannot filter on the "online" or "missing" fields?
I'm also entirely confused why this is necessary. This form is still present in the django-filter
docs and I can't imagine they would break this in a minor release anyways (we're on 2.2 currently). Why change it?
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's redundant since the online and missing filters are defined above. You can still filter on online and missing. Before django-filter 2.3, it just ignored these lines but with django-filter 2.3 it now complains that these fields aren't actually fields.
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.
You're right, I guess I'm just perplexed that they went from allowing it without a warning to making it a hard error in a point release.
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.
Yea, me too. I did some digging and it looks like this is the issue: carltongibson/django-filter#1013
Changes include:
closes #6915