Skip to content

Conversation

ayush3797
Copy link
Contributor

@ayush3797 ayush3797 commented Feb 19, 2024

Why make this change?

If there is a relationship defined for a (source,target) in the database with right cardinality as ONE, we add the relationship in both the directions (source->target and target->source) to the source's RelationshipMetadata corresponding to the target entity's entry in RelationshipMetadata.TargetEntityToFkDefinitionMap dictionary.

We do this because the relationship can be 1:1 or N:1. In case of N:1, the first FK (from source->target) would be present. In case of 1:1, either or both of them can be present. But all this is known to us only at a later stage when we collect metadata from the database and actually infer which of the two relationships (or both) is/are valid.

After validating the correct relationship, we don't remove the other relationship which we added earlier. The redundant relationship jadds a redundant JOIN condition (I think this is something which is visible in the sql query generated by DAB highlighted in this issue: #1940).

What is this change?

The functioning of SqlMetadatProvider.FillInferredFkInfo() is changed such that for all the FK definitions present for a target entity in the source entity's definition, the FK definitions for a target entity in the source entity's definition contain:

  1. One entry for relationships defined between source to target in the database (via FK constraint) in the right order of referencing/referenced entity.
  2. Two entries for relationships defined in the config between source to target (which don't exist in the database).

The above is achieved via a call to a newly added method SqlMetadataProvider.GetValidatedFKs() which loops over all the foreign key definitions defined for the target entity in the source entity's definition and adds to the set of validated FK definitions:

  1. All the FK definitions which actually map to a foreign key constraint defined in the database. In such a case, if the source/target fields are also provided in the config, they are given precedence over the FK constraint.
  2. FK definitions for custom relationships defined by the user in the configuration file where no FK constrain t exists between the pair of (source, target) entities.

How was this tested?

  • Added tests to SqlMetadataProviderUnitTests for Pg/Ms/MySql for the cases when an FK constraint exists between the source/target entities.

Manual Testing

  1. Did manual testing to ensure the redundant join condition is not added in the query.
    For a nested query like:
image

where the configuration for Book entity looks like:

image

the query generated before the fix (in main):

SELECT TOP 100 
[table0].[title] AS [title], 
JSON_QUERY ([table1_subq].[data]) AS [publishers] 
FROM [dbo].[books] AS [table0] 
OUTER APPLY (
    SELECT TOP 1
    [table1].[name] AS [name] 
    FROM [dbo].[publishers] AS [table1] 
    WHERE [table0].[publisher_id] = [table1].[id] 
    AND [table1].[id] = [table0].[publisher_id] 
    ORDER BY [table1].[id] ASC 
    FOR JSON PATH, INCLUDE_NULL_VALUES,WITHOUT_ARRAY_WRAPPER
) AS [table1_subq]([data]) 
WHERE 1 = 1 
ORDER BY [table0].[id] ASC 
FOR JSON PATH, INCLUDE_NULL_VALUES

after the fix:

SELECT TOP 100 
[table0].[title] AS [title],
JSON_QUERY ([table1_subq].[data]) AS [publishers] 
FROM [dbo].[books] AS [table0] 
OUTER APPLY (
    SELECT TOP 1 
    [table1].[name] AS [name] 
    FROM [dbo].[publishers] AS [table1] 
    WHERE [table0].[publisher_id] = [table1].[id] 
    ORDER BY [table1].[id] ASC 
    FOR JSON PATH, INCLUDE_NULL_VALUES,WITHOUT_ARRAY_WRAPPER
) AS [table1_subq]([data]) 
WHERE 1 = 1 
ORDER BY [table0].[id] ASC FOR JSON PATH, INCLUDE_NULL_VALUES

<Observe the removal of redundant join condition in the 4th line in the second query>

Similar observations for MySql:

SELECT COALESCE(JSON_ARRAYAGG(JSON_OBJECT(@param3, `subq6`.`title`, @param4, `subq6`.`publishers`)), JSON_ARRAY()) AS `data`
FROM ( SELECT `table0`.`title` AS `title`, `table1_subq`.`data` AS `publishers` FROM `books` AS `table0` LEFT OUTER JOIN LATERAL (SELECT JSON_OBJECT(@param2, `subq5`.`name`) AS `data` 
FROM ( SELECT `table1`.`name` AS `name` FROM `publishers` AS `table1` 
WHERE `table0`.`publisher_id` = `table1`.`id` 
ORDER BY `table1`.`id` ASC LIMIT 1 ) AS `subq5`) AS `table1_subq` ON TRUE WHERE 1 = 1 ORDER BY `table0`.`id` ASC LIMIT 100 ) AS `subq6`

For pgSql:

SELECT COALESCE(jsonb_agg(to_jsonb("subq6")), '[]') AS "data" FROM ( SELECT "table0"."title" AS "title", "table1_subq"."data" AS "publishers" FROM "public"."books" AS "table0" 
LEFT OUTER JOIN LATERAL (SELECT to_jsonb("subq5") AS "data" FROM ( SELECT "table1"."name" AS "name" 
FROM "public"."publishers" AS "table1" 
WHERE "table0"."publisher_id" = "table1"."id" 
ORDER BY "table1"."id" ASC LIMIT 1 ) AS "subq5") AS "table1_subq" 
ON TRUE 
WHERE 1 = 1 ORDER BY "table0"."id" ASC LIMIT 100 ) AS "subq6"
  1. Did manual testing to confirm that the relationship fields provided by user take precedence:
    (Creating issue here to add a test later:Add test to ensure user provided source/target fields take precedence #2104)
    Config:
image

GraphQL query:
image

Generated SQL query:
image

  1. Manually tested that for relationships which are not backed by an FK constraint, we infer both the source/target entities as the referencing entity. Test to be added post the merge of : Multiple-create: Order determination for insertions #2056

@ayush3797 ayush3797 changed the title Removing redunadant FK definition Removing redundant FK definition Feb 19, 2024
@ayush3797 ayush3797 self-assigned this Feb 19, 2024
@ayush3797 ayush3797 added the bug Something isn't working label Feb 19, 2024
@ayush3797 ayush3797 added this to the 0.12rc milestone Feb 19, 2024
@ayush3797 ayush3797 added the mssql pgsql mysql an issue that applies to all relational databases, same as labeling with `mssql` `mysql` and `pgsql` label Feb 19, 2024
@ayush3797 ayush3797 changed the title Removing redundant FK definition Removing wrong FK definition Feb 19, 2024
@ayush3797 ayush3797 marked this pull request as ready for review February 19, 2024 12:08
@ayush3797 ayush3797 requested a review from Aniruddh25 March 13, 2024 22:24
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.

Looks good so far, but the "Scenario 2" case mentioned in code comments has no tests exercising those paths. that would help understand those code segments so we can step through debugger in the code.

Copy link
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

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

This looks good and very thorough, please add an issue to track the addition of integration tests, since while everything looks good, these kinds of cases can be tricky to understand and no doubt in the future having good integration testing will be valuable for preventing regressions.

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.

Thanks for making these changes :)

  1. Added few straight-forward suggestions to incorporate.

  2. Evaluating whether some of the conditionals in BaseSqlQueryStructure.AddJoinPredicatesForRelatedEntity() determine whether the foreign key definition is valid or not can be taken up as a separate issue. Please open an issue to track.

Apart from these, looks good!

@seantleonard seantleonard dismissed Aniruddh25’s stale review April 1, 2024 20:18

recent iterations added changes which look to address resolved feedback.

@seantleonard
Copy link
Contributor

/azp run

@seantleonard seantleonard enabled auto-merge (squash) April 1, 2024 20:29
@seantleonard seantleonard merged commit 780f923 into main Apr 1, 2024
@seantleonard seantleonard deleted the dev/agarwalayush/removingWrongFK branch April 1, 2024 20:39
@ayush3797 ayush3797 added the Multiple mutations Fixes/enhancements related to nested mutations. label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mssql pgsql mysql an issue that applies to all relational databases, same as labeling with `mssql` `mysql` and `pgsql` Multiple mutations Fixes/enhancements related to nested mutations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to update relationships data after inferring info from the database
6 participants