fix!: fixes required properties when if/then/else is used

[kennethaasan]

Description

• fixes required properties when if/then/else is used by making all properties optional if if/then/else is used

[jonaslagoni]

I suggested two alternatives to how to solve this, you missing the recursive part, otherwise the rest looks good 👍

[jonaslagoni]

Yea this is not gonna be enough for cases of nested schemas 🤔 For example the following would not be caught:

{
  "$schema":"http://json-schema.org/draft-07/schema#",
  "properties":{
    "condition":{
      "type":"string"
    },
    "test":{
      "properties":{
        "test2":true
      }
    }
  },
  "if":{
    "properties":{
      "condition":{
        "const":"something"
      }
    }
  },
  "then":{
    "properties":{
      "test":{
        "required":[
          "test2"
        ]
      }
    }
  }
}

I guess there are two ways to solve this, recursively remove all keywords that we don't want applied from sub schemas, or we adapt the interpretAndCombineSchema function to take an option that decides whether the merging model should constrict the model that is being merged to.

What do you think? 🤔

[kennethaasan]

I've been looking at this, and there is no easy way to solve it recursively. It can quickly become a mess. Is that something that can be fixed later?

[jonaslagoni]

I am not a big fan of partial solutions if they can be avoided 😅 I know whenever I do that, I almost never return to the problem, and if I do or someone else stumbles upon it it's just a big headache for all parties...

I had an idea of how it could be solved "easily", check out #1158. It handles your use case as well as the complex recursive models.

Unfortunately, at the moment I do not have time to finish it and polish it with tests, etc... If you like the approach please do take over the solution! Happy to answer questions about the approach as well.

I have a feeling that this "problem" is not only for then/else but for other keywords as well, so this solution works long-term as well.

[kennethaasan]

Appreciate the help! I actually tried something like that, but the tests in were failing in InterpretIfThenElse.spec.ts. But, then I moved the test to TypeScriptGenerator.spec.ts like you did, and it worked. I fixed the test in InterpretIfThenElse.spec.ts by calling the interpreter instead. Let me know what you think.

[jonaslagoni]

Just a few comments 👍

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[jonaslagoni]

LGTM 👍

[jonaslagoni]

/rtm

[asyncapi-bot]

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

The release is available on:

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

Your semantic-release bot 📦🚀