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(*): Support string length validation #647

Merged
merged 8 commits into from
May 22, 2023

Conversation

ragi-dayananda
Copy link
Contributor

@ragi-dayananda ragi-dayananda commented May 17, 2023

Closes #643

  • Update parser logic to respect length metadata
  • Update concerto core with length validation logic
  • Add test cases for all these changes

Changes

Flags

Screenshots or Video

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
  • Validate JSON against model files
  • Compile generate code for a target platform
    • CSharp
    • JSON Schema
  • Parse parse a cto string to a JSON syntax tree
  • Print print a JSON syntax tree to a cto string
  • Compare compare two Concerto model files
  • Infer generate a concerto model from a source schema
  • Generate generate a sample JSON object for a concept

@dselman
Copy link
Contributor

dselman commented May 17, 2023

Looks great so far @ragi-dayananda! Once the introspection class is in place for validation etc. it would be great to update the JSON Schema code generator to use this as an example / test of it being applied.

https://json-schema.org/understanding-json-schema/reference/string.html#length

@dselman
Copy link
Contributor

dselman commented May 17, 2023

@ragi-dayananda ragi-dayananda marked this pull request as ready for review May 18, 2023 09:52
@ragi-dayananda ragi-dayananda requested a review from a team May 18, 2023 09:52
Copy link
Member

@mttrbrts mttrbrts left a comment

Choose a reason for hiding this comment

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

Good progress, Ragi. I've added some minor comments in-line.

Can you update the issue to track the remaining work, please?

For example:

  • validate validate JSON against model files
  • compile generate code for a target platform
    • TypeScript
    • JSON Schema
    • etc...
  • parse parse a cto string to a JSON syntax tree
  • print print a JSON syntax tree to a cto string
  • compare compare two Concerto model files
  • infer generate a concerto model from a source schema
  • generate generate a sample JSON object for a concept

We don't necessarily need complete coverage before we release this work, however, we do need to make sure that we fail gracefully / warn, or hide this work behind an environment variable until it's ready.

packages/concerto-core/lib/introspect/stringvalidator.js Outdated Show resolved Hide resolved
packages/concerto-core/lib/introspect/stringvalidator.js Outdated Show resolved Hide resolved
packages/concerto-core/lib/introspect/stringvalidator.js Outdated Show resolved Hide resolved
packages/concerto-core/test/introspect/stringvalidator.js Outdated Show resolved Hide resolved
packages/concerto-core/test/introspect/stringvalidator.js Outdated Show resolved Hide resolved
packages/concerto-core/test/introspect/stringvalidator.js Outdated Show resolved Hide resolved
packages/concerto-core/test/introspect/stringvalidator.js Outdated Show resolved Hide resolved
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.

Good work.

@mttrbrts
Copy link
Member

@ragi-dayananda There are a few edge cases missing from your tests. Can you bump the coverage, please?
https://coveralls.io/jobs/121606352/source_files/8583783901

@ragi-dayananda
Copy link
Contributor Author

@ragi-dayananda There are a few edge cases missing from your tests. Can you bump the coverage, please? https://coveralls.io/jobs/121606352/source_files/8583783901

Updated the tests cases for mentioned scenarios

jonathan-casey and others added 7 commits May 22, 2023 12:32
* feat(*): add grammar rules for MapDeclaration

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): add MapDeclaration case to Printer

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): add cto & ast test data, for Parse & Print tests

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): type definitions updated

Signed-off-by: jonathan.casey <[email protected]>

* chore(*): package-lock update

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): fix JSDoc

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): remove MapPropertyKeyDeclaration & MapPropertValueDeclaration as top level declaration

Signed-off-by: jonathan.casey <[email protected]>

* test(*): decorators on map top level and props

Signed-off-by: jonathan.casey <[email protected]>

* test(*): negative case for map type - too many properties

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): add MapRelationshipPropertyValue

Signed-off-by: jonathan.casey <[email protected]>

* test(*): add more test cov

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): parser.js update

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): AST  rename

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): printer evaluates AggregateRelationshipValueType

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): update test data

Signed-off-by: jonathan.casey <[email protected]>

* feat(*): cleanup

* feat(*): cleanup

* feat(*): update parser.js

---------

Signed-off-by: jonathan.casey <[email protected]>
Signed-off-by: [email protected] <[email protected]>
…cabulary (accordproject#640)

chore(deps): bump yaml in /packages/concerto-vocabulary

Bumps [yaml](https://github.com/eemeli/yaml) from 2.0.0-9 to 2.2.2.
- [Release notes](https://github.com/eemeli/yaml/releases)
- [Commits](eemeli/yaml@v2.0.0-9...v2.2.2)

---
updated-dependencies:
- dependency-name: yaml
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: [email protected] <[email protected]>
enhance edge cases for string validator

Signed-off-by: [email protected] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants