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

Improve api consistency #42

Closed
ghostbuster91 opened this issue Jan 27, 2019 · 3 comments
Closed

Improve api consistency #42

ghostbuster91 opened this issue Jan 27, 2019 · 3 comments
Labels
question Further information is requested

Comments

@ghostbuster91
Copy link
Contributor

ghostbuster91 commented Jan 27, 2019

I wonder if it is a good thing that Endpoint has so flexible api, e.g. following case are equivalent:

val booksListing: Endpoint[Limit, String, Vector[Book], Nothing] = baseEndpoint.get
    .in("list" / "all")
    .in(limitParameter)
    .out(jsonBody[Vector[Book]])
val booksListing: Endpoint[Limit, String, Vector[Book], Nothing] = baseEndpoint.get
    .in("list" / "all" and limitParameter)
    .out(jsonBody[Vector[Book]])
val booksListing: Endpoint[Limit, String, Vector[Book], Nothing] = baseEndpoint.get
    .in("list" / "all" / limitParameter)
    .out(jsonBody[Vector[Book]])
val booksListing: Endpoint[Limit, String, Vector[Book], Nothing] = baseEndpoint.get
    .in("list" / "all" & limitParameter)
    .out(jsonBody[Vector[Book]])

where limitParameter is definied as follow:

private val limitParameter = query[Option[Int]]("limit")
        .description("Maximum number of books to retrieve")

What's even worse, somebody can just type:

val booksListing: Endpoint[(Limit,Limit) String, Vector[Book], Nothing] = baseEndpoint.get
    .in("list" / "all" & limitParameter / "something" / limitParamter)
    .out(jsonBody[Vector[Book]])

which is a pure abomination :)

Doesn't such ambiguity make adoption slower?

In opposite to that I was thinking about something like this:

val booksListing: Endpoint[Limit, String, Vector[Book], Nothing] = baseEndpoint.get
    .in("list" / "all" ? limitParameter & otherQueryParam)
    .out(jsonBody[Vector[Book]])

Where "list" and "all" are both PathParams and only they have this / method to concatenate with other pathParams. Also there is ? method defined on them which allows joining with queryParams and it changes the object type to queryParam so there is no option to add next pathParams anymore (which in contrary is possible with current api).

There are still some things which are unclear with proposed change like:

  • how to concatenate with headers? and other types
  • should we allow specyfing EndpointInput multiple times?

I made a trashy implementation to better visualize my idea.

From it it seems that it is not a good approach due to sealed traits explosion and duplication.

Maybe it would be better to change Endoint.in method to accept some kind of higher abstraction dsl which would be internally converted to old EndpointInput structure?

See e8795f7

@ghostbuster91 ghostbuster91 added the question Further information is requested label Jan 27, 2019
@adamw
Copy link
Member

adamw commented Feb 2, 2019

I feared that too many of these combinators might be confusing :) But first, some motivation on why there are two ways to group parameters in the first place. I want to support two scenarios:

  1. creating a description of (possibly multiple) input (output) parameters which can be re-used in many endpoint descriptions. For example, quite often you have paging & limit parameters grouped in listing APIs. You can represent this as: val paging = query[String]("start") and query[Int]("limit") and use it in many endpoints

  2. creating re-useable endpoint descriptions, which can be specialized. For example, in your API all endpoints might have a path which starts with /api and upon error, return a ErrorInfo JSON. You can represent this as: val baseEndpoint = endpoint.in("api").errorOut(jsonBody[ErrorInfo]) and use it as a base endpoint for all other endpoints.

Given the tradeoff between less ways to group parameters and supporting the two use-cases above, I pick the second option.

However, we might still simplify the usages of and, / and &. Maybe it would make sense to drop &, and define / only for paths, that is on PathCapture and PathSegment, not on all EndpointIOs?

@adamw
Copy link
Member

adamw commented Feb 12, 2019

In the end keeping and and / as ways to combine inputs is as much as I could get - essentially dropping &. Any other suggestions welcome :)

@adamw
Copy link
Member

adamw commented Feb 27, 2019

Closing as "won't fix" for the reasons above. Feel free to reopen if you've got new ideas :)

@adamw adamw closed this as completed Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants