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

Treat unknown properties as invalid + some new helpers #126

Merged
merged 6 commits into from
Oct 17, 2017

Conversation

fizruk
Copy link
Member

@fizruk fizruk commented Oct 7, 2017

In some cases an easy mistake in ToJSON or ToSchema instances could make them incompatible while leaving validateToJSON tests passing.

The problem is that unknown properties in a JSON object are ignored when Schema does not have an additionalProperties schema. If Schema also does not have any required properties, then validateToJSON will most certainly pass without a warning or error.

Invalid ToJSON-ToSchema Examples

Mixed sum types and genericDeclareNamedSchemaUnrestricted

Deriving ToSchema for mixed sum types is not allowed since 2.1.5 without an unrestricted version:

data Status
  = StatusOk
  | StatusError String
  deriving (Generic, ToJSON)

instance ToSchema Status where
  declareNamedSchema = genericDeclareNamedSchemaUnrestricted defaultSchemaOptions

Resulting ToJSON and ToSchema instances don't match, and, worse, validateToJSON is silent:

>>> validateToJSON StatusOk
[]

This PR makes this change:

>>> validateToJSON StatusOk
["property \"tag\" is found in JSON value, but it is not mentioned in Swagger schema"]

Newtypes for maps with bounded enum keys

When you have a Map with just a few possible values for keys, it makes sense to list them (and only them) in your Swagger Schema:

data ButtonState = Neutral | Focus | Active
  deriving (Show, Eq, Ord, Bounded, Enum, Generic)

instance ToJSON ButtonState
instance ToSchema ButtonState
instance ToJSONKey ButtonState where toJSONKey = toJSONKeyText (T.pack . show)

type ImageUrl = T.Text

newtype ButtonImages = ButtonImages { getButtonImages :: Map ButtonState ImageUrl }
  deriving (Show, Generic)

instance ToJSON ButtonImages

-- | Use a custom Schema to list all possible keys.
instance ToSchema ButtonImages where
  declareNamedSchema _ = do
    imageUrlRef <- declareSchemaRef (Proxy :: Proxy ImageUrl)
    return $ NamedSchema (Just "ButtonImages") $ mempty
      & type_ .~ SwaggerObject
      & properties.at "Neutral" ?~ imageUrlRef
      & properties.at "Focus"   ?~ imageUrlRef
      & properties.at "Active"  ?~ imageUrlRef

However, in this example ToJSON and ToSchema don't match, yet validateToJSON is silent:

>>> validateToJSON (ButtonImages mempty)
[]

This PR makes this change:

>>> validateToJSON (ButtonImages mempty)
["property \"getButtonImages\" is found in JSON value, but it is not mentioned in Swagger schema"]

Moreover, this PR introduces helper for this sort of ToSchema instances:

instance ToSchema ButtonImages where
  declareNamedSchema = genericDeclareNamedSchemaNewtype defaultSchemaOptions
    declareSchemaBoundedEnumKeyMapping

- genericNameSchema to give schema a Generic-based name
- genericDeclareNamedSchemaNewtype to help declare NamedSchema for newtype
- declareSchemaBoundedEnumKeyMapping for maps with Bounded Enum keys
Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Is there something which I should check more closely?

Copy link
Contributor

@fierce-katie fierce-katie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should move common types used for tests (like Color, Paint, ButtonImages, etc.) to a separate module?

@fizruk
Copy link
Member Author

fizruk commented Oct 17, 2017

@fierce-katie yeah, later :)

@phadej no, just sanity check was needed, thanks! :)

@fizruk fizruk merged commit e2d1f94 into master Oct 17, 2017
@fizruk fizruk deleted the validate-unknown-properties branch October 17, 2017 10:57
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

Successfully merging this pull request may close these issues.

3 participants