-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add validation for source and target fields in relationships #2139
Add validation for source and target fields in relationships #2139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments so far. Still have more of the test file to review. RuntimeConfigValidator.cs
looks good though!
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
1 similar comment
/azp run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last suggestion on simplification and question about organization of column validity check placement.
/azp run |
/azp run |
/azp run |
…ship' of https://github.com/Azure/data-api-builder into dev/aaronburtle/ValidateSourceAndTargetFieldsInRelationship
/azp run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for pushing through the iterations and feedback!
based on old commit. refreshed feedback now.
## 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>
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
ValidateRelationshipsInConfig
method 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 key
level. 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,
TestRelationshipForCorrectPairingOfLinkingObjectWithSourceAndTarget
because 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
ConfigValidationUnitTests
class 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.