servant-swagger-ui icon indicating copy to clipboard operation
servant-swagger-ui copied to clipboard

Generalise over the Swagger datatype.

Open kosmikus opened this issue 3 years ago • 9 comments

This change modifies the previously introduced generalisation of the Haskell datatype that holds the Swagger specification to an aeson Value, see 6576953c39bf1407813ba1880525ed08ea8af9eb

By using an aeson Value, all ordering information of object fields is being lost. This information is in principle preserved by aeson, when going via Encoding rather than Value.

This effectively introduces a regression when used with swagger2. For openapi3, there's an independent problem that the Schema type does not have a proper definition of toEncoding, so openapi3 loses ordering information of object fields independently right now.

With this patch, we do not perform any aeson-related translation in the servant-swagger-ui code, and simply use the original datatype. It is servant's own responsibility to handle the actual conversion of the datatype to JSON. This means that all the servant-swagger-ui packages need neither a dependency on the swagger2/openapi3 packages, nor on aeson. The "cost" is an additional type parameter of the SwaggerSchemaUI type.

This is an interface-breaking change.

kosmikus avatar May 12 '21 08:05 kosmikus

Thanks! This was my original intention, but I did not want to introduce a type parameter and thus break the package API.

BTW, why is field order important for your use case?

maksbotan avatar May 12 '21 13:05 maksbotan

Thanks for the quick reply!

Regarding your question, I think it's very strange if you have some JSON object with schema

{ firstname : string
  lastname : string
  starttime : date
  endtime : date
  txt1 : string
  txt2 : string
  txt3 : string
}  

and it ends up being displayed as:

{ endtime : date,
  txt3 : string
  firstname : string
  txt1 : string
  lastname : string
  starttime : date
  txt2 : string
}

instead. And that's still a relatively modest example with only seven fields. Documentation is intended for a human reader, so order carries logical content.

If breaking the API really is a concern (I think it's a modest change, with an easy migration path), then another option would be to introduce something like

data EncodedJSON = EncodedJSON Value Encoding

instance ToJSON Encoding where
  toJSON = ...
  toEncoding = ...

and use that instead of Value as the specialised type. I think it's much more elegant to simply abstract over the type itself though, and make this package independent of aeson.

kosmikus avatar May 12 '21 15:05 kosmikus

Aha, I see your point now, thanks. But does using toEncoding for schema really guarantee correct field order when they are generated with Generic mechanisms?

Re breaking change: I propose to mark current Value-based type alias as deprecated, add a new generalized one and make a new release. Then in a couple of releases remove old alias and aeson dependency. What do you think?

maksbotan avatar May 12 '21 16:05 maksbotan

I don't think it's part of the actual specification or documentation of Encoding, unfortunately, but in practice I think it guarantees preservation of order and is likely to continue doing so, because the whole point of Encoding is to be efficient, so output is likely to be produced in the order it's generated, and not to end up in some intermediate data structure that could destroy the ordering.

kosmikus avatar May 12 '21 18:05 kosmikus

@maksbotan As to migration path, I think my personal opinion would be that it's not worth it (people who don't want the change right now would not be forced to upgrade immediately). But if you want to preserve the old API, temporarily or not, then I'd probably still use the wrapped Encoding approach described above for the transition period.

kosmikus avatar May 13 '21 09:05 kosmikus

FWIW, my migration patch to the version in this patch was just adding OpenApi to the type API = ... definition:

-    :<|> SwaggerSchemaUI "swagger-ui" "swagger.json"
+    :<|> SwaggerSchemaUI "swagger-ui" "swagger.json" OpenApi

phadej avatar May 13 '21 11:05 phadej

Are there plans to have this, or the rebase in #92 merged, @maksbotan or @phadej?

svobot avatar Jun 20 '22 12:06 svobot

Ping. Can this be merged?

arybczak avatar Jul 05 '22 08:07 arybczak

Ping. Can this be merged please?

arybczak avatar Jul 20 '22 12:07 arybczak