Skip to content

Adding validation to disallow multiple relationships between same source and target entities #1929

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

Conversation

ayush3797
Copy link
Contributor

@ayush3797 ayush3797 commented Dec 15, 2023

Why make this change?

Currently, we allow defining multiple relationships between the same source and target entities. A user is allowed to define multiple relationships each with different source fields/target fields and even different cardinalities. This is not only logically incorrect and difficult to make sense out of, but will also pose a problem in the functioning of nested mutations.

What is this change?

Added the above explained validation in development mode when GraphQL is enabled for the source entity..

How was this tested?

  • Added unit test via RuntimeConfigValidator.TestMultipleRelationshipsBetweenSourceAndTargetEntities.

@ayush3797 ayush3797 self-assigned this Dec 15, 2023
@ayush3797 ayush3797 added the bug Something isn't working label Dec 15, 2023
@ayush3797 ayush3797 added this to the 0.11rc milestone Dec 15, 2023
@ayush3797 ayush3797 linked an issue Dec 15, 2023 that may be closed by this pull request
@ayush3797 ayush3797 changed the base branch from main to dev/agarwalayush/schemaGeneration December 15, 2023 10:31
@ayush3797 ayush3797 marked this pull request as ready for review December 15, 2023 10:34
@ayush3797 ayush3797 changed the base branch from dev/agarwalayush/schemaGeneration to dev/NestedMutations December 16, 2023 07:03
Copy link
Contributor

@severussundar severussundar left a comment

Choose a reason for hiding this comment

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

I think this particular validation can be added directly to the main branch.

Also, would this validation run as part of dab validate command?

@abhishekkumams
Copy link
Contributor

I think this particular validation can be added directly to the main branch.

Also, would this validation run as part of dab validate command?

It will run both for start and validate command

abhishekkumams
abhishekkumams previously approved these changes Dec 18, 2023
@abhishekkumams abhishekkumams self-requested a review December 18, 2023 08:46
@abhishekkumams abhishekkumams dismissed their stale review December 21, 2023 05:27

need clarification on some questions

Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Looks good, 1 question and a nit

@Azure Azure deleted a comment from azure-pipelines bot Feb 7, 2024
@Azure Azure deleted a comment from abhishekkumams Feb 7, 2024
@Azure Azure deleted a comment from abhishekkumams Feb 7, 2024
@Azure Azure deleted a comment from azure-pipelines bot Feb 7, 2024
@Azure Azure deleted a comment from azure-pipelines bot Feb 7, 2024
@Azure Azure deleted a comment from azure-pipelines bot Feb 7, 2024
@Azure Azure deleted a comment from azure-pipelines bot Feb 7, 2024
@ayush3797
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

thanks for addressing comments.

@seantleonard seantleonard merged commit e850ead into dev/NestedMutations Feb 9, 2024
@seantleonard seantleonard deleted the dev/agarwalayush/validationForDuplicateRelationships branch February 9, 2024 21:49
@ayush3797 ayush3797 restored the dev/agarwalayush/validationForDuplicateRelationships branch February 15, 2024 12:00
@ayush3797
Copy link
Contributor Author

This PR was not to be merged yet. I and @Aniruddh25 had a discussion around this where we decided that throwing an exception at the startup would not be ideal. Rather, we will throw an exception during request execution. As for startup, a warning would suffice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple relationships between same source and target entities
5 participants