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: added exception for reserved keywords #100

Merged
merged 5 commits into from
May 7, 2024

Conversation

kailash360
Copy link
Contributor

Closes #48

It adds the feature to throw an exception if the JSON schema contains any reserved keyword. These reserved keywords are categorized into private - which is used internally by Concerto, and assignable - which is used in serialization. Errors are thrown only if a private keyword is encountered.

Changes

  • Added list of reserved keywords and categorized on basis of scope of usage
  • Added error message for the exception
  • Added tests to increase coverage

Flags

N/A

Screenshots or Video

N/A

Related Issues

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
  • Merging to main from fork:branchname

Signed-off-by: Kailash Kejriwal <[email protected]>
@kailash360
Copy link
Contributor Author

@sanketshevkar

This PR will add the feature to throw an exception if reserved keywords are used in a JSON schema.

@@ -614,7 +638,10 @@ class JsonSchemaVisitor {
};

// Handle reserved properties.
if (['$identifier', '$class', '$timestamp'].includes(propertyName)) {
if (privateReservedProperties.includes(propertyName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your contribution @kailash360 :). We already have a utility function for the check.
Please use this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I have replaced them with the ModelUtil methods.

@sanketshevkar
Copy link
Member

@stefanblaginov @mttrbrts can you please review this PR?

Signed-off-by: Kailash Kejriwal <[email protected]>
dselman and others added 2 commits May 7, 2024 15:24
@coveralls
Copy link

coveralls commented May 7, 2024

Coverage Status

coverage: 98.739% (+0.003%) from 98.736%
when pulling ea592e0 on kailash360:res-words
into bc2920f on accordproject:main.

@dselman dselman enabled auto-merge (squash) May 7, 2024 14:30
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.

LGTM

@mttrbrts mttrbrts disabled auto-merge May 7, 2024 15:13
@mttrbrts mttrbrts merged commit 5204904 into accordproject:main May 7, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants