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

Polymorphism & Inheritance #49

Open
gregsdennis opened this issue Jul 6, 2023 · 2 comments
Open

Polymorphism & Inheritance #49

gregsdennis opened this issue Jul 6, 2023 · 2 comments

Comments

@gregsdennis
Copy link
Member

gregsdennis commented Jul 6, 2023

Relevant discussion:

Examples of current use:

Typically, inheritance is modelled something like this:

// base
{
  "$id": "base",
  "type": "object",
  "properties": {
    "foo": { "type": "string" }
  },
  "required": ["foo"]
}

// derived
{
  "$id": "derived-1",
  "$ref": "base",
  "properties": {
    "bar": { "type": "integer" }
  },
  "required": ["bar"]
}

{
  "$id": "derived-2",
  "$ref": "base",
  "properties": {
    "baz": { "type": "boolean" }
  },
  "required": ["bar"]
}

There are no inherent relationships between the data types these schemas describe, and a codegen could simply create three disparate classes, each with all of the required properties. However, that's not the intent behind the schemas. In order for that intent to be more explicitly expressed while still allowing for normal validation to occur, I propose we add an new annotation keyword that a schema should represent a "derived" type.

derived is a boolean that directs codegen tools to consider in-place references that appear either on their own (sibling $ref) or in an allOf as base types.

The above would change to

// base
{
  "$id": "base",
  "type": "object",
  "properties": {
    "foo": { "type": "string" }
  },
  "required": ["foo"]
}

// derived
{
  "$id": "derived-1",
  "$ref": "base",
  "derived": true,
  "properties": {
    "bar": { "type": "integer" }
  },
  "required": ["bar"]
}

{
  "$id": "derived-2",
  "$ref": "base",
  "derived": true,
  "properties": {
    "baz": { "type": "boolean" }
  },
  "required": ["bar"]
}

Without this keyword, or with a value of false, codegen will be directed to create disparate types.

Multiple inheritance

If we had a schema that had multiple references in an allOf, each reference would be a base type.

{
  "$id": "derived-3",
  "allOf": [
    { "$ref": "base" },
    { "$ref": "extension-data" }
  ],
  "derived": true,
  "properties": {
    "baz": { "type": "boolean" }
  },
  "required": ["bar"]
}

for some extension-data schema.

This, however, requires multiple inheritance, which represents a problem for many languages. For example, JavaScript and .Net don't support multiple inheritance, whereas C++ and Python do. The workaround for those that don't may be to render the base types as interfaces (i.e. type definitions with no implementation). A "derived" class can then implement any number of these interfaces. You still get the polymorphism, but you don't get a class hierarchy.

For example, in C#, derived-3 may be generated as

interface IBase
{
    string Foo { get; set; }
}

interface IExtensionData
{
    // ... extension data
}

class Derived3 : IBase, IExtensionData
{
    public string Foo { get; set; }
    // ... extension data
}

Derived3 can be used anywhere an IBase or IExtensionData could be used, and any JSON instance that validates against derived-3 also validates against base and extension-data, so we have polymorphism in that respect.

However if we need to instantiate an IBase, we'd also need to create a class Base that implements IBase, and that Base class would not be polymorphic with Derived3.

It will need to be up to the tool to discern when to create base classes vs base interfaces as required by the generated language.

(This also illustrates why the JSON Schema team has historically recommended that generative logic should only be used as a developer tool, not in production. Generative logic cannot cater to every scenario, and any generated code should be verified before it's used.)

Other subschemas within an allOf

This issue only covers $ref schemas inside allOf. Subschemas could be handled as either as new types to be "inherited" from or merely as additional definition on the current type.

I've opened a separate issue for this since how to handle subschemas ties in with simple objects (#46).

Allowing undeclared properties

Many comments on this topic seem to want to apply additionalProperties to the base. But this is wrong. It would mean that a derived-* is not a base. But in terms of inheritance, a derived-* is a base. Leaving additionalProperties off solves the problems that arise when it's there.

@gregsdennis
Copy link
Member Author

gregsdennis commented Aug 17, 2023

The more I think about it, I'm coming to the conclusion that polymorphism isn't a good approach for code-generated models. I'd like to present an argument that we don't need to support polymorphism at all with this vocabulary.

If we think about the common use cases for code generation (specifically from JSON Schema), it's generally for creating types for data given to us by APIs of some sort, whether from a web source like OpenAPI or from a local source, like a CSV. Ideally, the models we end up making should be no more than DTOs.

Also, best practice for application architecture recommends that we don't use DTOs inside the application logic. Instead we should create domain models for the critical portions of the application to use and map between the two models when we need to interact with the API.

Secondly, polymorphism is commonly used to avoid code duplication, especially when the domain (business logic) requires that the same property on different types mean the same thing. You can extract a base type from the types and define that property once on the base type.

However, given that code generation is typically making DTOs, does it matter if properties are duplicated across multiple types? I can't say that it does.

@rsmckinney
Copy link

See JSON Schema support in the Manifold project for an existing implementation, most relevant to this discussion is Composition with AllOf.

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