Filling gaps in config validation for rest settings for entities#1496
Merged
Filling gaps in config validation for rest settings for entities#1496
Conversation
Removed some tests that weren't testing the CLI but dependencies (such as CommandLineParser or ILogger)
…them simpler and clearer with the new object structure
Moved logic for Entities defaults to RuntimeEntities rather than deserialiser, as that is a more logical place. We always pass that type around, so we can assume the ctor ran, but we aren't always assuming the deserialiser ran (such as what happens with tests)
Moving to a model where we load the config initially from disk and then generate an in-memory provider using an edited version of the config for the tests
Had introduced a regression that meant custom file names weren't supported on the engine directly. Now that can be passed to the RuntimeConfigLoader, otherwise the default filename is used. This means we're more flexible on the name of the file (useful in tests)
Singular/Plural were collapsed one level early, needed to nest them on a Type property
- Removing shared state as it can cause conflicts - Better modeling of a config for the tests, and swapping in mock services - Fixing the lookup for entities with the @model directive
…asn't the expected result
Had forgotten to remove some old code in the deserializer of RuntimeEntities as well
Since there's a new dependency, Snapshooter, which isn't in the central package repo, we need to override NuGet and tell it to look for it elsewhere. This commit will be reverted when Snapshooter.MSTest is on the primary package feed
Aniruddh25
reviewed
Jul 5, 2023
Aniruddh25
reviewed
Jul 5, 2023
2 tasks
seantleonard
reviewed
Jul 6, 2023
seantleonard
approved these changes
Jul 6, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
lgtm. Just one nit from me and Ani's comments remain.
seantleonard
reviewed
Jul 7, 2023
seantleonard
reviewed
Jul 7, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
question about alternative approach to creating record type properties just for lowercase string
Aniruddh25
approved these changes
Jul 10, 2023
Collaborator
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
LGTM, after resolving remaining comments.
…s://github.com/Azure/data-api-builder into dev/agarwalayush/fillingGapsInConfigValidation
seantleonard
approved these changes
Jul 11, 2023
Aniruddh25
dismissed
aaronpowell’s stale review
All the concerns have been resolved and the latest changes are building on top of PR #1402.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Why make this change?
There were a lot of validation failure cases that were not in place for runtime config validation and hence were not getting caught as mentioned in the issue: #1494. This PR addresses the same.
What is this change?
Added validations for the below mentioned cases for REST settings for entity:
path:How was this tested?