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

Json: schema validation support fromJson for jsii imported structs #3943

Closed
hasanaburayyan opened this issue Aug 23, 2023 · 5 comments · Fixed by #4040
Closed

Json: schema validation support fromJson for jsii imported structs #3943

hasanaburayyan opened this issue Aug 23, 2023 · 5 comments · Fixed by #4040
Assignees
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@hasanaburayyan
Copy link
Contributor

hasanaburayyan commented Aug 23, 2023

Currently we can only call fromJson() on user defined structs, this is because we jsify user defined structs into Json schemas. However, we also treat interfaces imported through jsii as struct types, and since these are just imported from the stdlib we do not create jsified schemas.

Not super sure what the best approach to support this would be just yet.

support something like:

bring cloud;

let j = { public: false };
let x = cloud.BucketProps.fromJson(j);

(broke this work out of #3792 since its a separate issue)

@hasanaburayyan hasanaburayyan added 🐛 bug Something isn't working 🛠️ compiler Compiler labels Aug 23, 2023
@monadabot monadabot added this to Wing Aug 23, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New - not properly defined in Wing Aug 23, 2023
@hasanaburayyan
Copy link
Contributor Author

hasanaburayyan commented Aug 23, 2023

@staycoolcall911 I extracted this from 3792 since it orignally covered this and imported structs from other wing files. Not sure if this is as high priority as the work in 3792 (which has a PR up now)

@hasanaburayyan hasanaburayyan changed the title Json: schema validation support fromJson for structs imported from stdlib Json: schema validation support fromJson for jsii imported structs Aug 24, 2023
@staycoolcall911
Copy link
Contributor

Thanks @hasanaburayyan - I think it is high priority. I'll set it as p1, unless someone else has any interesting input on why it could/should be added later. @yoav-steinberg and @Chriscbr might be able to assist you with ideas how to impl

@staycoolcall911 staycoolcall911 moved this from 🆕 New - not properly defined to 🤝 Backlog - handoff to owners in Wing Aug 24, 2023
@Chriscbr
Copy link
Contributor

I can try to suggest an implementation -- though it's just one approach, so feel free to take with a grain of salt. I think the solution can be split into two parts: type checking and jsification.

For type checking, we'd need to make sure fromJson and schema is modeled in the type system as available methods on every json-safe struct (regardless of where it came from). This might already work - I haven't checked.

For jsification, we could try changing how we're emitting struct files into a lazy process, so instead of eagerly emitting a .struct.js file for each Struct statement, we only emit a .struct.js file the first time we encounter a Reference expression that refers to a given struct (or a Call expression to fromJson / schema).

@hasanaburayyan
Copy link
Contributor Author

@Chriscbr thanks for the feedback,

For type checking, we'd need to make sure fromJson and schema is modeled in the type system as available methods on every json-safe struct (regardless of where it came from). This might already work - I haven't checked.

Yes this already works.

As for emitting a struct definition when we encounter a reference expression, let me look into that. It sounds logical that I should just be able to do a lookup on the struct and get everything I need for emitting the schema, though I guess that also means I need to jsify the reference to reference the schema file definition rather than the actual imported reference (maybe).

@mergify mergify bot closed this as completed in #4040 Sep 8, 2023
mergify bot pushed a commit that referenced this issue Sep 8, 2023
## Summary
So this is actually quite a substantial refactor to how we generate jsonschemas for structs. Previously anytime we encountered a user defined wing struct we generated a json schema file with a schema and fromJson methods. However this meant that we were not created schema files for structs defined outside of the wing code (I.E. JSII imported). With the new changes I am taking a lazy creation approach where we only generate schemas for structs if there are references that would require the schema to exist.

This change also no longer generates a separate file for the schema, instead it creates an instance of a stdlib.std.StructSchema using the jsonschema inline.

The other big change is the removal of the require statements and defs from schemas that were dependent on other schemas. As in take for example this Wing struct:
```js
struct A {
  val: num;
}

struct B {
  data: str;
}

struct C extends A {
  b: B;
}
```


The struct schema for `C` used to look like this:
```js
{
        id: "/C",
        type: "object",
        properties: {
          ...require("./A.Struct.js")().jsonSchema().properties,
          b: { "$ref": "#/$defs/B" },
        },
        required: [
          "b",
          ...require("./A.Struct.js")().jsonSchema().required,
        ],
        $defs: {
          "B": { type: "object", "properties": require("./B.Struct.js")().jsonSchema().properties },
          ...require("./A.Struct.js")().jsonSchema().$defs,
        }
      }
```

Now it looks like this:
```js
{
        id: "/C",
        type: "object",
        properties: {
          b: {
            type: "object",
            properties: {
              data: { type: "string" },
            },
            required: [
              "data",
            ]
          },
          val: { type: "number" },
        },
        required: [
          "b",
          "val",
        ]
      }
```

### Implementation Notes
1. Lazy creation of schemas, only emit struct schema files if there exists a reference to the struct type in the code.
2. Schemas no longer consist of require statements for dependent schemas. As in the schemas are now fully self contained.
3. Pre-Jsification parse of the ast to emit struct files for referenced schemas.

Closes: #3943
Closes: #3790

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [x] Docs updated (only required for features)
- [x] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@github-project-automation github-project-automation bot moved this from 🤝 Backlog - handoff to owners to ✅ Done in Wing Sep 8, 2023
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.29.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants