Skip to content

compare Field.method to the Link method in AutoSchema manual fields #5621

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

Closed
wants to merge 2 commits into from

Conversation

vdejager
Copy link

@vdejager vdejager commented Nov 24, 2017

Description

when adding a manual field to AutoSchema it appears on all methods of a Link objects.
When you add a 'body' parameter it ends up on the GET method of a link as well. (although allowed, normally GET requests do not do anything with body or form parameters and as such they do not show up in request.data

This pull request adds a check if a manual field specified a method to which it belongs.

i.e inside your endpoint view add:

schema = AutoSchema(manual_fields=[
        coreapi.Field(
            "my_extra_field",
            required=True,
            location="body",
            schema=coreschema.String(),
            method=("POST",) 
        ),
    ])

# method=("GET", "POST") would place the field on both get and post endpoints, works teh same for put and patch.

This field will now only appear in a 'POST' Link

See the necessary patch in core-api as well core-api/python-client#147

@carltongibson
Copy link
Collaborator

carltongibson commented Nov 24, 2017

Hi @vdejager.

Thanks for the input here. We actually decided against this kind of per-method adjustment for the manual_fields parameter. It's adding too much complication to the call site, which can already be verbose enough.

We don't want to add a method attribute to Field. To do so is to give Field knowledge it should not have.

Rather the approach we considered was to allow passing a dictionary to specify fields on a per-method basis...

{
    "GET": [ ... ],
    "POST": [ ... ],
}

It's then obvious that you'll end with repetition here. Looking at ways of handling this led to the decision to just handle the All Methods case, as the most common requirement. (The example we have in mind is adding an additional path or query parameter that is then used in get_object or such.)

The solution for your use-case is to subclass AutoSchema to conditionally add the additional fields in get_link, most likely after just calling super().

You are welcome to adjust how manual_fields is handled in you AutoSchema subclass. (You could for instance pass a per-method data structure...)

I would be open to a PR applying Extract Method to the get_manual_fields logic, currently inline in get_link.

if self._manual_fields is not None:
by_name = {f.name: f for f in fields}
for f in self._manual_fields:
by_name[f.name] = f
fields = list(by_name.values())

This would allow easier customisation of the behaviour here.

In general a PR will need to add tests covering the suggested change, have the test suite pass, and add relevant documentation.

@vdejager
Copy link
Author

vdejager commented Nov 24, 2017 via email

@carltongibson
Copy link
Collaborator

@vdejager We've extracted the get_manual_fields logic in #5633. This will be in 3.7.4, before the holidays.

The docs there add an example of how you might handle branching on GET/POST methods (and so on).

Since we're all still learning/developing the patterns here, I'm very happy to look at examples on the issue tracker here.

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

Successfully merging this pull request may close these issues.

2 participants