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

Conversation

Projects
None yet
3 participants
@tomasherman
Copy link
Contributor

commented Apr 12, 2019

attempt at #222

@tomasherman tomasherman changed the title Issue222 issue #222 Apr 12, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

So, in this case, I'd add an explicit test, similar to modules/codegen/src/test/scala/core/issues/Issue126.scala, that encapsulates what you hope to see out of your change. Diffing the output of current master against this branch yields the following:

 package issues.issue222.server.http4s.definitions
 import io.circe._
 import io.circe.syntax._
 import io.circe.generic.semiauto._
-case class Request(state: Option[BigInt] = None, id: Option[String] = None) extends RequestFields
+case class Request(id: Option[String] = None)
 object Request {
   implicit val encodeRequest = {
     val readOnlyKeys = Set[String]()
-    Encoder.forProduct2("state", "id") { (o: Request) => (o.state, o.id) }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
+    Encoder.forProduct1("id") { (o: Request) => o.id }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
   }
-  implicit val decodeRequest = Decoder.forProduct2("state", "id")(Request.apply _)
+  implicit val decodeRequest = Decoder.forProduct1("id")(Request.apply _)
 }

which I assume is not your goal. I presume your intent would be to preserve all the keys, just to get rid of the object inheritance, which your PR does partially, just needing a bit more massaging to get to where it needs to be.

Some considerations:

  • What happens if there are multiple $refs?
  • What happens if there are multiple object literals?
  • What happens if the allOf contains an object literal that itself has an allOf?

Not all of these need to be solved right now, I'm very much in favor of accomplishing primarily what is necessary to unblock consumers, but without consideration, it's very easy to implement something that will not be forward source-compatible.

Again, sorry for having blocked you; I'm interested in helping unblock this feature request, so feel free to ask any follow-up questions here.

Tomas Herman and others added some commits Apr 25, 2019

Tomas Herman
Tomas Herman
Tomas Herman
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

WOOOO! I can't believe it I made the tests pass 🎉

@blast-hardcheese
Copy link
Collaborator

left a comment

💯 Thanks for your effort so far! Your strategy is good, this just needs some polishing up before merge.

Tomas Herman and others added some commits Apr 26, 2019

Update modules/codegen/src/main/scala/com/twilio/guardrail/generators…
…/CirceProtocolGenerator.scala

Co-Authored-By: tomasherman <tomas.herman@gmail.com>

tomasherman and others added some commits Apr 27, 2019

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I think i addressed all questions / comments for this PR ... are there any changes needed before this can be merged?

@blast-hardcheese
Copy link
Collaborator

left a comment

There seem to be some gaps in implementation, one that would necessitate an immediate hotfix, another that would silently fail based on the ordering of parameters in a list

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

ok so this one was a doozy ... anyways, i added two more test cases, hopefully fixing your concerns ...

I fixed it but im not really sure why the original logic was the way it was (basically taking just last object from composed schema ... perheps bug and not intentional?

I also added some error handling - guardrail will now fail with error message in case some composed object references definition that does not exist (i wasted like 30 minutes debugging this case when fixing one of those tests i added so i added this validation/error reporting)

Let me know what you think!

Tomas Herman
@blast-hardcheese
Copy link
Collaborator

left a comment

This is wildly cool. Very well done. Thank you!

blast-hardcheese and others added some commits May 2, 2019

Update modules/codegen/src/main/scala/com/twilio/guardrail/generators…
…/CirceProtocolGenerator.scala

Co-Authored-By: tomasherman <tomas.herman@gmail.com>
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@kelnos Brain, i know this is not your issue but the changes you merged yesterday broke this PR...

You seem like the guy that's mostly working on jackson stuff, how do you feel issue #222 should be compiled for Java? Basically it's this hierarchy: https://github.com/twilio/guardrail/blob/20ad1f605006e24f2a5a42c075e672b6b81408cf/modules/sample/src/main/resources/issues/issue222.yaml

In scala, it's implemented such that if there is no discriminator, only the fields are copied without any hierarchy being generated...do you think it's ok to do the same in java?

@kelnos

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@tomasherman I know OO is somewhat controversial, but my opinion here is to implement these as class hierarchies and not flatten them out, for Java at least.

A lack of discriminator just means that the spec author doesn't need polymorphic deserialization, but that doesn't say anything about how the user wants to use the generated classes. Perhaps they've created a hierarchy intentionally and intend to exploit use of the base class so they can operate on all the classes generically? I'd rather not take that possibility away. For people who don't care, the interface to create and interact with the objects is the same either way, so there's no loss to usability by maintaining the hierarchy.

I think this is less an issue in Scala for some hand-wavy ill-defined reasons around programming philosophy, but I'm pretty happy with the behavior I've arrive at with Java/Jackson.

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Fair enough, however the generated code for the scenario i submitted above won't compile

The problem is that both Request and RequestFields have static method builder() ... and in Request the types don't match (you can see the output of travis job) ... do you have an opinion how to solve this?

@kelnos

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@tomasherman sorry, just a sec -- speaking offline with @blast-hardcheese & realizing I was missing context around why your change is being made. Agree that we need to flatten things out in the case where the swagger spec specifies multiple inheritance.

Unfortunately it's not super simple. Reverting my Jackson PR probably won't help, as the old code was garbage at dealing with polymorphism, so I'd expect your new test cases to fail with that code as well. I'm going to attempt to get the Jackson code generating flat objects for the mutiple-inheritance case over the next couple hours. If I can get it done, we'll merge that PR plus yours, and then cut a release. Otherwise we'll cut a release with what's in master now, and I'll continue working so we can get these changes out in the next release.

Really sorry that I hadn't kept tabs on your work well enough to realize we'd be affecting each other and should have been coordinating.

@kelnos kelnos referenced this pull request May 2, 2019

Merged

Flatten Jackson inheritance hierarchies where needed #277

1 of 1 task complete
@kelnos

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Ok, above PR referenced should make Jackson behave properly. I'm going to try to merge our branches together to see if we actually end up with something that works for everything.

@kelnos

This comment has been minimized.

Copy link
Member

commented May 2, 2019

So my test spec doesn't actually build for scala with your branch merged in. Not sure if it's worth fixing, but for reference:

swagger: 2.0
info:
  version: 1.0.0
paths: {}
definitions:
  A:
    type: object
    properties:
      a:
        type: string
  B:
    allOf:
      - $ref: "#/definitions/A"
      - type: object
        properties:
          b:
            type: string
  C:
    type: object
    properties:
      c:
        type: string
  D:
    type: object
    properties:
      d:
        type: string
  E:
    allOf:
      - $ref: "#/definitions/C"
      - $ref: "#/definitions/D"
      - type: object
        properties:
          e:
            type: string
  F:
    type: object
    discriminator: f
    required:
      - f
    properties:
      f:
        type: string
  G:
    type: object
    properties:
      g:
        type: string
  H:
    allOf:
      - $ref: "#/definitions/F"
      - $ref: "#/definitions/G"
      - type: object
        properties:
          h:
            type: string

And the errors I'm getting:

[error] /home/brian/src/twilio/guardrail/modules/sample-akkaHttp/target/generated/polymorphismMultipleInheritance/client/akkaHttp/definitions/H.scala:9:81: class G needs to be a trait to be mixed in
[error] case class H(g: Option[String] = None, h: Option[String] = None) extends F with G
[error]                                                                                 ^
[error] /home/brian/src/twilio/guardrail/modules/sample-akkaHttp/target/generated/polymorphismMultipleInheritance/client/akkaHttp/definitions/H.scala:9:12: case class H has case ancestor polymorphismMultipleInheritance.client.akkaHttp.definitions.G, but case-to-case inheritance is prohibited. To overcome this limitation, use extractors to pattern match on non-leaf nodes.
[error] case class H(g: Option[String] = None, h: Option[String] = None) extends F with G
[error]            ^
[error] /home/brian/src/twilio/guardrail/modules/sample-akkaHttp/target/generated/polymorphismMultipleInheritance/server/akkaHttp/definitions/H.scala:9:81: class G needs to be a trait to be mixed in
[error] case class H(g: Option[String] = None, h: Option[String] = None) extends F with G
[error]                                                                                 ^
[error] /home/brian/src/twilio/guardrail/modules/sample-akkaHttp/target/generated/polymorphismMultipleInheritance/server/akkaHttp/definitions/H.scala:9:12: case class H has case ancestor polymorphismMultipleInheritance.server.akkaHttp.definitions.G, but case-to-case inheritance is prohibited. To overcome this limitation, use extractors to pattern match on non-leaf nodes.
[error] case class H(g: Option[String] = None, h: Option[String] = None) extends F with G
[error]            ^
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Thanks for the effort! I think it's definitely worth fixing ... i will take look at this over the weekend and try to implement this properly, but if im not able to, can we agree to merge what i've done here? It's strictly better than what is curently implemented in guardrail and it will allow us (avast) to use it in our usecases and we can contribute further improvements along in the future

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

Merging this now, further bugfixes for #234 (comment) can be in future releases. Better to have the new functionality and iterate on what we have for now.

Again, thank you for your diligent work on this PR!

@blast-hardcheese blast-hardcheese merged commit 8f57075 into twilio:master May 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

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.