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

Add GetSchemaQualifiedHistoryTableName method to SqlServerEntityTypeExtensions #35018

Open
IanKemp opened this issue Oct 30, 2024 · 9 comments · May be fixed by #35368
Open

Add GetSchemaQualifiedHistoryTableName method to SqlServerEntityTypeExtensions #35018

IanKemp opened this issue Oct 30, 2024 · 9 comments · May be fixed by #35368
Labels
area-model-building customer-reported good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone
Backlog

Comments

@IanKemp
Copy link

IanKemp commented Oct 30, 2024 •
edited
Loading

RelationalEntityTypeExtensions has the GetSchemaQualifiedTableName method, a complementing method should be added for temporal tables on SqlServerEntityTypeExtensions:

public static string? GetSchemaQualifiedHistoryTableName(this IReadOnlyEntityType entityType)
{
    var tableName = entityType.GetHistoryTableName();
    if (tableName == null)
    {
        return null;
    }

    var schema = entityType.GetHistoryTableSchema();
    return (string.IsNullOrEmpty(schema) ? string.Empty : schema + ".") + tableName;
}
@maumar maumar added the good first issue This issue should be relatively straightforward to fix. label Oct 31, 2024
@TomLincoln066
Copy link

Hello~ @IanKemp @maumar
I'm quite new here.
If it's ok I'd like to start investigating this issue and see how far I can get.
Thank you!

@TomLincoln066
Copy link

TomLincoln066 commented Nov 1, 2024 •
edited
Loading

I’ve just reviewed the idea of adding GetSchemaQualifiedHistoryTableName to SqlServerEntityTypeExtensions (from Ian) and explored similar methods in the codebase. It seems this would complement GetSchemaQualifiedTableName for temporal tables.
From what I understand, we might need to use (or add) GetHistoryTableName and GetHistoryTableSchema for this to work correctly.
Feel free to correct me if I'm wrong.

However, I wonder do you guys know whether anyone in this repo would double confirm if this is on the right track?
If there are any specific conventions or testing guidelines for temporal tables, I’d appreciate the advice as well.
Thanks!

@AndriySvyryd AndriySvyryd added this to the Backlog milestone Nov 8, 2024
@AndriySvyryd
Copy link
Member

Sounds good. You can add or modify existing tests in https://github.com/dotnet/efcore/blob/main/test/EFCore.SqlServer.FunctionalTests/ModelBuilding/SqlServerModelBuilderTestBase.cs

@roji
Copy link
Member

roji commented Nov 8, 2024

@AndriySvyryd would we want to add such a dedicated API for something that users can easily do themselves? Would we add it to regular tables/views as well (which aren't the history table)?

@AndriySvyryd
Copy link
Member

Would we add it to regular tables/views as well (which aren't the history table)?

We already have them https://github.com/dotnet/efcore/blob/release/8.0/src/EFCore.Relational/Extensions/RelationalEntityTypeExtensions.cs#L221

@newmasterSG
Copy link

@AndriySvyryd , @roji is this issue done? If it is not done, can I take this issue?

@AndriySvyryd
Copy link
Member

@newmasterSG Sure, feel free to send a PR. However, we currently have a large backlog of PRs, so it might take a while for someone to take a look at it.

@rezaghadimim
Copy link

@AndriySvyryd , Can I create a pull request for this issue as my first contribution?

rezaghadimim pushed a commit to rezaghadimim/efcore that referenced this issue Dec 21, 2024
Add GetSchemaQualifiedHistoryTableName method to SqlServerEntityTypeE…
b437ef6 
…xtensions (dotnet#35018)

Fixes dotnet#35018
@rezaghadimim
Copy link

I have written a method for this in SqlServerEntityTypeExtensions and updated the existing tests to include the assertion for it.

@rezaghadimim rezaghadimim linked a pull request Dec 21, 2024 that will close this issue
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-model-building customer-reported good first issue This issue should be relatively straightforward to fix. type-enhancement
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Add GetSchemaQualifiedHistoryTableName method to SqlServerEntityTypeExtensions rezaghadimim/efcore
7 participants
@maumar @IanKemp @roji @AndriySvyryd @rezaghadimim @TomLincoln066 @newmasterSG