Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Validation to disallow multiple create on related entities backed by same database table #2189

Merged
merged 19 commits into from May 2, 2024
Merged

Validation to disallow multiple create on related entities backed by same database table #2189

merged 19 commits into from May 2, 2024

Conversation

ayush3797
Copy link
Contributor

@ayush3797 ayush3797 commented Apr 26, 2024 •
edited

Why make this change?

DAB doesn't support multiple-create on related entities which are backed by same database table. This PR adds a validation to catch that scenario and throw a meaningful exception to the end user. Issue to track support: #2157

What is this change?

  • Adds a validation during order determination logic in MultipleCreateOrderHelper.GetReferencingEntityName() method to catch the scenario.
  • In the method signature for MultipleCreateOrderHelper.GetReferencingEntityName(), a new parameter isMNRelationship is added with default value of false. The reasoning is explained in the section.

Reason for why MultipleCreateOrderHelper.GetReferencingEntityName() is called even for M:N relationships when we know that we are always going to return an empty string?

  1. For M:N relationships, we assume one of the source/target entity as the referencing entity when we do insertion during query execution because any of the entity can be considered as the referencing/referenced entity. However this does not hold true when the source and target entities are backed by same database tables - in which case we cannot consider any of the entity as the referenced/referencing entity.
  2. The not-supported use case - that of not allowing multiple-create via relationships in which source and target entities are backed by same database tables is because DAB cannot determine a valid order of insertion. Since this concerns order determination, it is better to keep the code inside the MultipleCreateOrderHelper.GetReferencingEntityName() method.
  3. Keeping this in MultipleCreateOrderHelper provides unit test coverage even for the other 2 db types i.e. PgSql,MySql.

How was this tested?

  • Added test MultipleCreateOrderHelperUnitTests.TestExceptionForSelfReferencingRelationships() to test the change for the 3 dbs - MySql/MsSql/PgSql.

Sample Request(s)

  1. Config:

image

Request:

image

@ayush3797 ayush3797 self-assigned this Apr 26, 2024
@ayush3797 ayush3797 added the Multiple mutations Fixes/enhancements related to nested mutations. label Apr 26, 2024
@ayush3797 ayush3797 added this to the 0.12rc2 milestone Apr 26, 2024
@ayush3797
Copy link
Contributor Author

/azp run

@ayush3797 ayush3797 mentioned this pull request Apr 26, 2024
Closed
seantleonard
seantleonard reviewed Apr 26, 2024
View reviewed changes
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.

are tests dependent on #2138

@ayush3797
Copy link
Contributor Author

are tests dependent on #2138

Are not dependent. But will require additional entities in the config and some more changes. Keeping bug-bash in mind, trying to get the logic changes in and test can follow.

@ayush3797
Copy link
Contributor Author

Now I'm onto adding test.

@ayush3797
Copy link
Contributor Author

/azp run

Aniruddh25
Aniruddh25 previously requested changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@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.

Waiting on safe typecast to DatabaseTable

seantleonard
seantleonard requested changes Apr 30, 2024
View reviewed changes
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.

dont need isMNRelationship as a param to getreferencingEntityName otherwise looks good. As Ani mentioned, casting feedback needs addressing

src/Core/Resolvers/MultipleCreateOrderHelper.cs Outdated Show resolved Hide resolved
src/Core/Resolvers/MultipleCreateOrderHelper.cs Show resolved Hide resolved
src/Core/Services/MultipleMutationInputValidator.cs Show resolved Hide resolved
seantleonard
seantleonard approved these changes Apr 30, 2024
View reviewed changes
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.

need not block on my feedback, only Ani's. Go ahead and make those changes and you should be good from my end. Let me know if you think we really need to keep isMNRelationship param.

@seantleonard seantleonard modified the milestones: 0.12rc2, 1.1rc Apr 30, 2024
ayush3797 and others added 5 commits May 1, 2024 14:47
@ayush3797 @Aniruddh25
Update src/Core/Resolvers/MultipleCreateOrderHelper.cs
83f887a 
Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
@ayush3797 @seantleonard
Update src/Core/Resolvers/MultipleCreateOrderHelper.cs
fd1a043 
Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>
@ayush3797 @Aniruddh25
Update src/Service.Tests/Unittests/MultipleCreateUnitTests/MultipleCr…
897fe23 
…eateOrderHelperUnitTests.cs

Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
@ayush3797 @Aniruddh25
Update src/Service.Tests/Unittests/MultipleCreateUnitTests/MultipleCr…
2fc5827 
…eateOrderHelperUnitTests.cs

Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
@ayush3797 @Aniruddh25
Update src/Core/Resolvers/MultipleCreateOrderHelper.cs
9b300d0 
Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
@ayush3797 ayush3797 changed the title Validation to disallow mutations on related entities backed by same database table Validation to disallow multiple mutations on related entities backed by same database table May 1, 2024
@ayush3797 ayush3797 changed the title Validation to disallow multiple mutations on related entities backed by same database table Validation to disallow multiple create on related entities backed by same database table May 1, 2024
@ayush3797 ayush3797 dismissed Aniruddh25’s stale review May 1, 2024 09:49

Addressed review

@ayush3797
Copy link
Contributor Author

/azp run

@ayush3797
Copy link
Contributor Author

/azp run

@ayush3797 ayush3797 enabled auto-merge (squash) May 1, 2024 10:17
@ayush3797
Copy link
Contributor Author

/azp run

@ayush3797
Copy link
Contributor Author

/azp run

@ayush3797 ayush3797 merged commit ec56744 into main May 2, 2024
7 checks passed
@ayush3797 ayush3797 deleted the dev/agarwalayush/prohibitSelfReferencingRelationshipForMutations branch May 2, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seantleonard seantleonard seantleonard approved these changes

@abhishekkumams abhishekkumams abhishekkumams approved these changes

@Aniruddh25 Aniruddh25 Aniruddh25 left review comments

@mbhaskar mbhaskar Awaiting requested review from mbhaskar mbhaskar is a code owner

@Mathos1432 Mathos1432 Awaiting requested review from Mathos1432 Mathos1432 is a code owner

@gledis69 gledis69 Awaiting requested review from gledis69 gledis69 is a code owner

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@tarazou9 tarazou9 Awaiting requested review from tarazou9 tarazou9 is a code owner

@aaronpowell aaronpowell Awaiting requested review from aaronpowell aaronpowell is a code owner

@severussundar severussundar Awaiting requested review from severussundar severussundar is a code owner

@ravishetye ravishetye Awaiting requested review from ravishetye ravishetye is a code owner

@rohkhann rohkhann Awaiting requested review from rohkhann rohkhann is a code owner

@neeraj-sharma2592 neeraj-sharma2592 Awaiting requested review from neeraj-sharma2592 neeraj-sharma2592 is a code owner

@sourabh1007 sourabh1007 Awaiting requested review from sourabh1007 sourabh1007 is a code owner

Assignees

@ayush3797 ayush3797

Labels
Multiple mutations Fixes/enhancements related to nested mutations.
Projects
None yet
Milestone
1.1rc
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ayush3797 @Aniruddh25 @seantleonard @abhishekkumams