schema-profunctor: Add ToSchema instance for lists #1497

akshaymankar · 2021-05-11T07:38:57Z

No description provided.

pcapriotti · 2021-05-11T07:41:29Z

libs/schema-profunctor/src/Data/Schema.hs

@@ -563,6 +563,8 @@ instance ToSchema String where schema = genericToSchema

 instance ToSchema Bool where schema = genericToSchema

+instance (S.ToSchema a, A.ToJSON a, A.FromJSON a)=> ToSchema [a] where schema = genericToSchema


I would prefer this:

Suggested change

instance (S.ToSchema a, A.ToJSON a, A.FromJSON a)=> ToSchema [a] where schema = genericToSchema

instance ToSchema a => ToSchema [a] where schema = array schema

Hmm, is there a difference. This does look better, asking for my education.
I also pushed another instance for NonEmpty a I am guessing array wouldn't produce a nice ToSchema instance for that, so we should use genericToSchema in that case?

Actually, I can see the difference, if we the one I made, it would ignore a ToSchema explicitly defined for a.

I think genericToSchema should only be used for bootstrapping primitive types, but not for instances that recurse over previously defined schemas. There isn't a huge difference in this case, but maybe in the future we might get rid of the S.ToSchema instances altogether (and simply convert the top-level SchemaP's to a named swagger schema when we generate the swagger), in which case this wouldn't work.

Hmm, array schema doesn't work as it doesn't produce a named schema, so either I need a TypeRep, which would be annoying or I need to set it to something like an empty string.

Ah, you're right, on both counts.

I think it's better not to have this instance. One can just use array when you need an array, since lists are indeed most commonly unnamed.

Similarly, I would add a nonEmptyArray combinator instead of the NonEmpty instance.

pcapriotti · 2021-05-11T07:55:19Z

libs/schema-profunctor/src/Data/Schema.hs

@@ -563,6 +564,10 @@ instance ToSchema String where schema = genericToSchema

 instance ToSchema Bool where schema = genericToSchema

+instance (S.ToSchema a, A.ToJSON a, A.FromJSON a) => ToSchema [a] where schema = genericToSchema
+
+instance (S.ToSchema a, A.ToJSON a, A.FromJSON a) => ToSchema (NonEmpty a) where schema = genericToSchema


I would also like to define this in terms of ToSchema a rather than S.ToSchema a. It should be possible to use array, and then withParser to convert it into a non-empty list.

…oSchema Declaring instances using S.ToSchema and A.{To,From}JSON is a little problematic as it wouldn't use an explicitly defined P.ToSchema, so instead of using instances for P.ToSchema, users will be expected to use the combinators as required.

pcapriotti · 2021-05-11T09:11:52Z

libs/schema-profunctor/src/Data/Schema.hs

+nonEmptyArray ::
+  forall ndoc doc a.(HasArray ndoc doc, HasName ndoc, S.HasMinItems doc (Maybe Integer)) =>
+  ValueSchema ndoc a ->
+  ValueSchema doc (NonEmpty a)
+nonEmptyArray sch = SchemaP (SchemaDoc s) (SchemaIn r) (SchemaOut w)
+  where
+    name = maybe "non-empty array" ("non-empty array of " <>) (getName (schemaDoc sch))
+    r = A.withArray (T.unpack name) $ \arr -> case V.toList arr of
+      [] -> A.parseFail "Unexpected empty array found while parsing a NonEmpty"
+      (x : xs) -> mapM (schemaIn sch) (x :| xs)
+    s = mkArray (schemaDoc sch) & S.minItems ?~ 1
+    w xs = A.Array . V.fromList <$> mapM (schemaOut sch) (NonEmpty.toList xs)
+


It should be possible to use array instead of reimplementing its functionality. Something like:

nonEmptyArray sch = NonEmpty.toList .= array sch `withParser` check where check = maybe (fail "Unexpected empty array found while parsing a NonEmpty") pure . NonEmpty.nonEmpty

I couldn't make the swagger work nicely, thought it was ok to have a small amount of duplication. Maybe we can look at it together after lunch to get it to work?

pcapriotti · 2021-05-11T09:14:24Z

libs/schema-profunctor/src/Data/Schema.hs

@@ -316,6 +318,19 @@ array sch = SchemaP (SchemaDoc s) (SchemaIn r) (SchemaOut w)
    s = mkArray (schemaDoc sch)
    w x = A.Array . V.fromList <$> mapM (schemaOut sch) x

+nonEmptyArray ::
+  forall ndoc doc a.(HasArray ndoc doc, HasName ndoc, S.HasMinItems doc (Maybe Integer)) =>


Not super important, but the combinators do not depend on Swagger so far. Would it possible to have a locally defined type class to set the minimum, instead of using S.HasMinItems?

I first added it to HasArray, but it didn't look very nice as the lens doesn't depend on the ndoc variable and it has to be type applied. I will defined a similar class, that should also work.

Did this now.

…r2 class

pcapriotti

Apart from the possible simplification of the minItems lens, looks good to me.

pcapriotti · 2021-05-11T12:10:14Z

libs/schema-profunctor/src/Data/Schema.hs

+instance HasMinItems SwaggerDoc (Maybe Integer) where
+  minItems =
+    lens
+      (\(WithDeclare _ s) -> s & view S.minItems)
+      (\(WithDeclare d s) mInt -> WithDeclare d $ s & S.minItems .~ mInt)


I think this one could simply be a lens composition: minItems = declared . S.minItems or similar.

akshaymankar requested a review from pcapriotti May 11, 2021 07:38

pcapriotti reviewed May 11, 2021

View reviewed changes

schema-profunctor: Add ToSchema instance for lists

e984d23

akshaymankar force-pushed the akshaymankar/schema-list branch from e7f8495 to e984d23 Compare May 11, 2021 07:45

pcapriotti reviewed May 11, 2021

View reviewed changes

akshaymankar added 7 commits May 11, 2021 12:47

schema-profunctor: Add tests for nonEmptyArray

511523b

schema-profuctor: Add example for lists and non-empty lists

e7d68f9

schema-profunctor: Redefine HasMinItems to avoid dependency on swagge…

d581b51

…r2 class

schema-profuctor: Use inline schemas for lists in example and tests

c2b4c1a

Formatting

8c5de6f

schema-profuctor: Reudce duplication in nonEmptyArray

534eebd

Fix imports

5e65e24

pcapriotti approved these changes May 11, 2021

View reviewed changes

schema-profuctor: Simplify HasMinItems instance for SwaggerDoc

f112bcb

akshaymankar merged commit b16ed84 into develop May 11, 2021

akshaymankar deleted the akshaymankar/schema-list branch May 11, 2021 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema-profunctor: Add ToSchema instance for lists #1497

schema-profunctor: Add ToSchema instance for lists #1497

akshaymankar commented May 11, 2021

pcapriotti May 11, 2021

akshaymankar May 11, 2021

akshaymankar May 11, 2021

pcapriotti May 11, 2021

akshaymankar May 11, 2021

pcapriotti May 11, 2021

pcapriotti May 11, 2021

pcapriotti May 11, 2021

akshaymankar May 11, 2021

pcapriotti May 11, 2021

akshaymankar May 11, 2021

akshaymankar May 11, 2021

pcapriotti left a comment

pcapriotti May 11, 2021

		@@ -563,6 +563,8 @@ instance ToSchema String where schema = genericToSchema

		instance ToSchema Bool where schema = genericToSchema

		instance (S.ToSchema a, A.ToJSON a, A.FromJSON a)=> ToSchema [a] where schema = genericToSchema

	instance (S.ToSchema a, A.ToJSON a, A.FromJSON a)=> ToSchema [a] where schema = genericToSchema
	instance ToSchema a => ToSchema [a] where schema = array schema

schema-profunctor: Add ToSchema instance for lists #1497

schema-profunctor: Add ToSchema instance for lists #1497

Conversation

akshaymankar commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcapriotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment