docs: new core data model

[jonaslagoni]

The change I am proposing can be read about here: https://github.com/asyncapi/modelina/blob/2769d6f20bd7f3ad0cd7ac525b00d4c7844e9246/docs/processing.md

The why

These are the issues that are triggering the change, that simply just shows the problems with the current CommonModel structure. We need a more precise structure to solve and prevent these issues from keep popping up:

• Generators automatically assumes class when it should be enum #243
• Typescript: Using oneOf leads to creation and use of an anonymous schema rather than use of a union type #367
• Member names cannot be the same as their enclosing type #388
• Enum generate int values when should be double #390
• Pattern matching numbers cannot be used as properties #449
• Numbers cannot be used as identifiers for properties #452
• Numbers cannot be used as names of classes #456
• Go generates reserved keyword properties #460
• Special cases in enum values fails to generate accurate enum keys for C# generator #503
• Code injection problem - GHSA-4jg2-84c2-pj95

The following issues highlight the need for better UX:

• Community ownership #487 (comment)
• Add enum for asyncapi message $id`s #181
• Improve reuseability and preset developer experience #365

The rewards

• No tight coupling to JSON Schema structure.
• Better UX for preset and generator developers.
• Better UX for users of Modelina, as it will be easier to understand the underlying data model.
• Abstraction between data model types.
• Enable easier representation of other input types, such as ProtoBuf, XML, JSON Type Definition, and many many more.

The code changes

So with these changes, how does it simplify the generators and presets?

For the examples below, I only focus on TypeScript but keep in mind these changes are relevant for each generator and preset.

Rendering of properties

Old implementation can be found here.

New simplified implementation becomes straight forward because patternProperties and additionalProperties are already resolved directly as ObjectModel properties using the new DictionaryModel.

  async renderProperties(): Promise<string> {
    const properties = this.model.properties || {};
    const content: string[] = [];

    for (const [propertyName, property] of Object.entries(properties)) {
      const rendererProperty = await this.runPropertyPreset(propertyName, property);
      content.push(rendererProperty);
    }
    return this.renderBlock(content);
  }

Serialization preset

Old implementation can be found here.

New simplified implementation for de/serialization functionality make use of the DictionaryModel and serializationType: 'unwrap' | 'normal' to support this behavior:

function renderMarshalProperty(modelInstanceVariable: string, model: ConstrainedMetaModel) {
  if (model instanceof ConstrainedReferencedModel) {
    const resolvedModel = model.referencedModel;
    if (resolvedModel instanceof ConstrainedObjectModel) {
      return `$\{${modelInstanceVariable}.marshal()}`;
    }
  }
  return realizePropertyFactory(modelInstanceVariable);
}
function renderMarshalProperties(model: ObjectModel, renderer: TypeScriptRenderer) {
  const properties = model.properties || {};
  const propertyKeys = [...Object.entries(properties)];
  const marshalProperties = propertyKeys.map(([prop, propModel]) => {
    const formattedPropertyName = renderer.nameProperty(prop, propModel);
    if (propModel.propertyModel instanceof ConstrainedDictionaryModel && propModel.propertyModel.serializationType === 'unwrap') {
      const modelInstanceVariable = 'value';
      const patternPropertyMarshalCode = renderMarshalProperty(modelInstanceVariable, propModel.propertyModel.keyType);
      const marshalCode = `json += \`"$\{key}": ${patternPropertyMarshalCode},\`;`;
      return `if(this.${prop} !== undefined) { 
  for (const [key, value] of this.${prop}.entries()) {
    //Only unwrap dictionary entries which are not already part of the properties
    if(Object.keys(this).includes(String(key))) continue;
    ${marshalCode}
  }
}`;
    } 
    const modelInstanceVariable = `this.${formattedPropertyName}`;
    const propMarshalCode = renderMarshalProperty(modelInstanceVariable, propModel.propertyModel);
    const marshalCode = `json += \`"${prop}": ${propMarshalCode},\`;`;
    return \`if(${modelInstanceVariable} !== undefined) {
    ${marshalCode} 
  }`;
  });
  return marshalProperties.join('\n');
}

Referenced model's type

Old implementation can be found here.

New implementation makes it easier to access the referencedModel directly through the new ConstrainedReferencedModel.

function renderMarshalProperty(modelInstanceVariable: string, model: ConstrainedMetaModel) {
  if (model instanceof RefModel) {
    const resolvedModel = model.referencedModel;
    if (resolvedModel instanceof ConstrainedObjectModel) {
      return `$\{${modelInstanceVariable}.marshal()}`;
    }
  }
  return realizePropertyFactory(modelInstanceVariable);
}

Figuring out a property's type

Old implementation can be found here.

Because we are dealing with a constrained data model, we can get the type as is:

  renderType(model: ConstrainedMetaModel): string {
    return model.type;
  }

And we will remove this function and use the type as is.

Accessing the model type externally

Because Modelina is expected to work with AsyncAPI generator templates we need a way to access the payload type. Similar to `Figuring out a property's type`.

Before, we had to use a bunch of internal functions to access it, and in some cases it was impossible. Now it is as easy as accessing the type property for the model.

const typescriptGenerator = new TypeScriptGenerator(...);
const generatedModels = await typescriptGenerator.generate(message.payload());
const messagePayloadModel = generatedModels[0];
const messagePayloadType = messagePayloadModel.type;

return `function publish(message: ${messagePayloadType}){ ... }`

Probably need to provide an extra external function to simplify this even more such as:

const typescriptGenerator = new TypeScriptGenerator(...);
const messagePayloadType = typescriptGenerator.getType(message.payload());

return `function publish(message: ${messagePayloadType}){ ... }`

But you get the idea 😄

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[smoya]

Good job @jonaslagoni! I like the approach of having that abstract model so we can support for any kind of input 👍
I dropped some comments. Might keep reviewing later!

[jonaslagoni]

@derberg have you ever encountered a PR that can go around the restriction of one peer-review? It says I can merge, but @magicmatatjahu has not approved it anywhere 😅 Is the settings changed?

[derberg]

@jonaslagoni but it is not against master but some next branch. So yeah, you need to enable branch protection for this branch too

[jonaslagoni]

@jonaslagoni but it is not against master but some next branch. So yeah, you need to enable branch protection for this branch too

Right 🤦 I don't have those permissions though 😛

[jonaslagoni]

I decided to rename DataModel to MetaModel as it's a lot of the same jab jab jab and hard to understand 😅

Hopefully, this makes much more sense 🤞 Please have a look @smoya @voxpelli @magicmatatjahu

[smoya]

Nice job @jonaslagoni 🎉 👏 ! It LGTM, however i'm not an owner 👐

[magicmatatjahu]

One comment to the docs.

I have also another question related to the development of new approach. Do you wanna make it in main branch but in separate folder to have current logic and new one and in some time switch to new or do you prefer new branch (like next)?

[magicmatatjahu]

I understand idea about that but maybe we should call it as Language Meta Model and then call JavaEnumModel etc... I think that it will produce some confusion because in some languages it will be exactly constraint model, because some languages don't support enums/unions etc but some allow more freedom than others. What do you think?

[magicmatatjahu]

Or maybe just Language Model?

[jonaslagoni]

The idea is that all outputs should by some means support all MetaModels in one capacity or another.

For example for unions, if they are not supported like in TS string | number we can always render them as any or create wrapper classes class unionT { stringProp: string, numberProp: string }. It all comes down to the output it's possibilities, as well as what the user defines through options.

The constrained variant only refers to the values within the models, i.e. property name/type, model name and type, enum values, and keys, that ensure they are properly formatted in correct syntax.

So to me language meta model makes less sense I think 🤔

[magicmatatjahu]

and ok this is more generic, so 👌🏼 then, thanks for explanation! However I wonder if we really need to call this Constrained Meta. Maybe if we have a Meta Model which is "basic" then Data Model might be a better name for it, as the "final" version for the language?

[jonaslagoni]

Constrained, refers to the process the meta models go through, i.e. the contrainer (TypeScriptConstrained, etc).

What would you change the naming of the process and model name to, to make it consistant? 🤔

Can you try elaborate a bit more why you think constrain does not encapsulate correctly what it does?

[magicmatatjahu]

The name may be but I am a man who thinks differently. To me constraint means that something is limited but in a validation/parsing way (probably too much JSON Scheme, hah 😄 ) and not in a language constraint way. I'm also not a fan of Java jargon to call it like LanguageConstrainedMetaModel, so ConstrainedMetaModel is ok but I'd prefer without Meta -> ConstrainedModel.

[jonaslagoni]

Haha, okay yea, I see 😄 Is there a way we need to highlight this difference in the docs?

Regarding ConstrainedModel, the "problem" is that the constrained model, is of type meta model as it inherits the class 😅 Think that will create more confusion, even though it is a long name 🤔

What if we keept it for now, and once we are ready to merge from next to master we can evaluate it then if it makes sense or not?

[jonaslagoni]

I have also another question related to the development of new approach. Do you wanna make it in main branch but in separate folder to have current logic and new one and in some time switch to new or do you prefer new branch (like next)?

My initial idea was to use next and iterate with small PR's being merged. Of course that means it will take a few PR's before any release on the next branch will happen, because tests and code will be in a balance between the two. But thats fine by me.

Mostly it will be the same code just with MetaModel and ConstrainedMetaModel instead.

[magicmatatjahu]

My initial idea was to use next and iterate with small PR's being merged. Of course that means it will take a few PR's before any release on the next branch will happen, because tests and code will be in a balance between the two. But thats fine by me.

Mostly it will be the same code just with MetaModel and ConstrainedMetaModel instead.

Ok 👌🏼

[magicmatatjahu]

LGTM!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[asyncapi-bot]

🎉 This PR is included in version 0.39.9-next.1 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀