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

Case-to-Case inehritance generated #222

Open
tomasherman opened this issue Mar 31, 2019 · 31 comments

Comments

Projects
None yet
3 participants
@tomasherman
Copy link
Contributor

commented Mar 31, 2019

Hey,

i came across this case of invalid scala code generated - basically if you have a definition which contains allOf with just one other definition, case class extension hierarchy is generated.

The swagger.json:

{
  "swagger" : "2.0",
  "info" : {
    "title" : "someapp",
    "description" : "someapp",
    "version" : "1"
  },
  "basePath" : "/v1",
  "schemes" : [ "http", "https" ],
  "produces" : [ "application/json" ],
  "paths": {},
  "definitions": {
    "PairedDeviceInfo" : {
      "description" : "Data about a device's pairing.",
      "allOf" : [ {
        "$ref" : "#/definitions/PairedDeviceFields"
      } ]
    },
    "PairedDeviceFields" : {
      "description" : "Fields describing the pairing of a managed / paired device.",
      "type" : "object",
      "properties" : {
        "status" : {
          "type": "integer"
        }
      },
      "required" : [ "status", "deviceState" ]
    }
  }
}

scala code:

// PairedDeviceFields.scala
case class PairedDeviceFields(status: BigInt)
object PairedDeviceFields {
  implicit val encodePairedDeviceFields = {
    val readOnlyKeys = Set[String]()
    Encoder.forProduct1("status") { (o: PairedDeviceFields) => o.status }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
  }
  implicit val decodePairedDeviceFields = Decoder.forProduct1("status")(PairedDeviceFields.apply _)
}

// PairedDeviceInfo.scala
case class PairedDeviceInfo(status: BigInt) extends PairedDeviceFields
object PairedDeviceInfo {
  implicit val encodePairedDeviceInfo = {
    val readOnlyKeys = Set[String]()
    Encoder.forProduct1("status") { (o: PairedDeviceInfo) => o.status }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
  }
  implicit val decodePairedDeviceInfo = Decoder.forProduct1("status")(PairedDeviceInfo.apply _)
}

I would expect either a type alias or some traits to be generated ...

replicable example can be found here: https://github.com/tomasherman/guardrail-playground

@tomasherman tomasherman changed the title Uncompilable scala code generated Case-to-Case inehritance generated Mar 31, 2019

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@blast-hardcheese just tagging you to make sure this doesn't get lost ;)

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

@tomasherman Unnecessary, fyi

As for the bug, I agree with you. This should be unrolled completely likely resulting in PairedDeviceInfo never being emitted.

If you take a look at where allOf is called in guardrail core, you'll find where the values are turned into their intermediate representation before being rendered. At that point, you'll be able to avoid emitting the case class entirely, in favor of a deferred reference. This should cause other attempts to deference the name to find the new alas and user that type instead.

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Thanks for your reply. I'm not sure what you mean by deferred reference.

The way i see it, i think the best case for allOf objects is to simply take all the field(s) defined in parent(s) and merge them together (with literals that can into a new case class with out extension. It might be quite easy to hit the 22 params case class limit but i guess that's another issue.

Is that by any chance what you had in mind?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

I'm talking more about creating one of these in the special case of only one element in an allOf, we can collapse the definition entirely

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

How do you feel this code should work:

{
  "swagger" : "2.0",
  "info" : {
    "title" : "someapp",
    "description" : "someapp",
    "version" : "1"
  },
  "basePath" : "/v1",
  "schemes" : [ "http", "https" ],
  "produces" : [ "application/json" ],
  "paths": {},
  "definitions": {
    "PairedDeviceInfo" : {
      "description" : "Data about a device's pairing.",
      "allOf" : [ {
        "$ref" : "#/definitions/PairedDeviceFields"
      },
        {
          "type": "object",
          "properties": 
          {
            "himom": {
              "type": "string"
            }
          }
        }
      ]
    },
    "PairedDeviceFields" : {
      "description" : "Fields describing the pairing of a managed / paired device.",
      "type" : "object",
      "properties" : {
        "status" : {
          "type": "integer"
        }
      },
      "required" : [ "status", "deviceState" ]
    }
  }
}

currently:

case class PairedDeviceInfo(status: BigInt, himom: Option[String] = None) extends PairedDeviceFields
object PairedDeviceInfo {
  implicit val encodePairedDeviceInfo = {
    val readOnlyKeys = Set[String]()
    Encoder.forProduct2("status", "himom") { (o: PairedDeviceInfo) => (o.status, o.himom) }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
  }
  implicit val decodePairedDeviceInfo = Decoder.forProduct2("status", "himom")(PairedDeviceInfo.apply _)
}

case class PairedDeviceFields(status: BigInt)
object PairedDeviceFields {
  implicit val encodePairedDeviceFields = {
    val readOnlyKeys = Set[String]()
    Encoder.forProduct1("status") { (o: PairedDeviceFields) => o.status }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
  }
  implicit val decodePairedDeviceFields = Decoder.forProduct1("status")(PairedDeviceFields.apply _)
}
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

I feel the code is good except for the extension of the base class ... do you think just dropping the extension would be good solution? Im not sure what is the reasoning behind constructing of hierarchies so i don't know whether just removing it is a good solution

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2019

Well, a PairedDeviceInfo is a valid PairedDeviceFields in this respect.

While we can break that association, for something where Cat { allOf: [Mammal, Pet] }, a Cat should be able to be treated as either a Mammal or a Pet in the case of getExtendedPetInfo(pet: Pet) or getExtendedMammalInfo(mammal: Mammal)

My motivation for using Deferred is that it would remove superfluous classes, which is almost always better than generating shims and other unpleasantries (we've actually been through this before; Deferred replaced the type aliases and package objects that we initially supported).

The only downside I can see in your original case is that switching from

      "allOf" : [ {
        "$ref" : "#/definitions/PairedDeviceFields"
      } ]

to

      "allOf" : [ {
        "$ref" : "#/definitions/PairedDeviceFields"
      }, {
        "$ref" : "#/definitions/OtherStuff"
      } ]

would end up changing the type names and identifiers. Suddenly, foo: PairedDeviceFields would become PairedDeviceInfo, which would be the union of PairedDeviceFields and OtherStuff (however this ends up working, be it traits or otherwise.

Foregoing case classes entirely in favor of a strategy similar to what https://www.scala-sbt.org/contraband/ uses is another option. Each definition is either a trait or an interface where if a definition becomes tainted by polymorphism, the definition widens to become more broad.

This will slightly break encoders and decoders, but it would permit multiple-inheritance even in Java, at the expense of class literals and requiring constructors and accessors be generated. We'd also lose copy, which would impact consumers more than the guardrail code.

So, to recap (with emoji for voting):

  • 👍 Eliminate intermediate type definition, rolling up allOf: [A] to just A
  • 😀 Copy all members, breaking all type hierarchy for allOf'd types
  • 🎉 Switch to trait/interfaces, providing static convenience function members to compensate for not using case classes
@hanny24

This comment has been minimized.

Copy link

commented Apr 2, 2019

I would prefer to break all type hierarchy. It makes the generated code predictable and quite straight forward.

First solution (Eliminate intermediate type definition) might give confusing results, which IMHO is something we would like to avoid.

As for the trait/interface implementation, I believe it complicates things by quite a lot for a little to almost none benefit (at least from my point of view).

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

i think 🎉solution would be cool but i think it would complicate the code unnecessarily for very little gain so i would be cool with 😄

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@blast-hardcheese do you have any opinions on this? i'm a bit torn on this but i would love to pick an one of these and maybe give it a go, i feel 😄 is the simplest of approaches

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

@tomasherman I'm in favor of attempting to manage the class hierarchy, for the case I mentioned above (a Cat is both a Pet and Mammal).

This copying approach may cause unnecessary copies or structural types to adequately genericize library code that needs to work with these hierarchies (or possibly shapeless Poly, now that I'm thinking about it).

I suspect starting with copying all members would be the easiest approach, and the full structural reuse (passing Cat to Pet or Mammal) would come from expressing these classes as shapeless records, which would also solve #45.

I'm ok proceeding with 😄

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

Whoops, accidentally closed on the mobile interface.

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

ok, i will give it a try ... im not sure i will get anywhere because the code is far from trivial, but i will give it tomorrow

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

just to verify, you are saying that it's ok to drop all inheritance hierarchies and cat.as[Pet] functionality, correct?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

I'd expect the following would not work:

def needsPet(pet: Pet) = ???
needsPet(json.as[Pet].get)

I think the only case where this behavior would change is in the case of

allOf:
- $ref: '#/definitions/Pet
- type: object
  schema:
    properties:
      ...

which is regrettable. Ideally, inheritance would be preserved in this case, only dropping in the case of multiple parent classes in an allOf, but this is already getting quite complicated.

An alternate which may be preferable is cat.toPet being automatically generated, but this is just a proposal at this point, I'd like for someone to actually request it before implementing it.

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

So i took a stab at this but didn't get very far...I suspect CirceProtocolGenerator has to be updated but I feel maybe it's more changes than i expect ... it looks like you need to change how encoding/decoding works and perhaps even data model. Is there any chance you guys (as in maintainers) would have time to work on this? Or at least do some preliminary design changes that i can finish implementing? At least i provided test case:

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
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

@tomasherman I would be surprised if your test case here did not work, as #43 was implemented in #82 and #104.

I thought the issue was around allOf with multiple named extension classes, am I incorrect?

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

unfortunately it does not work ... give it a try yourself ... case to case hierarchy is still generated

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

this is the simplest failing case i was able to came up with

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Ah, it seems as though the only difference is no discriminator. I'm onboard now.

I'd be surprised if it's not possible to just work directly in ProtocolGenerator, likely around where all members are resolved.

I can take a look tomorrow. If you have some initial thoughts in a branch somewhere, it might add some color to the conversation.

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

No worries! Even if it's just a few comments or questions, it doesn't need to compile :)

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

ok so i got a little bit more time to work on this ... it looks like you are correct and with discriminator a proper code is generated ... it generates a proper trait which could be useful

i wonder if for the test i proposed above we could do something like this:

request.scala:
case class Request(id: String, state: Int) extends RequestFields
case object Request {
  //  ... encoders and decoders ...
}

reqeuestfields.scala:
trait RequestFields {
  def id: String
}

object RequestFields {

  // encoders and decoders
  def apply(state: Int): RequestFields = RequestFieldsInstance(state)
}
case class RequestFieldsInstance(state: Int) extends RequestFields

This would be generated for all hierarchies without discriminator ... and i think it should work even for multiple inheritance

Would that make sense to you?

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

not sure how you handle objects with more params than case classes can handle though

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

If we introduced this type indirection, folks passing around RequestFields wouldn't be able to use any of the case class methods, so it would leak RequestFieldsInstance throughout the codebase. I don't think this would be a very good experience.

The reason the trait works well for the discriminator case is because it doesn't make sense to have a Pet without a valid petType, which naturally leads to using one of the implementation classes.

Without a discriminator, I still think the direct copy approach is best, which gives us the option to .asRequestFields if the user needs it. They can also use structural types of they're ok with the performance hit, or shapeless Poly as I mentioned before (this case may be beneficial to add documentation for)

As for more than 22 parameters, #45 has been open for some time, but it hasn't been an issue for us yet. Switching from case classes to shapeless Records after 22 parameters (or even optionally for everything) would solve this, as well as the inheritance issue we're discussing here, as the type of records can just check field presence instead of exact matches.

@tomasherman tomasherman referenced this issue Apr 12, 2019

Merged

issue #222 #234

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

so i gave it a shot ... i fixed it somewhat, altho i have no idea whether the solution is good...scala now compiles fine but java is having trouble ... i basically decided that we shouldn't extend a class unless discriminator is present

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

any ideas whether this is a right approach?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

Haven't seen it over the weekend, I'll take a look when I'm at a computer.

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

hey, any chance you had some time to look at this?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Taking a look now, sorry for not following up. The code change looks fine, though the implications are something I'd like to validate before committing to them.

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Question left in the PR

@blast-hardcheese blast-hardcheese referenced this issue May 2, 2019

Closed

Add security support for Dropwizard #274

0 of 1 task complete

blast-hardcheese added a commit that referenced this issue 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.