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

Add ToSchema Value #86

Open
phadej opened this issue Oct 24, 2016 · 17 comments
Open

Add ToSchema Value #86

phadej opened this issue Oct 24, 2016 · 17 comments

Comments

@phadej
Copy link
Collaborator

phadej commented Oct 24, 2016

AFAICS there isn't one. It's not that useful, but good to have for completeness

@fizruk
Copy link
Member

fizruk commented Oct 24, 2016

I don't think that's a good idea for reasons similar to bytestrings. If your servant API accepts/responds with just Value — wrap it in a newtype with meaningful description. We could provide helpers for that though, e.g. payloadJSONSchema or anyObjectSchema.

@phadej
Copy link
Collaborator Author

phadej commented Nov 15, 2016

I'd like to have Value instance still, as they might be somewhere deep in the objects.

ByteString is different, as it doesn't have ToJSON/FromJSON instances either.

@fizruk
Copy link
Member

fizruk commented Nov 16, 2016

Fine then. Are you willing to make a PR?
Also, is there a way to attach a compile-time warning to the instance? If so, should we add one?

@saurabhnanda
Copy link

I just got bitten by lack of this instance. My usecase -- supporting Posgres' JSONB in my client-facing JSON API as well.

@fizruk
Copy link
Member

fizruk commented Dec 23, 2016

@saurabhnanda is there a reason you can't use a newtype wrapper around Value?
Does it have to be any Value or is it Object really?

E.g. we have something like this to be able collect some extra info we haven't added to the API yet:

newtype Extra = Extra { getExtra :: Object }
  deriving (Eq, Show, ToJSON, FromJSON)

instance ToSchema Extra where
  declareNamedSchema _ = pure $ NamedSchema (Just "Extra") $ mempty
    & type_ .~ SwaggerObject
    & description ?~ "Arbitrary JSON object with extra information."

So far the only reason for having this instance is Value is buried deep in the objects and you might not have the access to wrap it in a newtype. I am okay with adding an instance for this use case, though, so PR's welcome :)

@saurabhnanda
Copy link

saurabhnanda commented Dec 23, 2016 via email

@phadej
Copy link
Collaborator Author

phadej commented Dec 23, 2016

Object has an object as the outer constructor, i.s. it's not an array or plain number or string. AFAIK, jsonb can be "just an int", so Value is correct one

@fizruk
Copy link
Member

fizruk commented Dec 23, 2016

Yes, Object can only be an object (i.e. a map) while Value can also be an array, a string, a number, a boolean or null. In most cases you want Object and not Value. Even if you're storing it in jsonb in PostgreSQL database.

One reason is that it is not clear how to combine two Values (e.g. when an update comes) while it is fairly straightforward to combine two Objects (merge or deep merge).

Another is that if you use Value you're bound to check that it's not a primitive or array before extracting useful information. This makes analysis of this extra data a bit more complex than it should in my opinion.

@saurabhnanda
Copy link

saurabhnanda commented Dec 24, 2016 via email

@fizruk
Copy link
Member

fizruk commented Dec 24, 2016

@saurabhnanda no, not yet. Object is just a type synonym for HashMap Text Value and since there's no instance for Value there's no instance for Object. But I think it can (and should) be added separately.

It's simple to define one locally for now:

instance ToSchema Object where
  declareNamedSchema _ = pure $ NamedSchema (Just "Object") $ mempty
    & type_ .~ SwaggerObject
    & description ?~ "Arbitrary JSON object."

@qrilka
Copy link
Contributor

qrilka commented Dec 29, 2017

@fizruk this instance above doesn't seem to be valid for version 2.2 of the library - is there any workaround?

@fizruk
Copy link
Member

fizruk commented Dec 29, 2017

@qrilka the instance is valid, but is overlapping with a more general one for HashMap (since Object is a type synonym). You only need to add {-# OVERLAPPING #-} pragma to make it work:

instance {-# OVERLAPPING #-} ToSchema Object where
  declareNamedSchema _ = pure $ NamedSchema (Just "Object") $ mempty
    & type_ .~ SwaggerObject
    & description ?~ "Arbitrary JSON object."

@qrilka
Copy link
Contributor

qrilka commented Dec 29, 2017

using your instance above:

λ> import qualified Data.HashMap.Strict as HM
λ> validateToJSON ( HM.fromList [("a", Number 1)] :: Object)
["property \"a\" is found in JSON value, but it is not mentioned in Swagger schema"]
λ> 

@qrilka
Copy link
Contributor

qrilka commented Dec 29, 2017

Value could be specified as AnyValue from https://stackoverflow.com/a/43328994/110400 but as far as I understand swagger2 can't support specs with not type specified.

@fizruk
Copy link
Member

fizruk commented Dec 31, 2017

λ> import qualified Data.HashMap.Strict as HM
λ> validateToJSON ( HM.fromList [("a", Number 1)] :: Object)
["property \"a\" is found in JSON value, but it is not mentioned in Swagger schema"]

Yeah, I see what I did there with "not mentioned" fields.
How bad is it for your particular use case though?

Value could be specified as AnyValue from https://stackoverflow.com/a/43328994/110400

Interesting, I didn't know type is an optional field!
I guess we should update swagger2 to support that.

@qrilka
Copy link
Contributor

qrilka commented Jan 3, 2018

As of now we've downgraded to version 2.1.6 but actually this new strictness in validation showed up minor inconsistency in one of our legacy endpoints which actually needs to be resolved.
And as for AnyValue I don't see a need for it in our application.

@fizruk
Copy link
Member

fizruk commented Feb 22, 2018

I think we can add an instance for Value once #138 is done.

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

No branches or pull requests

4 participants