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

Convert id arguments to graphene.Int field #2

Open
wants to merge 1 commit into
base: ginkgo-stable
Choose a base branch
from

Conversation

ksdaly
Copy link

@ksdaly ksdaly commented Dec 28, 2018

@Chris7 I am on the fence about this one. This is to address ID fields filters being cast as Floats, for example, users__id is defined as graphene.Argument(graphene.Float). It becomes an issue when combining multiple queries, that use the same variable to query fields defined either using graphene, graphen-django, or graphql-ruby, and don't apply the same logic to how arguments are defined. So the same variable can be an ID, Int, or Float.

The way graphene arguments are created from DRF filters is defined in graphene_django.filter.utils, so this only fixes the ID field generating a Float argument when using graphene-django-extras. Meaning that we probably need to patch get_filtering_args_from_filterset locally rather than here as the same issue will come up when somebody attempts to use graphene-django. Also maybe it should call super and do the ID check on args afterwards, to avoid redefining the function?

Note: I chose Int rather than ID, as I was not quite sure how it would affect number lookups like 'gt', 'lt'
etc. down the line. I can update it to ID for consistency.

But I am not entirely sure wether we should be doing any of this at all. The issue really is that NumberFilter is assigned DecimalField in django-filters. It makes sense that graphene-django assigns it to Float (they just implemented Decimal field, so eventually it would be Decimal). Also, we would still need to ensure that everyone wis using ID and Int arguments consistently.

I do agree that it is counterintuitive that ID arguments are Floats, and there already is a mismatch for ids argument, but it is pretty easy (if not very elegant) to work around it in the query by using multiple variables like this:

query SampelQuery($sampleIdFloat: Float, $sampleIdInt: Int) {
  ...
}

Copy link
Collaborator

@Chris7 Chris7 left a comment

Choose a reason for hiding this comment

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

Is there a reason this can't be more declarative? Seems a bit hacky to be assuming a relationship between a field name and a type.

args = {}
for name, filter_field in six.iteritems(filterset_class.base_filters):
# Avoid converting an ID field to a Float
if filter_field.field_class.__name__ in DECIMAL_FIELD_NAMES and re.match(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use isinstance(filter_field.field_class, (class1, class2, etc.)) unless the DECIMAL_FIELD_NAMES are not importable and are just strings.

DECIMAL_FIELD_NAMES = {'DecimalField', 'DecimalInField'}
ID_FIELD_PATTERN = r'(^id$|^id_|.*_id_.*|.*_id$)'

def get_filtering_args_from_filterset(filterset_class, type):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like we should be extending the old functionality instead of rewriting it. Something like:

from graphene_django.filter.utils import get_filtering_args_from_filterset as _get_filtering_args_from_filterset

...

def get_filtering_args_from_filterset(filterset_class, type):
  args = _get_filtering_args_from_filterset(filteset_class, type)
  ...new stuff here...
  return args

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants