modelina icon indicating copy to clipboard operation
modelina copied to clipboard

Allow overriding interpreters

Open artur-ciocanu opened this issue 2 years ago • 4 comments

Reason/Context

Today modelina allows different customizations via preset mechanism. It works great for most of the cases, however there are situations when customizing the code generator output is impossible, because of some of the hard-coded logic that can be found in processors and interpreter. For example, if we want to customize the output of the schema field that have additionalProperties, then we really have no choice, but to accept that any field that looks like this:

{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "title": "Test",
  "type": "object",
  "properties": {
    "prop": {
      "type": "object",
      "additionalProperties": false
    }
  },
  "additionalProperties": false,
  "required": [
    "id"
  ]
}

Will be translated into something like this (e.g. Java code gen):

public class Test {
  private Prop prop;

  public Prop getProp() { return this.prop; }
  public void setProp(Prop prop) { this.prop = prop; }
}

public class Prop {
  
}

Which is quite unfortunate since we can do anything useful with this Java class. I would argue that the above schema declaration should generate something like this:

public class Test {
  private Object prop;

  @JsonProperty("prop")
  public Object getProp() { return this.prop; }
  public void setProp(Object prop) { this.prop = prop; }
}

I would like to mention that if we use a schema like this:

{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "title": "Test",
  "type": "object",
  "properties": {
    "prop": true
  },
  "additionalProperties": false,
  "required": [
    "id"
  ]
}

Or

{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "title": "Test",
  "type": "object",
  "properties": {
    "prop": {
       "title": "Prop"
    }
  },
  "additionalProperties": false,
  "required": [
    "id"
  ]
}

Then we get a property of type Object. As far as I can tell all the 3 schema definitions are equivalent from code generation perspective, however since InterpretAdditionalProperties can't be customized we can't do anything about it.

Description

One possible solution is to allow clients to pass custom interpreters for all the different properties like oneOf, anyOf, additionalItems, additionalProperties, etc. This is somewhat similar to the way we can create custom presets and add them via config.

artur-ciocanu avatar Oct 02 '22 11:10 artur-ciocanu

This issue is related to this discussion: https://github.com/asyncapi/modelina/discussions/870.

artur-ciocanu avatar Oct 02 '22 11:10 artur-ciocanu

Using JSON Schema to interpret/disambiguate JSON Schema validation constructs for effective (and flexible code generator usage is a major focus of the IDL JSON Schema Vocabulary project.

handrews avatar Oct 02 '22 16:10 handrews

One possible solution is to allow clients to pass custom interpreters for all the different properties like oneOf, anyOf, additionalItems, additionalProperties, etc. This is somewhat similar to the way we can create custom presets and add them via config.

I would highly recommend doing this in stages, with one PR for each interpreter function. Makes it easier to introduce tests, the code, and review it 🙂

jonaslagoni avatar Oct 02 '22 23:10 jonaslagoni

One possible solution is to allow clients to pass custom interpreters for all the different properties like oneOf, anyOf, additionalItems, additionalProperties, etc. This is somewhat similar to the way we can create custom presets and add them via config.

I would highly recommend doing this in stages, with one PR for each interpreter function. Makes it easier to introduce tests, the code, and review it 🙂

@jonaslagoni I have just started exploring, how I should approach it. The plan is to prepare all the "infra" like passing processOptions to processors and other minor changes like this. Before I get to the "meat" of the problem. I'll send separate PRs to make it easier for everyone to review and provide feedback.

Stay tuned ... 😄

artur-ciocanu avatar Oct 03 '22 05:10 artur-ciocanu

@artur-ciocanu let me know if you need anything from me 🙂

jonaslagoni avatar Nov 01 '22 11:11 jonaslagoni

@jonaslagoni thanks for checking, I was a little busy I'll try to get back to this and let you know if I am blocked.

artur-ciocanu avatar Nov 08 '22 09:11 artur-ciocanu

@jonaslagoni I have tried the latest next build 1.0.0-next.36 and it seems that the interpreter customization is not required, at least for now.

I will close the issue and if we will ever need this customization I will open another issue.

Sorry that it took me so long to verify everything.

artur-ciocanu avatar Dec 15 '22 20:12 artur-ciocanu

@artur-ciocanu I thought the idea was that you could provide your own extension of the interpretation functionality so you could do something like:

const options = {
  interpreter: {
    interpretAllOf: () => { //Custom functionality for interpreting allOf }
  }
};

Or was this not want you meant? 🙂

jonaslagoni avatar Dec 19 '22 13:12 jonaslagoni

Gonna close this issue @artur-ciocanu as we solved your issue with providing simple interpreter options to enable and disable additionalProperties and additionalItems.

Feel free to re-open if this feature comes to mind again, or if others respond 🙂

jonaslagoni avatar Jan 30 '23 18:01 jonaslagoni