Allow `format` through GET validation filtering #535

serenecloud · 2018-12-28T02:12:29Z

Issue

The guides on https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html#configuration recommend including the rest_framework_json_api.filters.QueryParameterValidationFilter in initial configuration which is good advice, but by default, the Django REST Framework UI provides a dropdown which allows you to pick format=api or format=vnd.api+json

Without allowing the format parameter through you get the following error:

HTTP 400 Bad Request
Allow: GET, POST, HEAD, OPTIONS
Content-Type: application/vnd.api+json
Vary: Accept

{
    "errors": [
        {
            "detail": "invalid query parameter: format",
            "source": {
                "pointer": "/data"
            },
            "status": "400"
        }
    ]
}

Description of the Change

This change adds format to query_regex

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
changelog entry added to CHANGELOG.md
author name in AUTHORS

codecov · 2018-12-28T02:26:36Z

Codecov Report

Merging #535 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #535   +/-   ##
=======================================
  Coverage   94.33%   94.33%           
=======================================
  Files          60       60           
  Lines        3725     3725           
=======================================
  Hits         3514     3514           
  Misses        211      211

Impacted Files	Coverage Δ
rest_framework_json_api/filters.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc64dad...7a0d0b6. Read the comment docs.

sliverc · 2018-12-28T08:35:47Z

Not sure how to understand https://jsonapi.org/format/#query-parameters whether it allows additional query parameters or not.

Another way would be to change URL_FORMAT_OVERRIDE.

@n2ygk What do you think?

sliverc · 2019-01-07T09:09:51Z

JSON API version 1.1 Release candidate gets a bit more specific on query parameters, see https://jsonapi.org/format/1.1/#query-parameters-custom

This means for a valid implementation of format query parameter, URL_FORMAT_OVERRIDE would need to be set to filter[format]. This sounds a bit odd though but defining a profile is properly a bit of a overkill.

n2ygk · 2019-01-07T15:06:01Z

Not sure how to understand https://jsonapi.org/format/#query-parameters whether it allows additional query parameters or not.

Another way would be to change URL_FORMAT_OVERRIDE.

@n2ygk What do you think?

It says you can have additional "non-standard" query parameters but with the "additional requirement that they MUST contain at least one non a-z character" in order to avoid naming collisions. It's their version of an X- header.

I personally don't like use of the format= query parameter despite what the browseable API offers (which is a carryover from DRF which has no rules about query parameters). The functionality can be achieve using Accepts: application/vnd.api+json header. Perhaps best to override format to be something containing a non a-z character? It's not really a JSON API compliance thing since it's just used by the Browseable API: an HTML response is not covered by JSON API and it's not a filter.

sliverc · 2019-01-14T09:05:13Z

What we can do is to allow query parameter which set by URL_FORMAT_OVERRIDE in QueryParameterValidationFilter and add a recommendation to set URL_FORMAT_OVERRIDE to contentFormat.

sliverc · 2019-02-11T08:48:10Z

@serenecloud Are you open to adjust your PR as commented?

serenecloud · 2019-02-18T08:13:24Z

@sliverc I'm not sure what the change involves at this point. Changing to contentFormat would require a change on the Django end to have this working, yes?

sliverc · 2019-02-18T14:19:56Z

@serenecloud
In the README.md there is a section how rest framework needs to be configured to work well with DJA. There the config option URL_FORMAT_OVERRIDE would need to set to contentFormat as a recommendation. The QueryParameterValidationFilter would need to be adjusted not to hard code format query parameter but use the URL_FORMAT_OVERRIDE value.

n2ygk

I'm not happy with hardcoding allowing "format" query parameter since that violates the jsonapi spec in that a non-standard QP has a "standard looking" name. Furthermore, anything that is not 'Application/vnd.api+json' is not jsonapi and it should only be used when the non-jsonapi DRF browseable API is used during development -- it should not be used in production.

@sliverc Does the Browseable API code respect the value of FORMAT_SUFFIX_KWARG? Then perhaps query_regex could include the value of that kwarg. This still "breaks" jsonapi if the default is used.

Perhaps just document that if you intend to use the browseable API, you should subclass the QueryParameterValdationFilter and override query_regex?

sliverc · 2019-02-18T16:21:41Z

@n2ygk I am a bit confused on your comment as you have already upvoted my comment and I thought you agreed with it. Browsable API can be used in production and we even document it this way in the README.md so it needs to work.
FORMAT_SUFFIX_KWARG is only used for custom url patterns and what we are looking for is URL_FORMAT_OVERRIDE and README.md simply needs to be adjusted to configure this according to JSON API spec (like contentFormat).

We can simply ignore URL_FORMAT_OVERRIDE in QueryParameterValidationFilter if it is not changed from format to something JSON API valid but to be honest this rather sounds like "perfect is the enemy of good"... 😄 if someone wants to break json api spec we cannot stop them, but as long as our recommend configuration doesn't we are safe.

n2ygk · 2019-02-18T16:32:19Z

@n2ygk I am a bit confused on your comment as you have already upvoted my comment and I thought you agreed with it. Browsable API can be used in production and we even document it this way in the README.md so it needs to work.
FORMAT_SUFFIX_KWARG is only used for custom url patterns and what we are looking for is URL_FORMAT_OVERRIDE and README.md simply needs to be adjusted to configure this according to JSON API spec (like contentFormat).

We can simply ignore URL_FORMAT_OVERRIDE in QueryParameterValidationFilter if it is not changed from format to something JSON API valid but to be honest this rather sounds like "perfect is the enemy of good"... 😄 if someone wants to break json api spec we cannot stop them, but as long as our recommend configuration doesn't we are safe.

I upvoted your comment but the PR hardcodes "format" at 0da5fc2

I'm happy with them breaking the spec explicitly as they like, but this makes the default be to allow format in the Query Parameters. (And yes, I should have commented on the specific code change, sorry.)

And I missed that about using the browsable API in production. I guess to be "technical" that's not a JSONAPI interaction at that point.

n2ygk · 2019-02-18T17:03:39Z

I'm not sure what I'm doing differently, but the browseable API seems to work for me with no special setup. Not sure why that is. Is the format QP processed and popped before the QueryParameterValidationFilter backend? See my settings. Will investigate further and report back.

sliverc · 2019-02-18T17:05:05Z

@n2ygk
Ahh now I think I understand where the confusion comes from.. 😄 I think you have answered @serenecloud question before I did but in the GitHub history my comment comes above yours (at least on my view) but the timestamp of your answer is actually before mine...
So I have thought you have answered to my comment so I was confused... Hope this makes sense to you.

So to clarify are you ok with this PR being adjusted as I have outlined in comment? It is just rephrasing what is basically written in the previous comment but includes how to do it.

sliverc · 2019-02-18T17:10:39Z

@n2ygk
Our answers overlapped again 😉 Sometimes a chat application would be the better way of communication 😄

I think you need to configure another renderer like rest_framework.renderers.JSONRenderer to activate the option as seen in the screenshot of initial PR comment.

n2ygk · 2019-02-18T17:29:48Z

@n2ygk
Our answers overlapped again 😉 Sometimes a chat application would be the better way of communication 😄

I think you need to configure another renderer like rest_framework.renders.JSONRenderer to activate the option as seen in the screenshot of initial PR comment.

Let me quote your reply to sync up;-)

I see what happened in my #535 (comment). If I just GET /resource in the browseable API, no format QP is sent in yet it seems to work right, I guess because of a default somewhere. If I explicitly add ?format=api via the GET drop-down in the UI then I can reproduce @serenecloud's original issue.

Per your #535 (comment) I still think that "silently" checking whether it's for hardcoded format or the URL_FORMAT_OVERRIDE value (which defaults to format) still means that the default action will be to allow a non-JSONAPI compliant query parameter to pass strict QueryParameterValidationFilter. Maybe be more explicit with a DJA setting or QueryParameterValidatioFilter attribute to say it's being allowed or vice-versa? Then the documented default can allow it explicitly and when I deploy a backend service that is strictly jsonapi, I can make sure I am not allowing that QP to sneak in.

sliverc · 2019-02-19T07:47:59Z

@n2ygk
What we could do is to log a warning when URL_FORMAT_OVERRIDE is set to format and a uri contains a format query parameter, noting user to configure URL_FORMAT_OVERRIDE to contentFormat. How does this sound?

n2ygk · 2019-02-19T13:09:44Z

The client app won’t see the error. Jsonapi makes the point of being client-friendly by reporting useful errors. (Something needing improvements in DJA). Let’s say I have an attribute called format and I meant ‘filter[format]=api’ but instead I gave ‘format=api’.... Better to make this explicitly configured.

…

On Tue, Feb 19, 2019 at 2:48 AM Oliver Sauder ***@***.***> wrote: @n2ygk <https://github.com/n2ygk> What we could do is to log a warning when URL_FORMAT_OVERRIDE <https://www.django-rest-framework.org/api-guide/settings/#url_format_override> is set to format and a uri contains a format query parameter, noting user to configure URL_FORMAT_OVERRIDE to contentFormat. How does this sound? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#535 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5d6_Vt0CSm-eZwM15F3p8anrE1-JDks5vO6wwgaJpZM4ZjXp1> .

sliverc · 2019-02-20T08:06:33Z

@n2ygk
I see your point but I think we should really avoid adding additional configuration options for what is available in DRF.
Instead let's raise an error after all when URL_FORMAT_OVERRIDE is format and query parameter is format.

n2ygk · 2019-02-24T14:35:24Z

@n2ygk
I see your point but I think we should really avoid adding additional configuration options for what is available in DRF.
Instead let's raise an error after all when URL_FORMAT_OVERRIDE is format and query parameter is format.

@sliverc
I don't quite understand what you mean here. The default value of URL_FORMAT_OVERRIDE is format. The Browseable API requires format=api. So how does one explicitly allow the DRF browseable API or explicitly enforce only valid jsonapi query parameters?

sliverc · 2019-02-25T07:37:43Z

@n2ygk
We document to change URL_FORMAT_OVERRIDE to contentFormat in our README.
In the QueryParameterValidationFilter it's checked whether a query parameter is passed on which is equal configured value in URL_FORMAT_OVERRIDE. If this is true and the value of URL_FORMAT_OVERRIDE is set to something else than format then we let the user pass. If URL_FORMAT_OVERRIDE is still set to format a 400 error is raised (as would be the case today already without this change).

n2ygk · 2019-09-03T16:50:13Z

@sliverc Where'd we end up with this? I've lost track.

sliverc · 2019-09-03T17:44:26Z

@n2ygk This PR should be adjusted as outlined in #535 (comment)

sliverc · 2020-08-21T20:52:56Z

Closing in favor of #812

Ben Bradshaw added 2 commits December 28, 2018 15:03

Allow the GET param format through the filters

0da5fc2

Update docs to match

7a0d0b6

sliverc mentioned this pull request Jan 15, 2019

JSON API 1.1: Profle media type and query parameter #560

Closed

n2ygk requested changes Feb 18, 2019

View reviewed changes

sliverc mentioned this pull request Jun 21, 2019

500 error on invalid parameter when using QueryParameterValidationFilter with BrowsableAPIRenderer #665

Open

sliverc mentioned this pull request Aug 21, 2020

Allow changing of format with query parameter #812

Open

sliverc closed this Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `format` through GET validation filtering #535

Allow `format` through GET validation filtering #535

serenecloud commented Dec 28, 2018 •

edited

Loading

codecov bot commented Dec 28, 2018 •

edited

Loading

sliverc commented Dec 28, 2018

sliverc commented Jan 7, 2019

n2ygk commented Jan 7, 2019

sliverc commented Jan 14, 2019

sliverc commented Feb 11, 2019

serenecloud commented Feb 18, 2019

sliverc commented Feb 18, 2019

n2ygk left a comment

sliverc commented Feb 18, 2019

n2ygk commented Feb 18, 2019 •

edited

Loading

n2ygk commented Feb 18, 2019

sliverc commented Feb 18, 2019

sliverc commented Feb 18, 2019 •

edited

Loading

n2ygk commented Feb 18, 2019

sliverc commented Feb 19, 2019

n2ygk commented Feb 19, 2019 via email

sliverc commented Feb 20, 2019

n2ygk commented Feb 24, 2019 •

edited

Loading

sliverc commented Feb 25, 2019

n2ygk commented Sep 3, 2019

sliverc commented Sep 3, 2019

sliverc commented Aug 21, 2020

Allow format through GET validation filtering #535

Allow format through GET validation filtering #535

Conversation

serenecloud commented Dec 28, 2018 • edited Loading

Issue

Description of the Change

Checklist

codecov bot commented Dec 28, 2018 • edited Loading

Codecov Report

sliverc commented Dec 28, 2018

sliverc commented Jan 7, 2019

n2ygk commented Jan 7, 2019

sliverc commented Jan 14, 2019

sliverc commented Feb 11, 2019

serenecloud commented Feb 18, 2019

sliverc commented Feb 18, 2019

n2ygk left a comment

Choose a reason for hiding this comment

sliverc commented Feb 18, 2019

n2ygk commented Feb 18, 2019 • edited Loading

n2ygk commented Feb 18, 2019

sliverc commented Feb 18, 2019

sliverc commented Feb 18, 2019 • edited Loading

n2ygk commented Feb 18, 2019

sliverc commented Feb 19, 2019

n2ygk commented Feb 19, 2019 via email

sliverc commented Feb 20, 2019

n2ygk commented Feb 24, 2019 • edited Loading

sliverc commented Feb 25, 2019

n2ygk commented Sep 3, 2019

sliverc commented Sep 3, 2019

sliverc commented Aug 21, 2020

Allow `format` through GET validation filtering #535

Allow `format` through GET validation filtering #535

serenecloud commented Dec 28, 2018 •

edited

Loading

codecov bot commented Dec 28, 2018 •

edited

Loading

n2ygk commented Feb 18, 2019 •

edited

Loading

sliverc commented Feb 18, 2019 •

edited

Loading

n2ygk commented Feb 24, 2019 •

edited

Loading