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

issue #222 #234

Merged
merged 36 commits into from May 3, 2019
Merged

issue #222 #234

Changes from 12 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
062ffb5
Added test case for issue 222
Apr 9, 2019
08f9932
It finally compiles! Except for java :(
Apr 12, 2019
2ebdf07
Merge branch 'master' into issue222
tomasherman Apr 12, 2019
28b1b6b
Added test case for issue 222
Apr 25, 2019
cc1cc1b
Merge branch 'issue222' of github.com:avast/guardrail into issue222
Apr 25, 2019
184a5b9
Test passes
Apr 25, 2019
e014a04
Merge branch 'master' into issue222
tomasherman Apr 25, 2019
61d15f2
Reimplemented
Apr 25, 2019
5cd28fb
Merge branch 'issue222' of github.com:avast/guardrail into issue222
Apr 25, 2019
ee39921
Attempt to fix jacksonˆ
Apr 25, 2019
7a9b6da
Removed unused values
Apr 25, 2019
e525c60
Scan all parents for discriminators
Apr 25, 2019
db374e4
Cleanup
Apr 26, 2019
89cc0c4
Update modules/codegen/src/main/scala/com/twilio/guardrail/generators…
blast-hardcheese Apr 26, 2019
d2e7b96
Code review improvements #2
Apr 26, 2019
f244636
Merge branch 'master' into issue222
tomasherman Apr 26, 2019
cf0ddf9
Merge branch 'master' into issue222
tomasherman Apr 27, 2019
8c636a8
Removed comment
Apr 27, 2019
c3a8d6f
Merge branch 'issue222' of github.com:avast/guardrail into issue222
Apr 27, 2019
8411ede
Merge branch 'master' into issue222
tomasherman Apr 30, 2019
0efb82e
Merge branch 'master' of https://github.com/twilio/guardrail into iss…
May 1, 2019
e97efbd
Added another test case and implementation
May 1, 2019
0d54cab
Improved 2nd test case scenario
May 1, 2019
2a7e2fc
Merge branch 'issue222' of github.com:avast/guardrail into issue222
May 1, 2019
8aec398
Added another test case
May 1, 2019
83a5cfc
Added better error reporting when resolving references
May 1, 2019
bb38c38
Scalafmt
May 1, 2019
cb96779
Attempt to reduce filename sizes
May 1, 2019
f81912b
Update modules/codegen/src/main/scala/com/twilio/guardrail/generators…
blast-hardcheese May 2, 2019
5a75f7a
Merge branch 'master' of https://github.com/twilio/guardrail into iss…
May 2, 2019
20ad1f6
Merge branch 'issue222' of github.com:avast/guardrail into issue222
May 2, 2019
6c37f8a
Merge branch 'master' into issue222
tomasherman May 2, 2019
c0b3ddd
Merge branch 'master' into issue222
tomasherman May 2, 2019
fcbddde
Reverted changes to jackson generation
May 2, 2019
b5bfc35
Merge branch 'master' into issue222
tomasherman May 3, 2019
aa2ba14
Merge branch 'master' into issue222
blast-hardcheese May 3, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -41,6 +41,7 @@ val exampleCases: List[(java.io.File, String, Boolean, List[String])] = List(
(sampleResource("issues/issue164.yaml"), "issues.issue164", false, List.empty),
(sampleResource("issues/issue215.yaml"), "issues.issue215", false, List.empty),
(sampleResource("issues/issue218.yaml"), "issues.issue218", false, List.empty),
(sampleResource("issues/issue222.yaml"), "issues.issue222", false, List.empty),
(sampleResource("multipart-form-data.yaml"), "multipartFormData", false, List.empty),
(sampleResource("petstore.json"), "examples", false, List("--import", "support.PositiveLong")),
(sampleResource("plain.json"), "tests.dtos", false, List.empty),
@@ -246,7 +246,7 @@ object ProtocolGenerator {
val isRequired = requiredFields.contains(name)
SwaggerUtil.propMeta[L, F](prop).flatMap(transformProperty(clsName, needCamelSnakeConversion, concreteTypes)(name, prop, _, isRequired))
})
defn <- renderDTOClass(clsName, params, parents)
defn <- renderDTOClass(clsName, params, parents) //check for discriminator, see issue222
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator

Unnecessary comment -- The test infrastructure prevents regression, trying to work around the comments is undesirable.

This comment has been minimized.

Copy link
@tomasherman

tomasherman Apr 26, 2019

Author Contributor

the idea was to add some messages for future generations trying to understand the code

deps = params.flatMap(_.dep)
encoder <- encodeModel(clsName, needCamelSnakeConversion, params, parents)
decoder <- decodeModel(clsName, needCamelSnakeConversion, params, parents)
@@ -361,6 +361,7 @@ object ProtocolGenerator {

definitions.partitionEither({
case (cls, model) =>
// mark ClassParents iff present and has children, otherwise left as model without hierarchies
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 26, 2019

Collaborator

Comments should describe why something is the way it is, not how to do it.

the motivation being two things:

  • The types should drive correctness
  • The code should be terse and intelligible without side effects (where possible)
classHierarchy(cls, model).filterNot(_.children.isEmpty).toLeft((cls, model))
})
}
@@ -399,7 +400,9 @@ object ProtocolGenerator {
parents <- extractParents(comp, definitions, concreteTypes)
model <- fromModel(clsName, comp, parents, concreteTypes)
alias <- modelTypeAlias(clsName, comp)
} yield model.getOrElse(alias)
} yield {
model.getOrElse(alias)
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator

Unnecessary change

}

case arr: ArraySchema =>
fromArray(clsName, arr, concreteTypes)
@@ -171,11 +171,13 @@ object CirceProtocolGenerator {

case RenderDTOClass(clsName, selfParams, parents) =>
val discriminators = parents.flatMap(_.discriminators)
val parenOpt = parents.headOption
val parentOpt = parents.headOption
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator
Suggested change
val parentOpt = parents.headOption
val parentOpt = if (parents.exists(s => s.discriminators.nonEmpty)) { parents.headOption } else { None }
val terms = (parents.reverse.flatMap(_.params.map(_.term)) ++ selfParams.map(_.term)).filterNot(
param => discriminators.contains(param.name.value)
)
val code = parenOpt

val code = parentOpt
.filter(p => parents.exists(s => s.discriminators.nonEmpty)) // part of issue222
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator

This is not the right place to do this filter -- parentOpt should not even be able to be constructed. See the suggestion above.

.fold(q"""case class ${Type.Name(clsName)}(..${terms})""")(
parent =>
q"""case class ${Type.Name(clsName)}(..${terms}) extends ${template"..${init"${Type.Name(parent.clsName)}(...$Nil)" :: parent.interfaces
@@ -56,9 +56,11 @@ object JacksonGenerator {
parentOpt.foreach({ parent =>
val directParent = JavaParser.parseClassOrInterfaceType(parent.clsName)
val otherParents = parent.interfaces.map(JavaParser.parseClassOrInterfaceType)
cls.setExtendedTypes(
new NodeList((directParent +: otherParents): _*)
)
if (parent.discriminators.nonEmpty) {
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator

One caveat here is that this only checks to see if the discriminators are nonEmpty on the first parent, hence parentOpt, val parentOpt = parents.headOption.

This deviates from the behaviour in the Circe generator, which checks all parents.exists(s => s.discriminators.nonEmpty)

Recommendation is to consider the implications of this difference, then pick the one that will benefit users the most in the future.

cls.setExtendedTypes(
new NodeList((directParent +: otherParents): _*)
)
}
})

private def lookupTypeName(tpeName: String, concreteTypes: List[PropMeta[JavaLanguage]])(f: Type => Target[Type]): Option[Target[Type]] =
@@ -0,0 +1,91 @@
package core.issues

import com.twilio.guardrail.generators.Http4s
import com.twilio.guardrail.languages.ScalaLanguage
import com.twilio.guardrail.{ClassDefinition, Context, ProtocolDefinitions}
import org.scalatest.{Assertion, FunSuite, Matchers}
import support.SwaggerSpecRunner

class Issue222 extends FunSuite with Matchers with SwaggerSpecRunner {
import scala.meta._

val swagger: String = s"""
|swagger: '2.0'
|info:
| title: someapp
| description: someapp
| version: '1'
|basePath: "/v1"
|schemes:
| - http
|produces:
| - application/json
|paths: {}
|definitions:
| Request:
| description: Request fields with id
| allOf:
| - "$$ref": "#/definitions/RequestFields"
| - type: object
| properties:
| id:
| type: string
| RequestFields:
| description: Request fields
| type: object
| properties:
| state:
| type: integer
|""".stripMargin

test("Ensure case-to-case inheritance is not generated") {
val (ProtocolDefinitions(List(request: ClassDefinition[ScalaLanguage], requestFields: ClassDefinition[ScalaLanguage]), _, _, _), _, _) = runSwaggerSpec(swagger)(Context.empty, Http4s)

val List(reqEncoder, reqDecoder) = request.staticDefns.definitions

val expectedRequestTpe = t"""Request"""

val expectedRequestCls = q"""case class Request(state: Option[BigInt] = None, id: Option[String] = None)"""

val expectedRequestEncoder = q"""
implicit val encodeRequest = {
val readOnlyKeys = Set[String]()
Encoder.forProduct2("state", "id")((o: Request) => (o.state, o.id)).mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
}
"""
val expectedRequestDecoder = q"""
implicit val decodeRequest = Decoder.forProduct2("state", "id")(Request.apply _)
"""


compare(request.tpe, expectedRequestTpe)
compare(request.cls, expectedRequestCls)
compare(reqEncoder, expectedRequestEncoder)
compare(reqDecoder, expectedRequestDecoder)

val expectedFieldsTpe = t"""RequestFields"""
val expectedFieldsCls = q"""case class RequestFields(state: Option[BigInt] = None)"""

val List(fieldsEncoder, fieldsDecoder) = requestFields.staticDefns.definitions

val expectedFieldsEncoder = q"""
implicit val encodeRequestFields = {
val readOnlyKeys = Set[String]()
Encoder.forProduct1("state")((o: RequestFields) => o.state).mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 30, 2019

Collaborator

Isn't "id" missing from the encoder?

This comment has been minimized.

Copy link
@tomasherman

tomasherman Apr 30, 2019

Author Contributor

what do you mean? there is no id in request fields, it's only in request itself ... unless im missing something?

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 30, 2019

Collaborator

Ah, my apologies -- I encountered the error I mentioned below, and I was hasty in looking for an example of the undesired behaviour in what you already had here.

}
"""
val expectedFieldsDecoder = q"""
implicit val decodeRequestFields = Decoder.forProduct1("state")(RequestFields.apply _)
"""

compare(requestFields.tpe, expectedFieldsTpe)
compare(requestFields.cls, expectedFieldsCls)
compare(fieldsEncoder, expectedFieldsEncoder)
compare(fieldsDecoder, expectedFieldsDecoder)
}

private def compare(t1: Tree, t2: Tree): Assertion = {
println(s"$t1 | $t2")
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator

No println please

t1.structure shouldEqual t2.structure
}
}
@@ -444,7 +444,7 @@ class Issue43 extends FunSpec with Matchers with SwaggerSpecRunner {
:: ADT(namePet, tpePet, trtPet, staticDefnsPet) :: ADT(nameCat, tpeCat, trtCat, staticDefnsCat) :: Nil,
_,
_,
_
_3
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 25, 2019

Collaborator

Unnecessary change(?)

),
_,
_
@@ -0,0 +1,26 @@
swagger: '2.0'
info:
title: someapp
description: someapp
version: '1'
basePath: "/v1"
schemes:
- http
produces:
- application/json
paths: {}
definitions:
Request:
description: Request fields with id
allOf:
- "$ref": "#/definitions/RequestFields"
- type: object
properties:
id:
type: string
This conversation was marked as resolved by tomasherman

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 30, 2019

Collaborator

This doesn't render both id and id2

Suggested change
type: string
type: string
- type: object
properties:
id2:
type: string

This doesn't render parameters from any object literals, so id is missing

Suggested change
type: string
type: string
- "$ref": "#/definitions/RequestFields2"
RequestFields:
description: Request fields
type: object
properties:
state:
type: integer
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.