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

Allow changing options.type when patching options #26

Open
emk opened this issue Jun 3, 2022 · 2 comments
Open

Allow changing options.type when patching options #26

emk opened this issue Jun 3, 2022 · 2 comments

Comments

@emk
Copy link
Contributor

emk commented Jun 3, 2022

Right now, if we have a type with:

members:
  options:
    mutable: true
    schema:
      $interface: "ShapeOptions#SameAsInterface"

...and ShapeOptions is defined using the following (as supported by #18):

components:
  interfaces:
    # A "oneOf" interface can be used to generate a TypeScript type union or a
    # Rust enumeration, including the "Post", etc., variants.
    #
    # This is not a superclass! It's a type union, using `|` in TypeScript to
    # allow more than one type. This affects the design in several surprising
    # ways. Among other things, we might have several union types that include
    # different concrete interface types in their union. For example,
    # `ShapeOptions`, `RoundishShapeOptions`, etc.
    #
    # We can only create a `oneOf` union of multiple interfaces if the
    # individual types _all_ use the same discriminator member (see below).
    ShapeOptions:
      description: "Options for a shape."
      oneOf:
        - $interface: "SquareShapeOptions#SameAsInterface"
        - $interface: "RoundShapeOptions#SameAsInterface"

    SquareShapeOptions:
      # The discriminator is a property of `SquareShapeOptions`, even if this
      # type appears without being part of `ShapeOptions`. This is because it
      # affects how the variants of this type get generated.
      discriminatorMemberName: "type"
      members:
        type:
          required: true
          initializable: true
          schema:
            type: string
            const: square
        height:
          required: true
          mutable: true
          schema:
            type: number
        width:
          required: true
          mutable: true
          schema:
            type: number

    RoundShapeOptions:
      discriminatorMemberName: "type"
      members:
        type:
          required: true
          initializable: true
          schema:
            type: string
            const: round
        radius:
          required: true
          mutable: true
          schema:
            type: number

...then it is impossible to send a Merge Patch changing .options from:

{
  "type": "square",
  "height": 10,
  "round": 20
}

...to:

{
  "type": "round",
  "radius": 15
}

This patch necessary for this conversion looks like:

{
  "type": "round",
  "radius": 15,
  "height": null,
  "width": null,
}

We need to be able to pass other keys as null, so we can remove the original square parameters when PATCHing type.

There are several strategies we can use here:

  1. Allow passing any key with "key": null. We could do this using additionalProperties: { schema: { type: "null" } } }. But we need to check with @blakew about whether this breaks our doc tooling.
  2. Only allow height: null and width: null in RoundShapeOptionsMergePatch. But this would require generating a special version of RoundShapeOptionsMergePatch that knows about SquareShapeOptionsMergePatch. Call it "ShapeOptionsRoundShapeOptionsMergePatch". But since oneOf is a type union, not inheritance, we might need several versions of RoundShapeOptionsMergePatch, one for each type union which includes it. So if we have type RoundedShapeOptions = RoundShapeOptions | EllipticalShapeOptions, then we would need RoundedShapeOptionsRoundShapeOptionsMergePatch and RoundedShapeOptionsEllipticalShapeOptionsMergePatch, and so on. This is one of those classic bad ideas that just keeps getting worse.
  3. Like (2), but we bake RoundShapeOptionsMergePatch and SquareShapeOptionsMergePatch directly into ShapeOptionsMergePatch. This would require a lot of code, but we might be able to make it nice?

@hanakslr and @blakew, I'd love your thoughts here.

@emk emk changed the title Allow changing options.type when patching options Allow changing options.type when patching options Jun 3, 2022
@emk
Copy link
Contributor Author

emk commented Jun 3, 2022

Just as background, this currently works the way it does because we're deliberately mirroring the way this works in TypeScript. The simple version looks like this:

type ShapeOptions = SquareShapeOptions | RoundShapeOptions;

interface SquareShapeOptions {
  type: "square",
  height: number,
  width: number,
}

interface RoundShapeOptions {
  type: "round",
  radius: number,
}

A more complicated example where RoundShapeOptions is used in multiple type unions:

type ShapeOptions = SquareShapeOptions | RoundShapeOptions | EllipticalShapeOptions;

type RoundishShapeOptions = RoundShapeOptions | EllipticalShapeOptions;

interface SquareShapeOptions {
  type: "square",
  height: number,
  width: number,
}

interface RoundShapeOptions {
  type: "round",
  radius: number,
}

interface EllipticalShapeOptions {
  type: "round",
  major: number,
  minor: number,
}

The key insights here are:

  1. These are type unions, not inheritance.
  2. OpenAPI's decision to put discriminator information on ShapeOptions is probably caused by the fact that JSON Schema specifies validation not schemas, and the fact that OpenAPI awkwardly tries to graft schema semantics over JSON Schema and there are tons of corner cases where that doesn't work.

openapi-interfaces basically exists to let us declare TypeScript-style interface types in a higher-level fashion than OpenAPI can support. So in cases where OpenAPI makes things much too hard, our default response is to look at what TypeScript does.

@emk
Copy link
Contributor Author

emk commented Jun 3, 2022

Proposal (1), above, would change our output from:

    RoundShapeOptionsMergePatch:
      type: object
      required:
        - type
      properties:
        radius:
          type: number
        type:
          type: string
          const: round
      additionalProperties: false
    RoundShapeOptionsMergePatch:
      type: object
      required:
        - type
      properties:
        radius:
          type: number
        type:
          type: string
          const: round
      additionalProperties:
        type: "null"

This would allow passing arbitrary other keys, as long as they always contained a null. We'd only generate the special additionalProperties if discriminatorMemberName is present. We could also generate a description field explaining how and when to use the extra null properties in a Merge Patch.

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

No branches or pull requests

1 participant