poem icon indicating copy to clipboard operation
poem copied to clipboard

Generate union with disciminator name without making intermediate types

Open banool opened this issue 2 years ago • 12 comments

First, some code:

#[derive(Clone, Debug, PartialEq, Union)]
#[oai(discriminator_name = "type", one_of)]
pub enum WriteSetChange {
    DeleteModule(DeleteModule),
    DeleteResource(DeleteResource),
}

#[derive(Clone, Debug, PartialEq, Object)]
pub struct DeleteModule {
    pub address: Address,
    pub state_key_hash: String,
    pub module: MoveModuleId,
}

#[derive(Clone, Debug, PartialEq, Object)]
pub struct DeleteResource {
    pub address: Address,
    pub state_key_hash: String,
    pub resource: MoveStructTag,
}

If you generate a spec from this, you get:

    WriteSetChange:
      type: object
      oneOf:
        - $ref: "#/components/schemas/WriteSetChange[DeleteModule]"
        - $ref: "#/components/schemas/WriteSetChange[DeleteResource]"
      discriminator:
        propertyName: type
        mapping:
          DeleteModule: "#/components/schemas/WriteSetChange[DeleteModule]"
          DeleteResource: "#/components/schemas/WriteSetChange[DeleteResource]"
    "WriteSetChange[DeleteModule]":
      allOf:
        - type: object
          required:
            - type
          properties:
            type:
              type: string
              example: DeleteModule
        - $ref: "#/components/schemas/DeleteModule"
    "WriteSetChange[DeleteResource]":
      allOf:
        - type: object
          required:
            - type
          properties:
            type:
              type: string
              example: DeleteResource
        - $ref: "#/components/schemas/DeleteResource"
     DeleteModule:
      type: object
      required:
        - address
        - state_key_hash
        - module
      properties:
        address:
          type: string
          format: Address
        state_key_hash:
          type: string
        module:
          $ref: "#/components/schemas/MoveModuleId"
    DeleteResource:
      type: object
      required:
        - address
        - state_key_hash
        - resource
      properties:
        address:
          type: string
          format: Address
        state_key_hash:
          type: string
        resource:
          $ref: "#/components/schemas/MoveStructTag"

As you can see, this generates an intermediate type that holds both the "data", e.g. DeleteResource, and the type (a string).

What I really want is something like this:

    WriteSetChange:
      type: object
      oneOf:
        - $ref: "#/components/schemas/DeleteModule"
        - $ref: "#/components/schemas/DeleteResource"
     DeleteModule:
      type: object
      required:
        - address
        - state_key_hash
        - module
      properties:
        address:
          type: string
          format: Address
        state_key_hash:
          type: string
        module:
          $ref: "#/components/schemas/MoveModuleId"
    DeleteResource:
      type: object
      required:
        - address
        - state_key_hash
        - resource
      properties:
        address:
          type: string
          format: Address
        state_key_hash:
          type: string
        resource:
          $ref: "#/components/schemas/MoveStructTag"

As in, no intermediate types. This second output is actually generated from the schema when I don't specify discriminator_type, just one_of. The problem now is there is no discriminator type, since what I really want is this:

    WriteSetChange:
      type: object
      oneOf:
        - $ref: "#/components/schemas/DeleteModule"
        - $ref: "#/components/schemas/DeleteResource"
      discriminator:
        propertyName: type
     DeleteModule:
      type: object
      required:
        - address
        - state_key_hash
        - module
      properties:
        address:
          type: string
          format: Address
        state_key_hash:
          type: string
        module:
          $ref: "#/components/schemas/MoveModuleId"
    DeleteResource:
      type: object
      required:
        - address
        - state_key_hash
        - resource
      properties:
        address:
          type: string
          format: Address
        state_key_hash:
          type: string
        resource:
          $ref: "#/components/schemas/MoveStructTag"

This is necessary so clients know where to find the type of the data. I don't need the intermediate types, it makes the spec more verbose than necessary. The mapping is nice but not required in either case.

Is there any way to do this with Poem today?

banool avatar Jul 22 '22 03:07 banool

I see why it does what it does today, because without the intermediate types you're silently returning data with the type field even though the spec doesn't indicate it should. But if you don't mention discriminator_type explicitly it does that anyway.

banool avatar Jul 22 '22 03:07 banool

I understand that each of your types contains a type field, so you don't need the Union macro to generate intermediate types for you.

sunli829 avatar Jul 22 '22 05:07 sunli829

I will add a no_intermediate attribute to Union, but you need to make sure that each type contains the fields specified by the discriminator and that they are the same type, otherwise will panic when generating the schema.

sunli829 avatar Jul 22 '22 05:07 sunli829

Is this what you want?

#[derive(Clone, Debug, PartialEq, Union)]
#[oai(discriminator_name = "type", no_intermediate)]
pub enum WriteSetChange {
    DeleteModule(DeleteModule),
    DeleteResource(DeleteResource),
}

#[derive(Clone, Debug, PartialEq, Object)]
pub struct DeleteModule {
    pub type: String, // <<<<<   type
    pub address: Address,
    pub state_key_hash: String,
    pub module: MoveModuleId,
}

#[derive(Clone, Debug, PartialEq, Object)]
pub struct DeleteResource {
    pub type: String, // <<<<<   type
    pub address: Address,
    pub state_key_hash: String,
    pub resource: MoveStructTag,
}

sunli829 avatar Jul 22 '22 05:07 sunli829

Almost, the way poem works now (unless I'm mistaken) it returns data with the type field set already, so I don't expect that the struct should require the type field.

In other words, I want what you've got above but there is no type field added to each of the enum variant structs.

banool avatar Jul 22 '22 17:07 banool

Maybe you are wrong. 🙂

https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

In our example, the discriminator points to the objectType property that contains the data type name. The discriminator is used with anyOf or oneOf keywords only. It is important that all the models mentioned below anyOf or oneOf contain the property that the discriminator specifies. This means, for example, that in our code above, both simpleObject and complexObject must have the objectType property. This property is required in these schemas:

sunli829 avatar Jul 23 '22 00:07 sunli829

Hmmm I see, let me investigate further.

banool avatar Jul 23 '22 01:07 banool

Okay I looked and it indeed doesn't return the type field by default. Sooooooooo, this is probably a big ask but I think it'd be awesome if you could have code like this:

#[derive(Clone, Debug, PartialEq, Union)]
#[oai(discriminator_name = "type", no_intermediate)]
pub enum WriteSetChange {
    DeleteModule(DeleteModule),
    DeleteResource(DeleteResource),
}

#[derive(Clone, Debug, PartialEq, Object)]
pub struct DeleteModule {
    pub address: Address,
    pub state_key_hash: String,
    pub module: MoveModuleId,
}

#[derive(Clone, Debug, PartialEq, Object)]
pub struct DeleteResource {
    pub address: Address,
    pub state_key_hash: String,
    pub resource: MoveStructTag,
}

and then Poem would do some macro / derive magic to automatically add the type field based on the name of the value in the enum, without generating intermediate types in the spec. Or perhaps you add a derive to the values in the enum (e.g. above DeleteModule) that does this.

What do you think about this, is this feasible?

banool avatar Jul 25 '22 16:07 banool

To add further detail, I'm migrating from a regular webserver that returns JSON generated by serde, to Poem. With serde this is very easy, you can just add this:

#[serde(tag = "type")] 

I feel like Poem could leverage this model to make this just as easy. Perhaps the Poem version of this could mandate that the type in question impls Serialize and Deserialize.

banool avatar Jul 25 '22 18:07 banool

Okay thinking more about this, I figure this combination of things will work:

  1. Add no_intermediate, which assumes the enum variants all have the field specified in discriminator_name.
  2. Use #[serde(tag = "type)] so (de)serialization adds the type field automatically.
  3. Ensure we're using serde to do (de)serialization, so the field gets added.

As it is today, 3 doesn't seem to work, adding the tag in 2 doesn't result in the type field being included in the response. Actually if it did, I don't think I'd even need no_intermediate.

I suppose the hard part is #[serde(tag = "type)] just adds the field at runtime, so it's hard for to guarantee that the field will be there when compiling.

Just thinking out loud here. I'll look through the code for a solution, this is getting flack on my side because it makes our spec too verbose. It's pretty complicated in there (the union derive) though 😅

banool avatar Jul 28 '22 17:07 banool

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] avatar Aug 28 '22 03:08 github-actions[bot]

I say not stale hahah, but I can't remove the label

banool avatar Aug 28 '22 06:08 banool

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] avatar Sep 28 '22 03:09 github-actions[bot]

This issue was closed because it has been stalled for 5 days with no activity.

github-actions[bot] avatar Oct 04 '22 03:10 github-actions[bot]