Skip to content
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

Improve documentation on fields.DelimitedList vs fields.List #406

Closed
sm-moore opened this issue Jul 23, 2019 · 13 comments

Comments

@sm-moore
Copy link

commented Jul 23, 2019

The docs for the fields.DelimitedList say it "can load from either a list or a delimited string" but I cannot get it to load from a list. It's possible I'm doing something wrong but When I swap out fields.DelimitedList for fields.List everything works as expected. I think either the docs should be changed, or we should fix the implementation of DelimitedList so that it works as expected.

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

The current behavior appears correct:

>>> f = fields.DelimitedList(fields.Int())

>>> f.deserialize([42,24])
Out[2]: [42, 24]

>>> f.deserialize('42,24')
Out[3]: [42, 24]

Can you post code to reproduce your issue?

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

Creating a small reproducible example is going to take a while. Here's a small example though:

class MySchema(marshmallow.Schema):
    stages = webargs.fields.DelimitedList(webargs.fields.Str())

When I hit an endpoint that uses this schema, like this:
/myendpoint?stages=one&stages=two
I only get passed the first value for stages.

If I change my implementation to use marshmallow.fields.List everything works as expected.

I managed to create custom marshmallow field that's almost the same as the implementation for DelimitedList and got it working as expected end to end.

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

It's very possible that this implementation is broken due to something specific to my current application. The fix that worked for me is extremely puzzling and I cannot for the life of me figure out why this is breaking it.

Changing the field delimiter to delim here https://github.com/marshmallow-code/webargs/blob/dev/src/webargs/fields.py#L56 and all references to that value fixed my issue...

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Ah I see, you're parsing query params. DelimitedList should only be used when you expect the value to be a be a delimited list, e.g. /myendpoint?stages=one,two. List should be used for duplicated query params, ?stages=one&stages=two, in which case webargs will use something like request.args.getlist() (in Flask) to get the input value for the field. So if you're expecting duplicated args, using List is correct.

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

Gotcha, I feel we should change the docs then because the docs say this should operate exactly like marshmallow.fields.List but that’s not true.

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

I’ll stick with my custom implantation. Since apis and libraries are inconsistent, I wanted a solution that would support both list types.

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I agree that the docs can make this clearer. The docstring for DelimitedList is technically correct in documenting the behavior of the field itself, but we can certainly improve the narrative documentation to clarify when List should be used vs. DelimitedList. It's sorta buried right now in the first code snippet in the quickstart:

image

Perhaps we could add "Delimited Lists" section to the Quickstart.

@sloria sloria changed the title fields.DelimitedList doesn't function like the docs say it should. Improve documentation on fields.DelimitedList vs fields.List Jul 23, 2019

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

This is just a suggestion but perhaps we should change the function doc from
can load from either a list or a delimited string
to
can load from a delimited string.

I do just have one last question though, in your example above

>>> f = fields.DelimitedList(fields.Int())

>>> f.deserialize([42,24])
Out[2]: [42, 24]

>>> f.deserialize('42,24')
Out[3]: [42, 24]

When would we ever hit the case f.deserialize([42,24]) without using duplicated query parameters?

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Well, the field itself does load either a list or delimited string. However, webargs will transparently handle MultiDicts like request.args, calling getlist() if you're parsing with a List.

webargs/src/webargs/core.py

Lines 116 to 117 in 23dfd6f

if hasattr(data, "getlist"):
return data.getlist(name)

core.get_value skips that logic for DelimitedList because it expects DelimitedList to handle the deserialization of a single value.

It will also skip that logic for non-MultiDict types, e.g. request.json. Does that make sense?

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

core.get_value skips that logic for DelimitedList because it expects DelimitedList to handle the deserialization of a single value.

If we're always expecting DelimitedList to handle the deserialization of a single value, why have it extend marshmallow.fields.List? Why not just marshmallow.fields.Field?

@sm-moore

This comment has been minimized.

Copy link
Author

commented Jul 23, 2019

Also, this explains the super confusing behavior I was talking about above. Anything that extends marshmallow.fields.List and has a property delimiter will not behave as expected. Which for my custom field was very confusing and frustrating, perhaps this should be documented?

return isinstance(field, ma.fields.List) and not hasattr(field, "delimiter")

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

If we're always expecting DelimitedList to handle the deserialization of a single value, why have it extend marshmallow.fields.List

Conceptually, DelimitedList is a List, evidenced by the fact that you can't implement DelimitedList as a non-subclass of List without duplicating a bunch of List's code. I suppose you could use composition, but that's not clearly better design in this context.

Anything that extends marshmallow.fields.List and has a property delimiter will not behave as expected.

Yeah..I don't recall why we decided to use duck-typing here. At the very least, it hurts readability. At worst, it leads to unexpected behavior like you ran into. I'll change this.

sloria added a commit that referenced this issue Jul 24, 2019

@sloria sloria closed this in #407 Jul 24, 2019

sloria added a commit that referenced this issue Jul 24, 2019

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

5.4.0 removes the duck typing . I also added more docs re: parsing lists in query strings: https://webargs.readthedocs.io/en/latest/quickstart.html#parsing-lists-in-query-strings .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.