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

[gen] fix for codegen of sumtypes with reusable fields #2850

Conversation

hochgi
Copy link
Contributor

@hochgi hochgi commented May 17, 2024

fixes #2847

I added a test, initially failing (for the payload in the original issue):
sum-type_codegen_test_failure
Then fixed it, and even extended it to generate abstract fields from a reusable component, e.g:

sealed trait Animal {
def age: Int
def weight: Float
}
object Animal {
implicit val codec: Schema[Animal] = DeriveSchema.gen[Animal]
case class Alligator(
age: Int,
weight: Float,
num_teeth: Int,
) extends Animal
object Alligator {
implicit val codec: Schema[Alligator] = DeriveSchema.gen[Alligator]
}
case class Zebra(
age: Int,
weight: Float,
num_stripes: Int,
) extends Animal
object Zebra {
implicit val codec: Schema[Zebra] = DeriveSchema.gen[Zebra]
}
}

This PR also avoids generating duplicate copies of the sealed trait subtypes in separate files.

@hochgi hochgi marked this pull request as ready for review May 17, 2024 16:24
@987Nabil
Copy link
Contributor

@hochgi Thanks for your contribution. And this contains 100% a crucial bugfix.
But, I am not so eager with the change of the shared fields. Why? In OpenAPI you can mark a optional shared field as required for the subtype. This would be a conflict, that is not resolvable on the code generation side. I would like to keep here the more generic solution and keep the super type clean.
I think it is okay for devs to add extension methods if needed or to extend the code manually.
Extension methods will work, even if the code will be generated on each compile/build.

I also remember, that I had a reason for generating the types twice. Once in the sealed trait hierarchy and once separate. But I forgot the reason. I'll think about it and if I can' remember, we might try it with your way.

I like the idea of generating only what is actually used. Even though I think having unused types defined is not good, you can't force anyone (especially not public apis)

@hochgi
Copy link
Contributor Author

hochgi commented May 17, 2024

@hochgi Thanks for your contribution. And this contains 100% a crucial bugfix. But, I am not so eager with the change of the shared fields. Why? In OpenAPI you can mark a optional shared field as required for the subtype...

@987Nabil can you show me how we can define in OpenAPI this example zoo endpoint such that age & weight are enforced in the Animal interface?

I looked at how to do that, and the reusable component was the best I could find.

Of course I'll remove the trait body generation ad requested.
But since I need some mechanism to do this for a real project at work, I'd appreciate a pointer.

If not too hard, perhaps I can chanhe this PR to do it the right way :)

@987Nabil
Copy link
Contributor

@hochgi The best way would probably be a construct like having an animal type, that has three fields: age, weight and animalType. Animal type is then Zebra and Alligator that each have the fields that are unique to them. That should also work with how code is generated and is the kinda FP approach

@hochgi
Copy link
Contributor Author

hochgi commented May 17, 2024

@987Nabil I agree, and if I were to design the API I'm dealing with, that's how I'd do it.
What I'm attempting is a pragmatic solution on how to deal with existing APIs, were all fields are flattened, and I can't nest like with AnimalType containg extra (nested) fields...

EDIT:

Note that in the official docs, a similar approach is described:
https://spec.openapis.org/oas/latest.html#models-with-polymorphism-support
The difference is that in the official docs, Pet is an open super type (analogous to a regular trait or abstract class), whereas here, I'm more intrested with a closed super type (analogous to a sealed trait), which is why I made the super type a separate oneOf component.

Perhaps we can:

  1. Validate no subtype requires an optional field from any super type (and if so, omit that trait from required mixins)
  2. Make (in my example) AnimalSharedFields an open trait. (And optionally, if used also as top level class, add a case class with same fields in the trait's companion)
  3. Make any type that include such shared fields object extend the open trait
  4. Make the closed trait to enforce mixins of all (valid) open traits used by all subtypes.

In this zoo toy example, it could look like:
AnimalSharedFields.scala:

trait AnimalSharedFields {
  def age: Int
  def weight: Float
}
object AnimalSharedFields {
  // "Bare" or any other arbitrary name
  case class Bare (age: Int, weight: Float) extends AnimalSharedFields
}

And Animal.scala:

sealed trait Animal { this: AnimalSharedFields =>
}
object Animal {
  case class Alligator (age: Int, weight: Float, num_teeth: Int) extends Animal with AnimalSharedFields
  case class Zebra (age: Int, weight: Float, num_stripes: Int) extends Animal with AnimalSharedFields
}

@987Nabil
Copy link
Contributor

987Nabil commented May 18, 2024

@hochgi I feel this suggestion is too complex and idk if this will be useful for most users of the code generator.
As discussed in discord, I think a config with a switch would be nice.

I would say the default behaviour is

  • no duplicate types are generated
  • no common fields on the super type

And opt-in to

  • common fields on super type

So basically, your original proposal, but the accessors on the sealed trait are opt in.

I am also not sure, what you are doing actually. Because it seems on the one hand like you can define things in the open api spec, but you said you have to impl. against a given endpoint.

So just some thoughts I had

  • does it make more sense to write endpoints and generate open API specs?
  • if generating from open API, does it need to be perfect? MIght it be enough to have a starting point and adjust the generated code manually?
  • maybe having imperfect data structures generated, even if it is generating repeatedly on build, is good enough and further validation can happen later. For example like generatedEndpoint.copy(input = generatedEndpoint.input.transformOrFail(/* transform to domain or fail -> results in bad request */)) (we can add Endpoint#transformInOrFail if needed)

@hochgi
Copy link
Contributor Author

hochgi commented May 18, 2024

@987Nabil yes, in hindsight it is a bit complex.
Sure, lets go for the configurable opt in as you suggested.

Might be worth just adding validations such as to not require optional fields (if config flag enabled), or if same field is declared with a different type (perhaps in a separate reusable object), etc'...

But for this PR we can start with just the config :)

My motivation:
An existing service (non scala - python in this particular case), which should be used by several other services.
The API & data types exposed look more like the Pet example from the docs.
The openAPI spec is an afterthought, and goal is to serve this in a way that will: (a) conform existing API, and (b) preserve semantic as best is possible.
I will use it also to generate a (cross versioned) scala "SDK" for the service, but also want to enable other teams to reuse it in their non-scala services, using e.g. https://github.com/OpenAPITools/openapi-generator

@hochgi hochgi force-pushed the gen-issue-2847-fix-codegen-sumtypes-with-reusable-fields branch 2 times, most recently from ef0d570 to 9ea7c1d Compare May 23, 2024 11:20
…t actually extend the sealed trait.

      Sum types abstract members generation by reusable components in the schema, as an opt-in, configurable feature.
      Only generating sealed trait body if all subtypes include same reusable component, with it's fields as the abstract trait's members.
      Redundant duplicate classes for each case is now omitted.
      Validate fields in case classes and traits does not contain duplicates that cannot be reconciled.
      Existing tests has been amended to reflect the fix.
      New tests were added.
      Some utilities were added.
@hochgi hochgi force-pushed the gen-issue-2847-fix-codegen-sumtypes-with-reusable-fields branch from 9ea7c1d to b855ad7 Compare May 23, 2024 11:30
)

lazy val config: zio.Config[Config] =
zc.boolean("common-fields-on-super-type")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please no cryptic renaming. Better use the full path zio.Config.boolean

@987Nabil 987Nabil enabled auto-merge (squash) May 26, 2024 15:47
auto-merge was automatically disabled May 26, 2024 16:40

Head branch was pushed to by a user without write access

@987Nabil 987Nabil enabled auto-merge (squash) May 26, 2024 16:48
@987Nabil 987Nabil merged commit 87c3683 into zio:main May 26, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gen] faulty code generation for sum types with reusable fields
2 participants