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

feat(map): Codegen for JsonSchema Target #63

Merged
merged 19 commits into from
Nov 16, 2023

Conversation

jonathan-casey
Copy link
Member

@jonathan-casey jonathan-casey commented Oct 12, 2023

Description

Add JsonSchema code generation for Map Type. Validated against Schema Draft v7.

Sample <String, String>

{
  "$id": "org.acme.hr.Address.dictionary1",
  "schema": {
    "title": "Map1",
    "description": "An instance of org.acme.hr.Map1",
    "type": "object",
    "propertyNames": {
      "type": "string"
    },
    "additionalProperties": {
      "type": "string"
    }
  }
}

Sample <String, Boolean>

{
  "$id": "org.acme.hr.Address.dictionary1",
  "schema": {
    "title": "Map1",
    "description": "An instance of org.acme.hr.Map1",
    "type": "object",
    "propertyNames": {
      "type": "string"
    },
    "additionalProperties": {
      "type": "boolean"
    }
  }
}

Sample <String, Number>

{
  "$id": "org.acme.hr.Address.dictionary1",
  "schema": {
    "title": "Map1",
    "description": "An instance of org.acme.hr.Map1",
    "type": "object",
    "propertyNames": {
      "type": "string"
    },
    "additionalProperties": {
      "type": "integer"
    }
  }
}

Sample <String, Person>

{
  "$id": "org.acme.hr.Address.dictionary1",
  "schema": {
    "title": "Map1",
    "description": "An instance of org.acme.hr.Map1",
    "type": "object",
    "propertyNames": {
      "type": "string"
    },
    "additionalProperties": {
      "type": "number",
       "$ref": "#/definitions/org.acme.hr.Person"
    }
  }
}

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary

@jonathan-casey jonathan-casey requested a review from a team October 12, 2023 20:56
Signed-off-by: Jonathan Casey <[email protected]>
@github-actions
Copy link
Contributor

Coverage Status

coverage: 98.899% (+0.006%) from 98.893% when pulling 4646741 on jonathan/codegen_map_json_schema into 64de665 on main.

@dselman
Copy link
Contributor

dselman commented Oct 19, 2023

I noticed that in draft 6 of JSON Schema you can now do:

"propertyNames": {
    "pattern": "^[A-Za-z_][A-Za-z0-9_]*$"
  }

And even define different regexes for values, by key!

"patternProperties": {
    "^S_": { "type": "string" },
    "^I_": { "type": "integer" }
  },

We could use that for keys that have a regex validator (in a follow-on PR?).

Which version of JSON Schema are we currently targeting?

@jonathan-casey
Copy link
Member Author

@dselman see PR description, (http://json-schema.org/draft-07/schema)

Regarding regex validation, I'm assuming this would apply to validators on Scalar Types used within a Map? If thats the case then I see no reason why that shouldn't be in scope for this PR.

jonathan-casey and others added 10 commits November 6, 2023 10:44
* feat(map): add graphqlvisitor

Signed-off-by: Jonathan Casey <[email protected]>

* test(map): update snapshot

Signed-off-by: Jonathan Casey <[email protected]>

* test(map): adds graphqlvisitor

Signed-off-by: Jonathan Casey <[email protected]>

* test(map): restores sandbox

Signed-off-by: Jonathan Casey <[email protected]>

---------

Signed-off-by: Jonathan Casey <[email protected]>
feat(codegen): remove loopback src

Signed-off-by: Jonathan Casey <[email protected]>
Bumps [@babel/traverse](https://github.com/babel/babel/tree/HEAD/packages/babel-traverse) from 7.22.11 to 7.23.2.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.23.2/packages/babel-traverse)

---
updated-dependencies:
- dependency-name: "@babel/traverse"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
@mttrbrts mttrbrts added this to the v3.x milestone Nov 8, 2023
Copy link
Contributor

@dselman dselman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex support for strings is a really useful addition but it should be supported for both scalar strings and regular strings.

};

if (mapKey.isScalarDeclaration?.() && mapKey.getValidator()?.getRegex()) {
jsonSchema.schema.propertyNames.pattern = String(mapKey.getValidator().getRegex());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add support for regex validators for scalars we should also support it for string properties.

Copy link
Member Author

@jonathan-casey jonathan-casey Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wasn't part of the original spec unfortunately. Supporting regex on Map elements of type string is a fundamental change at the parser level, and on through validation, etc. We should definitely add this as an enhancement at a later stage.

fyi @mttrbrts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be a misunderstanding here?

We support regexs where they are defined in the Scalar type, we don't support them inline in the Map definition (for either Strings or Scalar types).

Copy link
Contributor

@dselman dselman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo!

@jonathan-casey jonathan-casey merged commit ee9117c into main Nov 16, 2023
11 checks passed
@jonathan-casey jonathan-casey deleted the jonathan/codegen_map_json_schema branch November 16, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants