Add validation for source and target fields in relationships #2139
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
aaronburtle
requested review from
mbhaskar,
Aniruddh25,
seantleonard,
Mathos1432,
gledis69,
tarazou9,
ayush3797,
abhishekkumams,
aaronpowell,
severussundar,
ravishetye,
rohkhann,
neeraj-sharma2592 and
sourabh1007
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
ayush3797
reviewed
Mar 28, 2024
ayush3797
reviewed
Mar 28, 2024
|
/azp run |
1 similar comment
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
…ship' of https://github.com/Azure/data-api-builder into dev/aaronburtle/ValidateSourceAndTargetFieldsInRelationship
|
/azp run |
seantleonard
approved these changes
Apr 12, 2024
seantleonard
dismissed stale reviews from severussundar and Aniruddh25
based on old commit. refreshed feedback now.
aaronburtle
deleted the
dev/aaronburtle/ValidateSourceAndTargetFieldsInRelationship
branch
ayush3797
added a commit
that referenced
this pull request
Apr 30, 2024
## Why make this change? Closes #2166 Related to #2139 ## What is this change? When we validate the relationship fields in the config, ie: source fields and target fields, we use the backing column names to validate against. Previously we were using aliases if one existed, but instead we will validate only against the actual names of the backing columns. So, for example, if someone had an alias for the column "id", so that it would display as "identifier" when using this alias, we would validate that the relationship defined with that field was defined in the config using the name "id", and would throw a validation error if the config instead used, "identifier." ## How was this tested? Current test suite passing, along with a regression test for DwSql, MsSql, MySql, and PostreSql. ## Sample Request(s) Please see this issue for details on samples to reproduce. #2166 --------- Co-authored-by: Sean Leonard <sean.leonard@microsoft.com> Co-authored-by: Ayush Agarwal <34566234+ayush3797@users.noreply.github.com>
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.
Why make this change?
Closes #1935
Closes #2145
By adding validation for source and target fields and linking source and linking target fields in the
RuntimeConfigValidator.What is this change?
In the
ValidateRelationshipsInConfigmethod within theRuntimeConfigValidator, we add logic to cover the cases from #1935 such that we properly validate the relationships in the config.It should be noted that this behavior aligns directly with the published documentation that describes how to use configs with dab. However, it also changes some behavior of the engine. That is, before this validation existed, it was possible to provide a config that was not valid as per our documentation, but dab would sort out the relationships on its own using foreign key information. For most versions this means merging the unknown values with the foreign key information, and in the most recent version it meant over riding entirely with the foreign key version in most invalid scenarios.
But we want the process of generating and using a config to be as clear as possible, with as few confusing grey areas as we can manage. To do this, we now enforce the validation steps that align with our documentation, and do not allow for badly formed configs to be used. We no longer will try to fix a bad config with the engine but will instead clearly communicate to the user that their config does not pass validation, and why.
There are two main differences in the validation logic that we add for this PR. They differ when we are in a one to many or many to one versus when we are in a many to many scenario.
In either one to many or many to one, there exists a relationship directly between source and target entities. This means that source and target fields need to match. However, in a many to many relationship, the source and target entities are related through a linking object. When that linking object exists in the config we know we are in a many to many relationship and use the separate validation logic. In this case, is is the source and linkingSource fields that must match, as well as the target and linkingTarget fields that must match. See: https://learn.microsoft.com/en-us/azure/data-api-builder/relationships for more information.
We will enforce the same principles for one to many, many to one, and many to many, such that the entire set of required fields must be included in any of those scenarios, but at the
foreign keylevel. This means that in many to many you can have the source and linkingSource fields existing, but null target and linkingTarget fields, so long as the foreign key information provides the relationship information needed on the missing side. So for missing target and linkingTarget fields that would be a foreign key relationship between the target entity and linking object, and if source and linkingSource are missing, then between the source entity and linking object.We also remove, in its entirety,
TestRelationshipForCorrectPairingOfLinkingObjectWithSourceAndTargetbecause the cases that it tests are no longer valid. This test was checking for foreign key relationships when there were missing fields for one of source/target or linkingSource/linkingTarget, which is no longer valid in our config. Related cases which are valid such that both source/linkingSource or both target/linkingTarget are null are included withinTestRelationshipWithLinkingObjectNotHavingRequiredFields. Because of this, we removeTestRelationshipForCorrectPairingOfLinkingObjectWithSourceAndTarget, and will add in additional unit testing for many to many relationships in the follow up PR related to #2144 as needed.How was this tested?
We add to the
ConfigValidationUnitTestsclass all of the cases from the above issue. We assert that the correct kind of exception is thrown, with the correct message, status code, and sub status code.Sample Request(s)
You can manually test this by creating configs that violate the rules outlined in the above issue. Or by stepping through the new unit tests which do the same, but programmatically in the tests.