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

Honor custom types for array items #1131

Closed
wants to merge 2 commits into from
Closed

Conversation

kelnos
Copy link
Member

@kelnos kelnos commented Jun 5, 2021

Previously this only worked for protocol objects; now it also works for parameters and in-line request bodies.

Addresses #738

@kelnos kelnos requested a review from blast-hardcheese June 5, 2021 06:58
def propMetaWithName[L <: LA, F[_]](tpe: L#Type, property: Tracker[Schema[_]])(
def propMetaWithName[L <: LA, F[_]](tpe: L#Type, property: Tracker[Schema[_]], arrayTypeLifter: (L#Type, Option[L#Type]) => F[L#Type])(
Copy link
Member Author

@kelnos kelnos Jun 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love arrayTypeLifter, but one of my goals here was to use the same type resolution code for protocol objects and parameters. The issue here is that for protocol objects we want to use a concrete type like Vector, but for client parameters, we don't care which type the caller passes, so we want to use Iterable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, it appears that this ends up using the abstract container type for server routes, which almost certainly will not work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I guess that was always happening. For Java stdlib it's not a problem since it's always a concrete type, and I guess for the Scala stuff the routes don't have types listed? But for Java Vavr I'm using Seq now, which will not work. I guess I can go back to Vector. Hrm.

@kelnos kelnos force-pushed the fix-custom-type-in-params branch from 4b7e4da to 3b65406 Compare June 5, 2021 08:21
Previously this only worked for protocol objects; now it also works for
parameters and in-line request bodies.
@kelnos kelnos force-pushed the fix-custom-type-in-params branch from 3b65406 to 84d2583 Compare June 5, 2021 08:35
@kelnos
Copy link
Member Author

kelnos commented Jun 5, 2021

Ugh, so this does seem to break things, but because we were arguably doing questionably-wrong things previously. Every array param was always Iterable[String] even when it was not a string. I guess we just never had tests for those cases?

But now that it does the right thing, the defaults break when the real type isn't String, e.g.:

[error] /home/brian/src/twilio/guardrail/modules/sample-akkaHttp/target/generated/examples/client/akkaHttp/pet/PetClient.scala:88:61: type mismatch;
[error]  found   : String("available")
[error]  required: examples.client.akkaHttp.definitions.PetStatus
[error]   def findPetsByStatus(status: Iterable[PetStatus] = Vector("available"), headers: List[HttpHeader] = Nil): EitherT[Future, Either[Throwable, HttpResponse], FindPetsByStatusResponse] = {

And also it can't properly generate routes on the server for non-string params:

[error] /home/brian/src/twilio/guardrail/modules/sample-akkaHttp/target/generated/issues/issue249/server/akkaHttp/Routes.scala:35:106: type mismatch;
[error]  found   : akka.http.scaladsl.common.RepeatedValueReceptacle[issues.issue249.server.akkaHttp.definitions.Baz]
[error]  required: akka.http.scaladsl.server.directives.ParameterDirectives.ParamSpec
[error]       path("foo")(get(parameter(Symbol("bar").as[_root_.issues.issue249.server.akkaHttp.definitions.Baz].*).map(xs => Option(xs).filterNot(_.isEmpty).map(x => x.toVector)).apply(bar => discardEntity(complete(handler.doFoo(DoFooResponse)(bar))))))

@kelnos
Copy link
Member Author

kelnos commented Jun 5, 2021

I'm actually surprised the latter thing doesn't work, because I thought you were generating akka unmarshallers automatically from circe decoders?

@blast-hardcheese
Copy link
Member

@kelnos This is definitely done for non-array members, though I've had to restrict the Directives stuff quite a bit to prevent overeager JSON decoders, for instance requiring JSON-encoded path parameters like /foo/%22bar%22/baz (JSON string literals, including quotes)

I'll take a look at this sometime this week, thank you for driving this

@kelnos
Copy link
Member Author

kelnos commented Jun 7, 2021

Ah, makes sense. Just to summarize, there are three issues:

  1. This will cause server parameters to use the 'abstract' type Iterable for the types of route params, which definitely will break Java Vavr, not sure about others.
  2. Scala default arg values are now broken when the type isn't String.
  3. akka-http doesn't have the required unmarshallers in place to handle e.g. enum query parameters.

@github-actions github-actions bot added the guardrail Pertains to guardrail label Oct 29, 2021
@blast-hardcheese
Copy link
Member

Going to close this in favor of #1407, all functionality here should be reflected in the new unified codepath.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guardrail Pertains to guardrail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants