haskell-servant / servant-swagger-ui

Provide embedded swagger UI for servant and swagger
46 stars 34 forks source link

Generalise over the Swagger datatype. #89

Open kosmikus opened 3 years ago

kosmikus commented 3 years ago

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.

maksbotan commented 3 years ago

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?

kosmikus commented 3 years ago

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.

maksbotan commented 3 years ago

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?

kosmikus commented 3 years ago

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 commented 3 years ago

@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.

phadej commented 3 years ago

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
svobot commented 2 years ago

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

arybczak commented 2 years ago

Ping. Can this be merged?

arybczak commented 2 years ago

Ping. Can this be merged please?