-
Notifications
You must be signed in to change notification settings - Fork 255
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
Default django-filter types and overriding types #234
Comments
hi @indigane! i think what you are asking for is already existing:
though i'm thinking about deprecating that class and actually adding filter extensions. In that case everything should work out of that box. i already have a local WIP branch for that. |
i chose to include the extension. please don't use the suggested can you confirm this now works as expected? |
@tfranzel We've been using django-filter to filter on a query expression (annotation) rather than a model field. While django-filter allows it, drf-spectacular schema view is now crashing starting from e16e730.
|
@tfranzel I encountered a similar problem to stnatic. django-filter also provides filters such as django_filters.rest_framework.OrderingFilter which can't map into model fields. |
@tfranzel I've just found another related issue, that is the "description" in the result of |
@stnatic @wonderbeyond apparently i have not considered all use cases of django-filter, but opted-in all users. that was not the best decision on my part. i'll try to improve this. @stnatic can you give an example of that? in the end we can fall back on "string" if map_model_field fails, because that is exactly what |
However, we can still do some effort if map_model_field fails. In fact I'm trying to fix in my own application scope. FYR: class FixDjangoFilterExtension(OpenApiFilterExtension):
"""
See https://github.com/tfranzel/drf-spectacular/issues/234#issuecomment-752215103
This Extension class can be removed if the problem is resolved.
See also https://github.com/wonderbeyond/django-filter/blob/d0f82ac091d1cee96eb71f76e959b30136c51e8e/django_filters/rest_framework/backends.py#L20
"""
priority = 1
target_class = 'django_filters.rest_framework.DjangoFilterBackend'
match_subclasses = True
_FILTERS_OPANAPI_TYPE_MAP = {
filters.CharFilter: 'string',
filters.BooleanFilter: 'boolean',
filters.NumberFilter: 'number',
}
def get_schema_operation_parameters(self, auto_schema, *args, **kwargs):
def get_filter_description(filter):
field = filter.field
return field.help_text if field.help_text is not None else filter.label
def get_filter_openapi_type(filter):
for filter_type, openapi_type in self._FILTERS_OPANAPI_TYPE_MAP.items():
if isinstance(filter, filter_type):
return openapi_type
return 'string'
model = get_view_model(auto_schema.view)
if not model:
return []
filterset_class = self.target.get_filterset_class(auto_schema.view, model.objects.none())
if not filterset_class:
return []
parameters = []
for field_name, field in filterset_class.base_filters.items():
path = field.field_name.split('__')
model_field = follow_field_source(model, path)
description = get_filter_description(field)
if isinstance(model_field, models.Field):
schema = auto_schema._map_model_field(model_field, direction=None)
description = description or schema.pop("description", None)
else:
schema = {'type': get_filter_openapi_type(field)}
parameters.append(build_parameter_type(
name=field_name,
required=field.extra['required'],
location=OpenApiParameter.QUERY,
description=description,
schema=schema,
enum=[c for c, _ in field.extra.get('choices', [])],
))
return parameters |
@tfranzel Imagine the following data structure: Team:
TeamMembership:
In a viewset you could introduce an "extra field" by creating an annotation. I'm using an overly complicated example, but something as simple as def get_queryset():
return Team.objects.annotate(
primary_owner=FilteredRelation("members", condition=Q(members__role=TeamRole.PRIMARY_OWNER)
).select_related("primary_owner) Then the django-filter part would look like this: class TeamFilter(filterset.FilterSet):
primary_owner = filters.ModelChoiceFilter(
queryset=User.objects.all(), label="Primary owner", method="filter_by_primary_owner"
)
def filter_by_primary_owner(self, queryset, name, value):
return queryset.filter(primary_owner__user=value)
class Meta:
model = Team
fields = [
"primary_owner",
] Another example inspired by @wonderbeyond would be to |
Before I submitted the issue, I had tried to use the It may take a while before I can get back to test it again, so if you say it works then that is fine. In other cases where there is need for customizing typing, what would you recommend? Is the only way to override the full schema with |
@indigane, when i changed the behaviour to better automatic introspection, i did not account for some edge cases of i will do a fix later today to remedy that. i will try to fix the assertion errors first and foremost, while also trying to pick up more edge cases |
Updating from What I tried in "DEFAULT_FILTER_BACKENDS": [
"drf_spectacular.contrib.django_filters.DjangoFilterBackend",
], to the settings, and I tested with debug prints that it did run the filter backend, but the typing didn't change. Not that it matters since it has been deprecated. |
thank you all for the feedback. i addressed the core issue and tried to do some improvements. filter functions should now be handled gracefully. @stnatic @wonderbeyond thanks for sharing your approach. i incorporated some of your changes into the solution. much appreciated. @indigane: sounds weird with the please let me know if there are any issues left so that we can close this issue. |
@tfranzel Great work! |
@tfranzel Thanks a lot for your work! I have the same issue with a custom # TODO underspecified filters. fallback to STR
# filters.ChoiceFilter: None,
# filters.TypedChoiceFilter: None,
# filters.MultipleChoiceFilter: None,
# filters.DurationFilter: None,
# filters.NumericRangeFilter: None,
# filters.RangeFilter: None,
# filters.BaseCSVFilter: None,
# filters.LookupChoiceFilter: None, I'd be happy to help improve this! |
@wonderbeyond sry i saw it but overlooked it later because multiple people raised different issues here. i see the problem but i'm pretty sure your statement is incorrect.
is explicitly mentioned in the schema object. also all model properties are schema objects and thus all existing |
@martinlehoux, which one specifically? there were multiple issues discussed at the same time.
yes. for example, the choice field in itself has no obvious type (at least to my knowledge). this is not that bad because if there is a model field behind it as we use that. a few of those filters can certainly be improved (e.g. there are 3 approaches in the code now:
happy to hear any suggestions. |
@tfranzel Sorry I'm late here! I took a quick review on your recent commits, mainly on the drf-spectacular/drf_spectacular/contrib/django_filters.py Lines 56 to 84 in 6febe8f
|
regarding 1: i added the help_text from extra. i think that should do it. it takes precedence over label as there is only one spot for documentation "description". there is no summary afaik. regarding 2: i see the problem. the reason i chose this path is because filters are not always more descriptive. For example i want to push out a new release so we ideally should wrap this up soon. |
i'll close this issue as the majority of items (including the first issue) have been resolved. for more improvements to |
There's a long-standing issue in
django-filter
, where the typing of parameters ends up wrong in the resulting OpenAPI schema, due to them being hardcoded as'string'
indjango-filter
: https://github.com/carltongibson/django-filter/blob/6045fff/django_filters/rest_framework/backends.py#L164This is the closest related GitHub issue that I am aware of carltongibson/django-filter#1104
Is it at this point possible to resolve this using
drf-spectacular
, and I just have not figured out how?If not, then would it be plausible to add to
drf-spectacular
'scontrib.django_filters
something similar to this https://github.com/tfranzel/drf-spectacular/blob/master/drf_spectacular/openapi.py#L396 that would check if the parameters are instances of specificdjango-filter
filters, such asfilters.NumberFilter
-> would map to some numeric type?If that is too far-fetched as well, then is it possible to override only the parameter
type
usingdrf-spectacular
? It would seem that if I override the parameter withOpenApiParameter
then it replaces the whole schema for the parameter.Or if none of these are anywhere near the solution, what would you suggest for solving the wrong typing, or what approach have you or others used? The library includes some support for
django-filter
so it does not seem to be a completely new use case.The text was updated successfully, but these errors were encountered: