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

fix: Remove extra apply in generated multiple query parameters #287

Merged
merged 2 commits into from Jul 7, 2019

Conversation

Projects
None yet
2 participants
@rtfpessoa
Copy link
Contributor

commented May 8, 2019

Trying to fix codegen in #210

When generating the akka code it does not compile. After removing it and compiling just the akka part it compiles in my tests. But seems like the tests in this repo fail. Any ideas?

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

I'm interested to see whether this compiles; this was added to fix #249, it may trip the regression test. I think #250 may have resolved #210 already, but I'm interested as to why you're getting compilation failures about Function1. If you could provide an OpenAPI specification that produces the failure you're seeing, that'd be excellent to take a look at.

@rtfpessoa

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Here is an example that breaks, anything else I can help with?

Edit: Not sure if related but I am using akka-http 10.0.15

swagger: "2.0"
info:
  title: Whatever
  version: 1.0.0
host: localhost:1234
schemes:
  - http
definitions:
  User:
    type: object
    required:
      - id
      - address
    properties:
      id:
        type: string
      address:
        $ref: '#/definitions/UserAddress'
  UserAddress:
    type: object
    properties:
      line1:
        type: string
      line2:
        type: string
      line3:
        type: string
paths:
  /commit/{commitId}/files:
    get:
      tags:
        - file
      summary: Finds all files for commit
      description: 'Finds all files for a commit.'
      operationId: commitCommitIdFiles
      produces:
        - application/json
      parameters:
        - in: path
          name: commitId
          description: Commit identifier
          required: true
          type: integer
          format: int64
          x-example: 1
        - in: query
          name: fileIds
          description: File identifiers
          required: false
          type: array
          items:
            type: integer
            format: int64
          # HACK: Force type Long
          x-scala-type: 'Iterable[Long]'
          collectionFormat: multi
        - in: query
          name: fileNames
          description: File names
          required: false
          type: array
          items:
            type: string
          collectionFormat: multi
      responses:
        '200':
          schema:
            $ref: '#/definitions/User'
        '404':
          description: Not found
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

Ah, it seems as though this is a divergence of behaviour between 2.11 and 2.12. Finding a structure that compiles in both will be interesting. Are you available to try and fix this? If not, I can take a look after I'm done on my current PR, attempting to address #184

@rtfpessoa

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I would gladly do it, but I am not even understanding the exact issue here.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

My understanding is as follows:

parameter(Symbol("foo").as[String].*).map(Option.apply _)

compiled in 2.11 and 2.12. This had the unfortunate effect of returning a Seq[String] with no other validation.

parameter(Symbol("foo").as[String].*).map(xs => Option(xs).filterNot(_.isEmpty))

compiles in 2.12 but not 2.11, due to some syntactic ambiguity and related type divergence.

parameter(Symbol("foo").as[String].*).map(xs => Option(xs).filterNot(_.isEmpty)).apply

compiles in 2.11 and 2.12, and tests pass. That being said, your proffered specification does not compile, exposing a previously untested limitation of this suffixed apply approach.

Can you post the surrounding code that resulted in

[error] missing argument list for method apply in trait Function1
[error] Unapplied methods are only converted to functions when a function type is expected.
[error] You can make this conversion explicit by writing `apply _` or `apply(_)` instead of `apply`.

please? It sounds like we're ending up with something like:

path("foo") {
  parameter(Symbol("foo").as[String].*).map(xs => Option(xs).filterNot(_.isEmpty)).apply
}

causing that apply to not end up with another Directive or Term.Block to be applied against.

@rtfpessoa

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I just generated with ./cli.sh scala --server --specPath modules/sample/src/main/resources/tes1.yaml --outputPath modules/sample-akkaHttp/target/generated --packageName tes1 --framework akka-http

Where tes1.yaml a file with the spec above.

Anyway here is the content of method around the code:

def routes(handler: FileHandler)(implicit mat: akka.stream.Materializer): Route = {
    {
      get(path("commit" / LongNumber / "files")(commitId => (parameter(Symbol("fileIds").as[Long].*).map(xs => Option(xs).filterNot(_.isEmpty)).apply & parameter(Symbol("fileNames").as[String].*).map(xs => Option(xs).filterNot(_.isEmpty)).apply)((fileIds, fileNames) => discardEntity(complete(handler.commitCommitIdFiles(commitCommitIdFilesResponse)(commitId, fileIds, fileNames))))))
    }
  }

And the full full Routes.scala:

/*
 * This file was generated by Guardrail (https://github.com/twilio/guardrail).
 * Modifications will be overwritten; instead edit the OpenAPI/Swagger spec file.
 */
package tes1.file
import akka.http.scaladsl.model._
import akka.http.scaladsl.model.headers.RawHeader
import akka.http.scaladsl.unmarshalling.{ Unmarshal, Unmarshaller, FromEntityUnmarshaller, FromRequestUnmarshaller, FromStringUnmarshaller }
import akka.http.scaladsl.marshalling.{ Marshal, Marshaller, Marshalling, ToEntityMarshaller, ToResponseMarshaller }
import akka.http.scaladsl.server.Directives._
import akka.http.scaladsl.server.{ Directive, Directive0, Directive1, ExceptionHandler, MalformedHeaderRejection, MissingFormFieldRejection, Rejection, Route }
import akka.http.scaladsl.util.FastFuture
import akka.stream.{ IOResult, Materializer }
import akka.stream.scaladsl.{ FileIO, Keep, Sink, Source }
import akka.util.ByteString
import io.circe.Decoder
import cats.{ Functor, Id }
import cats.data.EitherT
import cats.implicits._
import scala.concurrent.{ ExecutionContext, Future }
import scala.language.higherKinds
import scala.language.implicitConversions
import java.io.File
import java.security.MessageDigest
import java.util.concurrent.atomic.AtomicReference
import scala.util.{ Failure, Success }
import scala.language.higherKinds
import _root_.tes1.Implicits._
import _root_.tes1.AkkaHttpImplicits._
import _root_.tes1.definitions._
trait FileHandler { def commitCommitIdFiles(respond: FileResource.commitCommitIdFilesResponse.type)(commitId: Long, fileIds: Option[Iterable[Long]] = None, fileNames: Option[Iterable[String]] = None): scala.concurrent.Future[FileResource.commitCommitIdFilesResponse] }
object FileResource {
  def discardEntity: Directive0 = extractMaterializer.flatMap { implicit mat => 
    extractRequest.flatMap { req => 
      req.discardEntityBytes().future
      Directive.Empty
    }
  }
  def routes(handler: FileHandler)(implicit mat: akka.stream.Materializer): Route = {
    {
      get(path("commit" / LongNumber / "files")(commitId => (parameter(Symbol("fileIds").as[Long].*).map(xs => Option(xs).filterNot(_.isEmpty)).apply & parameter(Symbol("fileNames").as[String].*).map(xs => Option(xs).filterNot(_.isEmpty)).apply)((fileIds, fileNames) => discardEntity(complete(handler.commitCommitIdFiles(commitCommitIdFilesResponse)(commitId, fileIds, fileNames))))))
    }
  }
  sealed abstract class commitCommitIdFilesResponse(val statusCode: StatusCode)
  case class commitCommitIdFilesResponseOK(value: User) extends commitCommitIdFilesResponse(StatusCodes.OK)
  case object commitCommitIdFilesResponseNotFound extends commitCommitIdFilesResponse(StatusCodes.NotFound)
  object commitCommitIdFilesResponse {
    implicit val commitCommitIdFilesTRM: ToResponseMarshaller[commitCommitIdFilesResponse] = Marshaller { implicit ec => 
      resp => commitCommitIdFilesTR(resp)
    }
    implicit def commitCommitIdFilesTR(value: commitCommitIdFilesResponse)(implicit ec: scala.concurrent.ExecutionContext): scala.concurrent.Future[List[Marshalling[HttpResponse]]] = value match {
      case r @ commitCommitIdFilesResponseOK(value) =>
        Marshal(value).to[ResponseEntity].map {
          entity => Marshalling.Opaque {
            () => HttpResponse(r.statusCode, entity = entity)
          } :: Nil
        }
      case r: commitCommitIdFilesResponseNotFound.type =>
        scala.concurrent.Future.successful(Marshalling.Opaque {
          () => HttpResponse(r.statusCode)
        } :: Nil)
    }
    def apply[T](value: T)(implicit ev: T => commitCommitIdFilesResponse): commitCommitIdFilesResponse = ev(value)
    implicit def OKEv(value: User): commitCommitIdFilesResponse = OK(value)
    def OK(value: User): commitCommitIdFilesResponse = commitCommitIdFilesResponseOK(value)
    def NotFound: commitCommitIdFilesResponse = commitCommitIdFilesResponseNotFound
  }
}
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

Oh. Of course that won't work.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

If you runScalaExample and akkaHttpSample/test:compile, you should see the compiler errors that caused this build to fail. If you're inclined to throw some time at figuring out a syntax that makes all use cases work (yours included) I'd be more than happy to merge it and cut a bugfix release. If not, I'll try and find some time to resolve this, as I'm sure it'll bite others sooner than later.

@blast-hardcheese blast-hardcheese force-pushed the twilio:master branch from 6a41c95 to 3f0f265 May 15, 2019

@blast-hardcheese blast-hardcheese referenced this pull request May 24, 2019

Merged

Upgrade http4s to version 0.20.0 #275

1 of 1 task complete

@blast-hardcheese blast-hardcheese force-pushed the rtfpessoa:master branch from 410943e to 272805e Jul 7, 2019

@blast-hardcheese
Copy link
Collaborator

left a comment

Sorry for the delay here, merging now. This'll be in the next release.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

Ok -- when rebasing, the tests continued to fail with the same problem that was originally happening.

I was able to work around it by putting the apply at the point at which we create the inner function literal to consume the supplied fields. This seems to force the magnet pattern to solidify, and type inference works again after that point.

@rtfpessoa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

@blast-hardcheese nice. I tried a couple times, but I was not able to get anything close to the real issue.
Thanks for the help.

@blast-hardcheese blast-hardcheese merged commit eaa9d86 into twilio:master Jul 7, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.