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

_operationServers should be Maybe #17

Open
ilyakooo0 opened this issue Mar 23, 2021 · 7 comments
Open

_operationServers should be Maybe #17

ilyakooo0 opened this issue Mar 23, 2021 · 7 comments

Comments

@ilyakooo0
Copy link
Contributor

servers field description from the specification:

An alternative server array to service this operation. If an alternative server object is specified at the Path Item Object or Root level, it will be overridden by this value.

This seems to indicate that the field should accept null values and be optional. This does not seem to be the case at the moment.

I don't see how it would be possible to not override server objects.

@maksbotan
Copy link
Collaborator

mempty for Operation will just set servers = [] and default ToJSON implementation will omit all empty lists, so in the resulting JSON there will be no servers field at all.

Is there a problem with that behavior?

@ilyakooo0
Copy link
Contributor Author

The behavior becomes problematic when you try to parse openapi specifications from a file. (The FromJSON instance)

(the exact same problem with #14)

@ilyakooo0
Copy link
Contributor Author

Or does the instance parse omitted fields as empty?

In any case, an empty list and null are semantically distinct, leading to different specifications.

@maksbotan
Copy link
Collaborator

Decoder will set [] for missing field. null is allowed as well:

λ> decode' @Operation "{\"responses\": {}}" ^? _Just . servers
Just []
λ> decode' @Operation "{\"responses\": {}, \"servers\": null}" ^? _Just . servers
Just []

This decisions re encoding/decoding exist already in swagger2 package.

@ilyakooo0
Copy link
Contributor Author

Decoder will set [] for missing field. null is allowed as well

Well, this is not a good thing. "An operation needs no authorization" ([]) and "Inherit the authorization needed from the parent context" (null) are two completely different things, and it is a good thing to distinguish between them.

The current behavior means that there is no way of setting "Inherit the authorization needed from the parent context" when encoding a spec, and when reading a spec "Inherit the authorization needed from the parent context" becomes "An operation needs no authorization". I don't think this is desired behavior.


This decisions re encoding/decoding exist already in swagger2 package.

Maybe I misunderstand something. Is the point of this package to match swagger2 as closely as possible? Or is it to implement the OpenApi 3 spec?

@maksbotan
Copy link
Collaborator

Well, this is not a good thing. "An operation needs no authorization" ([]) and "Inherit the authorization needed from the parent context" (null) are two completely different things, and it is a good thing to distinguish between them.

Is null even allowed by OpenAPI spec? I'm not sure, but if it is, and has the meaning you described, then the package should be fixed ofc.


Maybe I misunderstand something. Is the point of this package to match swagger2 as closely as possible? Or is it to implement the OpenApi 3 spec?

No, I've just provided the historical context. Of course if those decisions happen to be wrong, they must be changed. I just didn't touch encoding code at all, hoping that it is OK.

@ilyakooo0
Copy link
Contributor Author

ilyakooo0 commented Mar 24, 2021

Is null even allowed by OpenAPI spec? I'm not sure, but if it is, and has the meaning you described, then the package should be fixed ofc.

This package already serializes fields as missing. As far as I understand, in most cases a missing field and a field with null are interchangeable.

I provided an excerpt from the OpenApi 3 spec in my initial comment. Are you suggesting that it is implying the omission of the field, instead of setting it to null?


In either case, even if the correct behavior is to omit the field (I am now leaning towards that), the way to encode both a missing field, and null in haskell would be with a Maybe (The difference being in the ToJSON instance).

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

No branches or pull requests

2 participants