-
Notifications
You must be signed in to change notification settings - Fork 34
Add metadata for apispec compatibility #25
base: master
Are you sure you want to change the base?
Conversation
@@ -71,6 +71,11 @@ def __init__( | |||
|
|||
super(EnumField, self).__init__(*args, **kwargs) | |||
|
|||
self.metadata['enum'] = [ | |||
e.value if self.by_value else e.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by_value needs to be deprecated in favor of the load_by
and dump_by
attributes so this isn't a reliable path forward. The other issue is that you can load by either name/value and dump by name/value independently. I'd rather not deprecate this behavior as it's actually a nice way of working around not great third party APIs (e.g. load from their bad API into our nicer one, and then dump out our nicer version).
Something that could be done is creating a dictionary here of "Accepts" and "Returns" (or something similar) and loading both keys with a list of what they actually do. I imagine that 99% of the time they'll be the same but since this functionality exists, it should be supported fully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, the openapi spec does not allow differentiating between using a schema for loading and for dumping data.
Would it be a reasonable approach to set the enum
metadata to either the values or the names if both load_by
and dump_by
are equal and use both values and names in other cases?
For the test this would result in
class Test(Enum):
one = 1
two = 2
three = 3
load_by |
dump_by |
metadata['enum'] |
---|---|---|
name |
name |
one,two,three |
value |
value |
1,2,3 |
name |
value |
one,two,three,1,2,3 |
value |
name |
one,two,three,1,2,3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that'd be confusing to end users. As for differentiating between the sent model and returned model, is that a limit of apispec or the marshmallow extension? Because that's a fairly common use case in apis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Birne94 's proposal adds a lot of value for the
99% of the time they'll be the same
and is also my personal preference for the cases where they're not as well (If automatic tooling can't get the exact set of allowable values, at least it narrows it down to a superset which is no more than 2x the set of allowable values for a person who cares about one side of the api--either the ingested schema or the output schema).
@justanr if you disagree, would not setting it for load_by != dump_by
cases be preferable? Then it does no harm if it can't get it exactly right, and is helpful in the cases where it definitely can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has there been any activity on a decision regarding this MR? We've just started using this library, and ran into this exact use case and could use a fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for differentiating between the sent model and returned model, is that a limit of apispec or the marshmallow extension? Because that's a fairly common use case in apis.
This is actually a limitation of the OpenAPI specification itself. I'd suggest either raising an error, or falling back on the previous behavior when load_by/dump_by are unequal.
It would be nice if we could get this merged. I am having troubles getting EnumField to work correctly currently with apispec.
|
Could you please merge this PR? We're also running into this same issue of being unable to export Enum values using apispec and marshmallow_dataclass. Would be fantastic to have this! |
I added a fork (based on #25) that fixes this issue (while removing the load_by/dump_by ambiguity) at https://github.com/h/marshmallow_enum This issue hasn't been merged since 2018 -- so if anyone else needs support for APISpec with marshmallow, simply run:
|
fixes #24
This PR adds the key
enum
to the field's metadata which allows apispec (and possibly other tools) to extract the valid enum values.