feat!: handle const and discriminator

[kennethaasan]

I started on this feature at #958, but had to abandon it because of other priorities.

Description

Properties that use const should be initialized with that const value and not be included in the constructor of a model. This functionality is added for TypeScript and Java.

If discriminator is set in a schema that other schemas are extending, it will create an enum instead of a string const.

• Test Java: https://github.com/kennethaasan/modelina/blob/const/test/generators/java/JavaGenerator.spec.ts#L389-L421
• Test TypeScript: https://github.com/kennethaasan/modelina/blob/const/test/generators/typescript/TypeScriptGenerator.spec.ts#L691-L803 and https://github.com/kennethaasan/modelina/blob/const/test/generators/typescript/TypeScriptGenerator.spec.ts#L911-L990

Interpreters
The interpreters are now reading the discriminator property from schemas and saves it to the interpreterOptions so that it can be used recursively inside properties. In https://github.com/kennethaasan/modelina/blob/const/src/interpreter/InterpretConst.ts#L29-L31, we now only make the const an enum if discriminator is set. By doing that, we allow const to be a string const if the discriminator is not set. In https://github.com/kennethaasan/modelina/blob/const/src/interpreter/InterpretAllOf.ts#L35-L42, we make sure to pass the discriminator to schemas that are extending the schema with discriminator set. The same is done in https://github.com/kennethaasan/modelina/blob/const/src/interpreter/InterpretOneOfWithProperties.ts#L33-L38 and https://github.com/kennethaasan/modelina/blob/const/src/interpreter/InterpretOneOfWithAllOf.ts#L34-L41. In https://github.com/kennethaasan/modelina/blob/const/src/interpreter/InterpretProperties.ts#L34-L40, it will now pass the discriminator to properties if the value of the discriminator is the same as the property name so that we can handle it in interpretConst.

MetaModel
I've added options to the MetaModel in https://github.com/kennethaasan/modelina/blob/const/src/models/MetaModel.ts#L9. This caused a major diff in this PR because the MetaModel was used in a lot of places. By adding an options object, we can easier add things like default later. At first, I tried to add it to the ObjectPropertyModel, but that didn't work because I couldn't read the constValue in the UnionModel.

ConstrainedMetaModel
When I added the options object to the MetaModel, I also had to add it to the ConstrainedMetaModel because it was extending the MetaModel, and we need access to the constValue in renderers.

ConstrainedObjectPropertyModel
In ConstrainedObjectPropertyModel, I added methods related to the constValue which we can use in renderers.

src/generators/java/renderers/ClassRenderer.ts
In the ClassRenderer, I've added a getConstValue method which is used to render const values for Java.

src/generators/typescript/TypeScriptObjectRenderer.ts
In the TypeScriptObjectRenderer, I've added a getConstValue method which is used to render const values for TypeScript.

Related issue(s)
Resolves #946

[netlify]

❌ Deploy Preview for modelina failed.

Name	Link
🔨 Latest commit	a5c2b63
🔍 Latest deploy log	https://app.netlify.com/sites/modelina/deploys/64394b46f85b9b0008831095

[github-advanced-security]

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

[kennethaasan]

TODO:

• add tests for Java
• add migrating guidelines

[jonaslagoni]

@kennethaasan you could split it up and introduce it in the meta model, and interpreter etc first, and then do the generators afterwards.

Just to keep the pr as small as possible 🙂

[kennethaasan]

@jonaslagoni, 🤔 . I can if you prefer, but it's now ready for review.

[jonaslagoni]

It's entirely up to you @kennethaasan 😄 It's just gonna take longer to review 🙂

[kennethaasan]

@jonaslagoni, it does. I can try to split up next time. I added some extra tests. Please have a look whenever you have time :)

[jonaslagoni]

@kennethaasan do you mind updating with the latest next changes, I want to see if I can use the Playground to test your changes, without having to clone -> test 😄

[kennethaasan]

@jonaslagoni done 😄

[jonaslagoni]

Got a couple of things without even going into the code.

Should const always use enums or values?

I.e. should the following:

{
    "type": "object",
    "$id": "LightMeasured",
    "additionalProperties": false,
    "properties": {
        "type": {
            "type": "string",
            "const": "test"
        }
    }
}

Generate

private _reservedType?: ReservedType = ReservedType.TEST
// Or
private _reservedType?: string = "test"

Or should the user choose? 🤔

Weird results

Multiple enum values only render 1

{
    "type": "object",
    "$id": "LightMeasured",
    "additionalProperties": false,
    "properties": {
        "type": {
            "const": "MyMessage",
            "allOf": [
                {
                    "$ref": "#/definitions/MyCommonEnums"
                }
            ]
        },
        "type2": {
            "allOf": [
                {
                    "$ref": "#/definitions/MyCommonEnums"
                },
                {
                    "const": "MyMessage2"
                }
            ]
        }
    },
    "definitions": {
        "MyCommonEnums": {
            "enum": ["MyMessage", "MyMessage2"]
        }
    }
}

I would expect the above to give me a single enum with two values, where type uses the first one, and type2 uses the second one 🤔 There are two bugs there

1. They both should be referring to the same enum instead of two different ones. This is something that is unrelated to your PR, so ignore that.
2. There are a different interpretation between the type and type2 properties, where type2 actually does what I would expect, and type does not. They are mutually exclusive the two representations.

Problem number 2 is related to this PR as far as I can tell 🤔 Especially when it comes to your cloudevent example, where the messages uses DogType and CatType instead of referring to Pet 🤔

oneOf/anyOf splits them out

{
    "type": "object",
    "$id": "LightMeasured",
    "additionalProperties": false,
    "properties": {
        "type": {
            "oneOf": [
                {
                    "$ref": "#/definitions/MyCommonEnums"
                },
                {
                    "const": "MyMessage2"
                }
            ]
        }
    },
    "definitions": {
        "MyCommonEnums": {
            "title": "MyCommonEnums",
            "enum": ["MyMessage", "MyMessage2"]
        }
    }
}

I would expect that only MyCommonEnums would be used and technically the const ignored because they overlap 🤔

[jonaslagoni]

@kennethaasan I have no idea where to even begin here 😄

Do you mind listing the changes you are proposing, so its easy to follow along in the code?

[kennethaasan]

@jonaslagoni, understandable 😄 I tried to make it more clear in the PR description. The change in the MetaModel caused a big diff, because it's used in a lot of tests. I can also walk you through it over zoom if you want. Feel free to dm me on Slack.

[jonaslagoni]

Sorry, it took so long to review... Been a bit swamped lately 🙃

This caused a major diff in this PR because the MetaModel was used in a lot of places

Yea, we might want to use an object argument sometime in the future instead of regular arguments. But yea, down the road.

Yea, after reviewing the code I don't really like you have to remember where information is within the MetaModel, i.e. whether it's under .options or .. But on the other hand it is future proof for adding extra information as you said.

Now we are just mixing object arguments (options) and regular arguments, which should not be the case IMO. But if we get an issue for it I think its fine for now 👍

At first, I tried to add it to the ObjectPropertyModel, but that didn't work because I couldn't read the constValue in the UnionModel.

Ahh okay!

In ConstrainedObjectPropertyModel, I added methods related to the constValue which we can use in renderers.

Is there a reason it's now functions, instead of raw values? 🤔 Because functions were one of the reasons the last big refactor took place, it was a horrible user experience when writing presets. I.e. you had to call a function to get a rendered property name instead of just saying model.propertyName.

I get the renderer's helper functions, but for the models I am a bit unsure of the benefit 🤔

[kennethaasan]

But if we get an issue for it I think its fine for now

Yes, def agree. I'll create an issue for that once this PR is merged.

[kennethaasan]

As discussed on Slack, I've added a constant constrainer which is called in https://github.com/kennethaasan/modelina/blob/const/src/helpers/ConstrainHelpers.ts#L133 and https://github.com/kennethaasan/modelina/blob/const/src/helpers/ConstrainHelpers.ts#L603.

The TS constant constrainer looks like this: https://github.com/kennethaasan/modelina/blob/const/src/generators/typescript/constrainer/ConstantConstrainer.ts

The Java constrainer looks very similar.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[jonaslagoni]

I dont think I can find anything specific about the changes, other than there is too many things going on in this PR 😄 For the future try to keep one PR = one feature or change 😅 It really helps on the reviewing part and the speed it can happen 👍

The only things I see we need to handle after this PR are the following:

• Add related docs for each language for the new constraints (I see we are missing for enumValue): https://github.com/asyncapi/modelina/blob/master/docs/constraints/Java.md
• Add issues for the other generators that they should implement the new constrain logic, and discriminator and adapt serialization presets where applicable, this can be done either now or once we release v2.

How does that sound? I'll help you create the issues if you want 🙂

[kennethaasan]

@jonaslagoni, thanks! Will do. When working on this PR I didn't realize that this PR would end up this big. An experience to learn from.

[asyncapi-bot]

🎉 This PR is included in version 2.0.0-next.7 🎉

The release is available on:

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

Your semantic-release bot 📦🚀