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

Why not support refs in union mapping? #10

Open
AlexSugak opened this issue Dec 2, 2021 · 5 comments
Open

Why not support refs in union mapping? #10

AlexSugak opened this issue Dec 2, 2021 · 5 comments

Comments

@AlexSugak
Copy link

Hi!

I get it (I think so) why members of the discriminated union's mapping must be of propertiesForm. But why can't it be a refForm pointing to a propertiesForm? E.g. something like this is not allowed, while I think it is fine to have such schema:

{
  "discriminator": "kind",
  "mapping": {
    "a": {
      "properties": {
        "foo": { "type": "string" }
      }
    },
    "b": {
      "ref": "b"
    }
  },
  "definitions": {
    "b": {
      "properties": {
        "baz": { "type": "string" }
      }
    }
  }
}

I understand that changing this would require a new RFC but I was just wondering about the reasoning behind such limitation.

@ucarion
Copy link
Contributor

ucarion commented Dec 2, 2021

It's not just that it has to be of the properties form. It also needs to not re-define the discriminator property. And it's valid for a definition to itself be a ref, so you'd have to have the validator recursively follow the refs and then perform the discriminator check. This makes it harder to implement the spec.

More significantly, refs in properties form don't work well with code generation. At a high level: you can't just "drop-in" ref'd schemas in a mapping, because mapping schemas implicitly have the discriminator property.

For most languages, the natural way to implement a discriminator "mapping" type can't be done independently of the "parent" discriminator. For instance, in Java, the discriminator schema becomes an abstract class, and each mapping is a concrete implementation of that class. Whereas with "normal" properties form schemas, like /definitions/b in your example, you would usually just get a POJO. Which means from the same definition, you'd potentially get N different generated types -- one for the normal "properties" case, and then a separate class for each mapping which refers to the definition. Similar parallels hold for most other target languages -- TypeScript is an exception.

@AlexSugak
Copy link
Author

AlexSugak commented Dec 6, 2021

Thank you!

Not sure I understand the "recursive validation" part though.
Yes, validator needs to check if properties form does not redefine the discriminator property. But allowing a refForm in discriminator mapping would only result in one extra step of getting the referenced propertiesForm from the root "definitions", or am I missing something?

For example, in Java when generating a type for union member (inherited from the base union class) one needs to have the properties form to get all its members. Isn't getting this form from inline definition or from the ref is basically the same thing?

Refs inside the properties form of a union mapping are allowed, so the recursive expansion of types is already there.

To give more details on why not having refs in union mappings could be a problem - for example, what if I want to reference one of the union-member types "inside" another union-member? Something like this:

{
  "discriminator": "kind",
  "mapping": {
    "aBigReusableType": {
      "properties": {
        "a": { "type": "string" },
        "b": { "type": "string" },
        "andManyMore": { "type": "string" }
      }
    },
    "anotherTypeThatWantsToReferenceBigReusableOne": {
      "properties": {
        "nestedBigOne": {"ref": "aBigReusableType????howToDoThis????"}
      }
    }
  }
}

Since there is no way to use refs in union mapping, I cannot "extract" a "aBigReusableType" type to definitions and then reference it inside of another union type.
Oh, now I get it. Allowing that would mean that in for example Java the nested referenced type would need to be the same "inherited from base union" type, not the POJO. Ugh.

Could this be handled using interfaces though? E.g. to have aBigReusableType class both inherit "base union abstract class" and implement a "pojo" interface, and then use interface as a type of nestedBigOne property.

@MrBrN197
Copy link

Hi, @AlexSugak so how did you get around this issue? I don't know Java so I'm not sure I understand why this limitation exists. but I'd still like to know how you got around this.

@AlexSugak
Copy link
Author

Hi @MrBrN197! We worked around this by simplifying the schema a bit and allowing for definitions duplication in few places. Still, it would be nice to have resuable definitions in unions.

@MrBrN197
Copy link

I see. thanks that won't be possible for me.

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

3 participants