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

Improved Swagger Generation #220

Open
zarthross opened this issue Feb 11, 2018 · 6 comments
Open

Improved Swagger Generation #220

zarthross opened this issue Feb 11, 2018 · 6 comments

Comments

@zarthross
Copy link
Member

The swagger for my services are often generated incorrectly because I use custom JSON codecs, or my data types are recursive etc.
While I can override this with SwaggerFormats.withSerializers I feel like their should be an easier/better way.
(Side note... you can modify Model and Property this way, but not Parameters)

Would it be possible, or desirable to instead of using a TypeTag have a some other way of gathering the metadata that allows me as a Rho user to specify the swagger model when I define my datatype in cases the original method doesn't work?

I'm thinking a typeclass approach with a default version that uses the TypeTag like the current does. Something not dissimiliar from 'CodecJson' in argonaut.

I would be happy to submit a PR to fix this.. Seeking input before I get started.

@jcranky
Copy link
Contributor

jcranky commented Feb 12, 2018

I also had this kind of problem in the past, so if you can come up with a clean way to fix that, that would be nice :D

@chuwy
Copy link
Contributor

chuwy commented Oct 1, 2018

I got over-excited about this one recently and even started to work on typeclass-based approach when stumbled upon the erasure in RhoService. Basically, we have routers: Seq[RhoRoute[F, _ <: HList] during Rho middleware invocation, which means we don't have compile-time information about contents of that HList in RhoRoute and hence can pull it only with reflections.

I believe that replacing that Seq with another top-level-HList would help us to get rid of reflections. Is there anything obvious (apart from drastic API change) that I'm missing here?

Also, what would be preferable API? Are we thinking that defining instances of hypothetical SwaggerModel typeclass would be good enough? E.g. we can do:

case class Foo(a: String)

object Foo {
  implicit val model: SwaggerModel[Foo] =
    deriveModel[Foo].copy(description = "My own model")
}

Does it look sufficient?

@zarthross
Copy link
Member Author

So, I also started this a while back and unfortunately it fell on the back burner.

Here was some of my work: master...zarthross:Swagger-Typeclasses

I was taking the approach of having each route capture its metadata rather than doing a middleware approach like it sounds like you were taking.

Your idea for a 'SwaggerModel' is not dissimilar from what I was doing.

Would you consider collaborating on this?

The only downside to the approach I was starting is it starts to become more closely tied with swagger/open-api specifically with less/no support for things like HAL... but it probably just depends on if teh 'ResultMetadata' has enough information.

@chuwy
Copy link
Contributor

chuwy commented Oct 2, 2018

Hey @zarthross! This is great to see some work has been done already!

it starts to become more closely tied with swagger/open-api

That's one of the reasons I wanted to go with middleware approach. It would allow us to not depend on particular representation of type/model (e.g. ResultMetadata and pretty much any class we can define will be lossy compared to reflection types) and perform lossy transformations (such as Rho -> Swagger) only in the end (when HttpService is generated).

In other words, each particular createRhoMiddleware-function could define its own typeclass requirements (SwaggerModel, XmlDefinition, JustGetClassName, whatever) and accept HList of HLists to use those instances.

Nevertheless, these are just thoughts aloud about "ideal solution", I don't know how to implement it and will be happy with any non-reflection solution.

Would you consider collaborating on this?

I think we can start with the PR. I don't have much time on it, but task is super-interesting, so I'll try to do my best to assist.

@zarthross
Copy link
Member Author

I agree that my approach was going to be at lossy. I was trying to have a minimal API change, and minimize some pain points, but perhaps something more drastic will be required.

The first hurdle with your path is going to be the way the RhoService and CompileService throw away the type information. Once you tackle that you now have the incomprehensible monster of an HList of RhoRoutes for each service. Its going to be really difficult to tell a user which thing in the HList caused the implicit resolution failure... Rho is already hard enough to use at times because of its use of typeclasses and HLists.

Next you'll need to change the RhoRoute to keep type info for its outputs, since right now it only keeps the route parameters as type parameters and it stores only the metadata and the Types for its return.

We'd probably need to change how a RhoService is created from being a class inheritance to being a function so we can capture this data and let type inference do its thing...

I can see the upsides: Mainly the ability to get all possible data at the middleware for swagger/etc generation, but i suspect the API changes and the usability might be too much of an issue to overcome....

Thoughts?

@chuwy
Copy link
Contributor

chuwy commented Oct 2, 2018

I totally agree, API change will be huge. Apart from things you've mentioned: RhoService uses in-place append on VectorBuffer, which won't be possible with HLists and thus we'll need to change a way users define RhoServices:

object MyService extends RhoService[F] {
  // Each endpoint is a RhoRoute[F, R <: HList] as it is now
  private val firstEndpoint = GET / 'foo => action1
  private val secondEndpoint = POST / 'bar => action2

  // routes is an HList of RhoRoutes, RhoService defines this member
  override val routes = firstEndpoint or secondEndpoint
}

Which looks less magical and more composable than current API. Maybe we'll end up with something more like Typedapi or Servant.

But what is more important is that whether or not community thinks it makes sense to define routes as HList, good first step towards there is to get rid of runtime reflection. So, I'd like to know which of following paths can be taken (have chance to be merged):

  1. Lossy model without reflection
  2. Lossy model without reflection first, evolved to heterogeneous routes afterwards
  3. None, we're happy with reflection

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

No branches or pull requests

3 participants