-
Notifications
You must be signed in to change notification settings - Fork 766
#534 add filterset validation to DjangoFilterConnectionField #535
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
#534 add filterset validation to DjangoFilterConnectionField #535
Conversation
Looks like |
@zbyte64 thanks! It's fixed now. |
6 similar comments
@zbyte64 want to review it? |
for key, error_list in filterset.form.errors.as_data().items() | ||
} | ||
|
||
raise GraphQLError(exc) |
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 think we're passing a dictionary here, but GraphQLError appears to take a str
. Should we format this into a string? I'm not sure that relying on the stringifying behaviour of GraphQLError
is the right thing to do.
One alternative would be to wrap up the dict -> str
processing in a GraphQLDjangoFilterError
(better name needed). This would:
a) make the code here simpler
b) make it easier to customise the behaviour in the view the API that lets the Django view intercept errors.
|
||
if not (filterset.is_bound and filterset.form.is_valid()): | ||
exc = { | ||
str(key): [str(e.message) for e in error_list] |
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.
The key here is in Python/Django source casing I believe. I think we could improve the error reporting here anyway (see other comment), but if we do want to send these to the client, they should use the field names as in the API – i.e. probably camelCase
names, although I believe this can be configured per schema, and overridden per field.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
No description provided.