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

'extends' isn't not a keyword in JSON-schema spec #277

Open
woshimaliang opened this issue Aug 24, 2020 · 2 comments
Open

'extends' isn't not a keyword in JSON-schema spec #277

woshimaliang opened this issue Aug 24, 2020 · 2 comments

Comments

@woshimaliang
Copy link

woshimaliang commented Aug 24, 2020

Currently extends is used for models inheritance in Plank schema.

Pros:

  • Support of inheritance in schema just makes it easier.

Cons:

  • It is really "allOf" other than having a base class.

  • In the generated models, for common methods, it may confuses the complier. I.e,
    `Class A :

    • aStandardMethodWith:(ClassA)obj

    Class B : ClassA

    • aStandardMethodWith:(ClassB)obj
      `
      This pattern is either explicitly not allowed or behaving unexpectedly in most programing languages.
  • Its not a standard keyword thus most existing JSON schema validators don't support that, or we have to make/use Plank's own validator tools on both provider-consumer sides.

  • It may encourages 'polymorphicTypeIdentifier' or similar kind of field in the schema to tell real type of model.

AFAIK, Inheritance isn't supported in most popular model-gen(GraphQL, protoBuf, and etc) tools. I guess the main reasons is that IDL is more for defining constraints on model, not primarily on OOP.

Possible solutions:

  • support allOf then deprecate extends
    This shouldn't lose any property or method from the base class but base class itself.

  • A new build option to treat extends as allOf
    For existing extends users, they may assume base class of the models, this is a breaking change in that case. A possible workaround is to specify a base class in the build option mentioned above? And users of "extend" would take care of the base class and its compliance in their code base.

I created this issue to capture what @jparise and me discussed a while go.

Your thoughts are appreciated.

@rahul-malik
Copy link
Collaborator

I think there are two distinct points here:

  • The extends keyword is not documented in JSON Schema making Plank divergent from the open specification. Should this be changed and should we enforce its aligned with the open specification?

Plank occasionally has taken liberties with the schema syntax to make our migration to immutable models earlier. The extends keyword was added as well as a few others like polymorphicTypeIdentifier to support ADT types. I don't have a strong opinion around switching to allOf but it likely will open it up to more of a mix-in style schema where you could "inherit" from multiple types which can be powerful but also will change how we consider code generation.

Additionally, there are missing attributes of JSON schema outside of allOfand we should figure out if we should attempt to support these as well. Alternatively, we could also use JSON schema to write a schema definition for what Plank accepts which could be another path forward here.

  • How should code generation handle cases like allOf or extends? Should inheritance be supported at all or should we migrate towards composing models?

The ObjC generation is the only one I'm aware of that actually uses inheritance. The Java output from what I remember actually flattens the inherited set of properties and I think we can do the same on the ObjC side. We should consider generating protocols for the specific properties on each type (unflattened) though to allow for APIs to be migrated where they specified a base class type.

@woshimaliang
Copy link
Author

woshimaliang commented Aug 25, 2020

Thanks @rahul-malik! Very good points.

#1, yep, either subset of or full json-schema spec is a good idea;
#2, I don't think we need inheritance in Plank at all. JSON is not primarily designed for that. I like the idea of flattening properties, as that's what allOf is supposed to do: look up properties till root and flatten them.

allOf is slightly different than extends, which will be flatten and is useful for in abstracting common data without a base class or protocol.

Also, I may misunderstood and i don't think polymorphicTypeIdentifier /ADT has to do with extends or allOf. I thought it is a solution to union(doesn't seem to be valid keyword in json-schema) or oneOf. I don't have strong opinion around that and I think they should continue to work after flattened.

Yeah, migration is always the most difficult part. Usually NSObject should be good enough unless their codebase replies on properties of base class like we do (PIModel 😄 ). I like the idea of generating protocol for extended type but:

  1. that might be overhead in code generating if there're many immediate protocols. Unlikely in real word though.
  2. once we support that, we can't get rid of that.

Having said that, this seems to be the best path moving forward. To summarize:

  • Support allOfkeyword
  • Add build option to treat extends as allOf and also generate protocol for extended type
  • Deprecate methods in generated protocols to expedite migration
  • Remove 'extends` support from Plank, also the build option.

I've not looked into source code or java implementation yet. I am more than happy to spend some time on this. Very interesting stuff!

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

2 participants