Set dynamic field for GET method only

[dwisulfahnur]

I have try to using dynamic fields in POST method and i got an error caused there is a required field are not on the fields selection. I think this PR should be handle the problem.

Thanks

[jtrain]

Are you able to show a test case for the problem you were seeing? It sounds like you are POSTing to an endpoint but also adding the fields query param while you are POST-ing and want the library to ignore that?

[jtrain]

e.g. POST {'name': 'dwisulfahnur'} /api/v1/committers?fields=id

[dwisulfahnur]

Are you able to show a test case for the problem you were seeing? It sounds like you are POSTing to an endpoint but also adding the fields query param while you are POST-ing and want the library to ignore that?

Yes, i think post/put/patch methods should be ignored for fields query params.
Create/update data should not be using selection fields, we have to define the fields that requried/not.

[jtrain]

Correct me if I'm wrong, but a valid use-case might be someone who has expensive fields, and wants to POST their content, and only get back a subset of the fields (excluding the expensive ones).

This would prevent that scenario completely.

Is there a reason that you are inserting the field query params onto your create/update requests? Can't you stop adding the query param?

[jtrain]

BTW seems somewhat related to this closed branch:
#20

[dbrgn]

Hm, I somewhat agree that the response to POST requests should not be filtered. This could have unintended side effects and I think the behavior is generally undefined / unclear (this can be seen by us discussing about whether or not the response should be filtered or not).

@jtrain could you agree to that?

@dwisulfahnur could you maybe add a test? e.g. an endpoint that returns JSON for both GET and POST, but where only GET is filtered. The tests are here.

[jtrain]

I don't think anyone would be relying on the current behaviour, considering it is very limited (fields are filtered for both to_internal and to_representation).

And I (in my own code) don't use GET params for any of our POST requests etc.

So I'm OK if you want to cut the functionality. We should have a test for it though, so people know it was intentional too.

[dbrgn]

@dwisulfahnur thanks for bringing this up!

I'm happy to accept this PR if you add a test and update the first sentence in the README like this:

This package provides a serializer mixin that allows the user to dynamically select only a subset of fields per resource on GET requests.

[dwisulfahnur]

Correct me if I'm wrong, but a valid use-case might be someone who has expensive fields, and wants to POST their content, and only get back a subset of the fields (excluding the expensive ones).

This would prevent that scenario completely.

Is there a reason that you are inserting the field query params onto your create/update requests? Can't you stop adding the query param?

Hello @dbrgn

Thanks for your reply,
Before i add a test, I got this point from @jtrain . It's absolutely right.
And actually, we can do this by change the "fields" property method to "_readable_fields"
It will filter the fields for presentation fields only, so event we are using POST method with fields query param, it will not give an effect for the existing fields.

[jtrain]

@dwisulfahnur yes its true, we could do that. However its much simpler overall to ignore POST entirely and say we don't handle them.

We've had one other pull request asking for this functionality (filter read fields, not POST ones). But otherwise no one asking for it.

And when we release this we should bump the version for breaking change?

[dbrgn]

I actually can't quite follow regarding the _readable_fields thing. But I think only filtering on GET is both the simplest and the clearest solution.

And when we release this we should bump the version for breaking change?

Yep, since we're below 1.0.0, according to SemVer 0.4.0 would be the correct next version.

But you don't have to include the bump in your PR, I prefer always bumping before a release (this avoids conflicts between multiple PRs).