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.

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

Allow using foreign keys in Dataverse reverse engineering #34689

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MarkMpn
Copy link

@MarkMpn MarkMpn commented Sep 16, 2024

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team (Dataverse reverse engineering - foreign keys #34686)
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

Reverse engineering Dataverse tables ignores primary keys and therefore foreign keys, presumably because Dataverse does not support the COL_NAME function being used in the query to retrieve the index details.

This PR mainly changes that query to use the column name from the sys.columns catalog view instead, which is supported by Dataverse.

It also introduces checks on the primary and foreign keys to ensure it only uses single uniqueidentifier column keys for Dataverse as the metadata can expose various other internal keys of different types which break the code generation, such as a foreign key of a string type referencing a primary key of a uniqueidentifier type. As the developer is not able to make any changes to these internal schemas to correct them, it's necessary to simply ignore them during the code generation instead.

@MarkMpn
Copy link
Author

MarkMpn commented Sep 16, 2024

@dotnet-policy-service agree

@@ -1028,7 +1025,7 @@ private void GetIndexes(DbConnection connection, IReadOnlyList<DatabaseTable> ta
[i].[has_filter],
[i].[filter_definition],
[i].[fill_factor],
COL_NAME([ic].[object_id], [ic].[column_id]) AS [column_name],
[c].[name] AS [column_name],
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder why COL_NAME was used previously??

Copy link
Author

Choose a reason for hiding this comment

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

This was my main concern, I haven't found a situation yet where this produces different results but I assume it must be there for some reason. It was introduced in 746ff59 but I can't see any specific reasoning behind it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! It was there because earlier there was no join with sys.columns - and it was just left there

https://github.com/dotnet/efcore/blob/32434d2d86a75e091e331940384115e447ff33de/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs - so nothing to be concerned about

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 17, 2024

@MarkMpn LGTM - I have verified that the same results appear in AdventureWorks for the new SQL statement with this query:

SELECT
    SCHEMA_NAME([t].[schema_id]) AS [table_schema],
    [t].[name] AS [table_name],
    [i].[name] AS [index_name],
    [i].[type_desc],
    [i].[is_primary_key],
    [i].[is_unique_constraint],
    [i].[is_unique],
    [i].[has_filter],
    [i].[filter_definition],
    [i].[fill_factor],
    COL_NAME([ic].[object_id], [ic].[column_id]) AS [column_name],
    [ic].[is_descending_key],
    [ic].[is_included_column]
FROM [sys].[indexes] AS [i]
JOIN [sys].[tables] AS [t] ON [i].[object_id] = [t].[object_id]
JOIN [sys].[index_columns] AS [ic] ON [i].[object_id] = [ic].[object_id] AND [i].[index_id] = [ic].[index_id]
JOIN [sys].[columns] AS [c] ON [ic].[object_id] = [c].[object_id] AND [ic].[column_id] = [c].[column_id]

EXCEPT

SELECT
    SCHEMA_NAME([t].[schema_id]) AS [table_schema],
    [t].[name] AS [table_name],
    [i].[name] AS [index_name],
    [i].[type_desc],
    [i].[is_primary_key],
    [i].[is_unique_constraint],
    [i].[is_unique],
    [i].[has_filter],
    [i].[filter_definition],
    [i].[fill_factor],
    [c].[name] AS [column_name],
    [ic].[is_descending_key],
    [ic].[is_included_column]
FROM [sys].[indexes] AS [i]
JOIN [sys].[tables] AS [t] ON [i].[object_id] = [t].[object_id]
JOIN [sys].[index_columns] AS [ic] ON [i].[object_id] = [ic].[object_id] AND [i].[index_id] = [ic].[index_id]
JOIN [sys].[columns] AS [c] ON [ic].[object_id] = [c].[object_id] AND [ic].[column_id] = [c].[column_id]

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 17, 2024

@MarkMpn If you are interested, I can try to add this fix to EF Core Power Tools (for EF Core 6 only) and you can verify against a Dataverse?

@MarkMpn
Copy link
Author

MarkMpn commented Sep 17, 2024

@MarkMpn If you are interested, I can try to add this fix to EF Core Power Tools (for EF Core 6 only) and you can verify against a Dataverse?

That would be great, thanks!

bool IsValidPrimaryKey(DatabasePrimaryKey primaryKey)
{
if (_engineEdition != EngineEdition.DynamicsTdsEndpoint)
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

use brackets, not one line return statements

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 17, 2024

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 17, 2024

I implemented a fix for this in the latest daily build, would be grateful if you could try it out.

@MarkMpn
Copy link
Author

MarkMpn commented Sep 17, 2024

Thanks, that seems to work great! Most of the tables now don't have the warning about a missing primary key:

image

There's possibly more Dataverse-specific logic we can do to infer the primary key based on other metadata we have available, but this covers all the main data tables at least.

Generating the code for this doesn't produce any warnings, and the related tables can be queried using:

var results = ctx.Accounts
    .Include(a => a.ContactParentcustomers)
    .Select(a => new { a.Name, Contacts = a.ContactParentcustomers.Select(c => c.Fullname).ToList() });

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 18, 2024

@MarkMpn Wonderful! Would you be interested in this for EF Core 8 and later in Power Tools?

@MarkMpn
Copy link
Author

MarkMpn commented Sep 18, 2024

Would you be interested in this for EF Core 8 and later in Power Tools?

Absolutely! Power Tools is my default way of using EF Core so it would be great to see this merged in there too.

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 18, 2024

Power Tools is my default way of using EF Core so it would be great to see this merged in there too.

Implemented in latest daily

@MarkMpn
Copy link
Author

MarkMpn commented Sep 19, 2024

Implemented in latest daily

Thanks, yes that's working great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants