Validate package subsection in package.json #597
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
…need to discovery. Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
… primary keys for mutations. Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
…s and sinks Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Matthias Broecheler <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
* Add test infra Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
* Update UseCaseTest Signed-off-by: Daniel Henneberger <[email protected]> * Update snaps Signed-off-by: Daniel Henneberger <[email protected]> * Fix casing Signed-off-by: Daniel Henneberger <[email protected]> --------- Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
* Expand profiles Signed-off-by: Daniel Henneberger <[email protected]> * Add jdbc 1.18 Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
* moving, updating and deleting old use case tests Signed-off-by: Matthias Broecheler <[email protected]> * completed test case migration. Need to add test hints and graphql queries Signed-off-by: Matthias Broecheler <[email protected]> * Added test cases and queries Signed-off-by: Matthias Broecheler <[email protected]> * Updating snapshots Signed-off-by: Matthias Broecheler <[email protected]> * Changed test case name Signed-off-by: Matthias Broecheler <[email protected]> * Delete old files Signed-off-by: Matthias Broecheler <[email protected]> * Rebase and update tests Signed-off-by: Daniel Henneberger <[email protected]> * Fix some tests Signed-off-by: Daniel Henneberger <[email protected]> * Updated test cases Signed-off-by: Matthias Broecheler <[email protected]> * Improve snapshot util to show full diff and add line numbers. Update snapshots Signed-off-by: Matthias Broecheler <[email protected]> * Update serializer, small bug fixes Signed-off-by: Daniel Henneberger <[email protected]> * Migrate json to jsonnode Signed-off-by: Daniel Henneberger <[email protected]> * Update snaps Signed-off-by: Daniel Henneberger <[email protected]> * Serialize json with bytes Signed-off-by: Daniel Henneberger <[email protected]> * External test Signed-off-by: Matthias Broecheler <[email protected]> * Readd default test dir Signed-off-by: Daniel Henneberger <[email protected]> * Only include graphql schema if there is a tests folder Signed-off-by: Daniel Henneberger <[email protected]> * Made test delay configurable through env variable Signed-off-by: Matthias Broecheler <[email protected]> * Fixed and extended conference test. Signed-off-by: Matthias Broecheler <[email protected]> * Making clickstream test deterministic. Updating snapshots. Signed-off-by: Matthias Broecheler <[email protected]> * Include function qualifier when cloning sqlcall nodes Signed-off-by: Daniel Henneberger <[email protected]> * Update snaps, remove stack trace on failure Signed-off-by: Daniel Henneberger <[email protected]> * Update snapshot Signed-off-by: Daniel Henneberger <[email protected]> * Add flink 1.17 profile Signed-off-by: Daniel Henneberger <[email protected]> * Don't format numbers, fix profile override Signed-off-by: Daniel Henneberger <[email protected]> * updating test cases Signed-off-by: Matthias Broecheler <[email protected]> * Bump watermark back to 0 Signed-off-by: Daniel Henneberger <[email protected]> * Update snaps Signed-off-by: Daniel Henneberger <[email protected]> --------- Signed-off-by: Matthias Broecheler <[email protected]> Signed-off-by: Daniel Henneberger <[email protected]> Co-authored-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
Signed-off-by: Daniel Henneberger <[email protected]>
* initial flexible schema inference preprocessor Signed-off-by: Matthias Broecheler <[email protected]> * updated snapshots Signed-off-by: Matthias Broecheler <[email protected]> * Make preprocessors resolve through DI Signed-off-by: Daniel Henneberger <[email protected]> * Fixed templating bug in vertx kafka subscription configuration Signed-off-by: Matthias Broecheler <[email protected]> * Adding test case for FlexibleSchemaInferencePreprocessor Signed-off-by: Matthias Broecheler <[email protected]> * Adding test case for FlexibleSchemaInferencePreprocessor and CopyStaticDataPreprocessor Signed-off-by: Matthias Broecheler <[email protected]> * Remove old discovery code. Signed-off-by: Matthias Broecheler <[email protected]> * Filter out empty lines in JsonRecordReader Signed-off-by: Matthias Broecheler <[email protected]> --------- Signed-off-by: Matthias Broecheler <[email protected]> Signed-off-by: Daniel Henneberger <[email protected]> Co-authored-by: Daniel Henneberger <[email protected]>
Implementing support for the repository, including both public and private packages --------- Signed-off-by: Daniel Henneberger <[email protected]>
nandorsoma
commented
May 10, 2024
| }, | ||
| "version": { | ||
| "type": "string", | ||
| "pattern": "^(\\d+\\.\\d+\\.\\d+)$" |
There was a problem hiding this comment.
Do we want to enforce a pattern in versioning or is it enough if it is a string?
mbroecheler
requested changes
May 15, 2024
| } catch (ConfigurationException e) { | ||
| throw localErrors.handle(e); | ||
| } | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| private static boolean isValid(Configuration baseconfig) { |
There was a problem hiding this comment.
SqrlConfigCommons is a general configuration class that we use to read all kinds of configuration files. The logic for validating that a file has a particular schema needs to live in a separate class to not mix concerns.
| } | ||
| }, | ||
| "required": [ "package" ], | ||
| "additionalProperties": false |
There was a problem hiding this comment.
The package.json file is used to configure the datasqrl compiler. It contains lots of other configuration options.
I'm surprised that this does not break the compiler. Are you not getting test failures with this?
There was a problem hiding this comment.
My bad, I did not see the subsequent change. Regardless: Is there test coverage for this?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces validation for config files, with initial focus on validating packages being uploaded to the repository. The ultimate goal is to make it extendable for the validation of the entire Sqrl config, hence its placement within SqrlConfigCommons. Here are some considerations driving the current design:
Individual vs. combined configurations: We can validate either individual configurations or the resultant
CombinedConfiguration. While theCombinedConfigurationis critical for us, providing validation feedback on individual files may be more beneficial to the user. Hence, every file is validated separately.Precedence in configurations: Given that if a section is present across multiple files, one of them takes precedence over the others, we need to consider if this is valid for the "package" section. Should there be only one "package" section? If so, we may require a "strict" combination rule which doesn't allow overrides for certain use-cases. This requires further thoughts and feedback.
Validation point: Validation could be performed either before parsing the file into
Configuration(validating raw user input) or after (validatingConfigurationobject). The latter avoids duplicate file reads from the disk and has been implemented for now.Configuration to JsonObject: The conversion of
ConfigurationtoJsonObjectrequired a custom converter. An alternate approach would utilizeSqrlConfigCommons.toMapand pass the result to theJsonObjectconstructor, necessitating minor modifications to thetoMapmethod. This alternate approach was not chosen to avoid touch-points withgetKeysandgetAllKeys, and to retain code readability despite the minor duplication in function.I look forward to further discussions on these points, particularly on the potential implementation of a "strict" combination rule and the preferred approach for converting
ConfigurationtoJsonObject.