Codecov Report

Merging#535 intomaster willnot change coverage.
The diff coverage is100%.

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

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatecc64dad...7a0d0b6. Read thecomment docs.

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

Another way would be to changeURL_FORMAT_OVERRIDE.

@n2ygk What do you think?

JSON API version 1.1 Release candidate gets a bit more specific on query parameters, seehttps://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 tofilter[format]. This sounds a bit odd though but defining aprofile is properly a bit of a overkill.

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

Another way would be to changeURL_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 anX- header.

I personally don't like use of theformat= 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 usingAccepts: application/vnd.api+json header. Perhaps best to overrideformat 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 afilter.

What we can do is to allow query parameter which set byURL_FORMAT_OVERRIDE in QueryParameterValidationFilter and add a recommendation to setURL_FORMAT_OVERRIDE tocontentFormat.

@serenecloud Are you open to adjust your PR ascommented?

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

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

@n2ygk I am a bit confused on your comment as you have already upvoted mycomment 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 isURL_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 I am a bit confused on your comment as you have already upvoted mycomment 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 isURL_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" at0da5fc2

I'm happy with them breaking the spec explicitly as they like, but this makes the default be to allowformat 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.

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 theformat QP processed and popped before theQueryParameterValidationFilter backend? See mysettings. Will investigate further and report back.

@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 mycomment 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 incomment? It is just rephrasing what is basically written in the previouscomment but includes how to do it.

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

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

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

I think you need to configure another renderer likerest_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 hardcodedformat or theURL_FORMAT_OVERRIDE value (which defaults toformat) still means that the default action will be to allow a non-JSONAPI compliant query parameter to pass strictQueryParameterValidationFilter. Maybe be more explicit with a DJA setting orQueryParameterValidatioFilter 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.

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

@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 whenURL_FORMAT_OVERRIDE isformat and query parameter isformat.

@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 whenURL_FORMAT_OVERRIDE isformat and query parameter isformat.

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

@n2ygk
We document to changeURL_FORMAT_OVERRIDE tocontentFormat in our README.
In theQueryParameterValidationFilter it's checked whether a query parameter is passed on which is equal configured value inURL_FORMAT_OVERRIDE. If this is true and the value ofURL_FORMAT_OVERRIDE is set to something else thanformat then we let the user pass. IfURL_FORMAT_OVERRIDE is still set toformat a 400 error is raised (as would be the case today already without this change).

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

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

Closing in favor of#812