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

RevEng: Scaffold Many-To-Many join table #25544

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Conversation

smitpatel
Copy link
Contributor

No description provided.

@smitpatel smitpatel requested a review from a team August 16, 2021 21:31
@@ -864,5 +1138,31 @@ private void GenerateAnnotations(IAnnotatable annotatable, Dictionary<string, IA
lines.AddRange(annotations.Values.Select(
a => $".HasAnnotation({_code.Literal(a.Name)}, {_code.UnknownLiteral(a.Value)})"));
}

internal static bool IsManyToManyJoinEntityType(IEntityType entityType)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no concept of a join/association type in the model? It's just defined by its structure? Kinda makes this code tedious. Should this be an extension method in the core assembly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is implicit join entity type but in this case the entity type is configured explicitly so it is not same as what we create by convention in definition.

Comment on lines +2091 to +2102
.UsingEntity<Dictionary<string, object>>(
""BlogPost"",
l => l.HasOne<Post>().WithMany().HasForeignKey(""BlogsId""),
r => r.HasOne<Blog>().WithMany().HasForeignKey(""PostsId""),
j =>
{
j.HasKey(""BlogsId"", ""PostsId"");

j.ToTable(""BlogPost"");

j.HasIndex(new[] { ""PostsId"" }, ""IX_BlogPost_PostsId"");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems pretty verbose. Isn't all this done by convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fk currently has issue about not accounting for all facets. To err on the side of producing correct code, I just printed it out.

Index is necessary even with data-annotation since no concrete class.

Key doesn't seem to have deterministic ordering in what would be PK. @AndriySvyryd would know. It is not easy to figure convention PK like Id.

Table call may be removed, should we remove it when it has same name as entity type?

I ran generation on EF6 created M2M table,

  • It needs to print FKs because constraint has name.
  • Table goes through pluralizer so doesn't match entity type name.
  • 2 indexes
  • It even generates properties since column names are different than property names.

Copy link
Member

Choose a reason for hiding this comment

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

The PK is based on the FK order, which is based on the property names. I don't mind the redundant configuration for PK, but we could avoid the ToTable call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filed #25547

Specifically want to play more with the feature to see different patterns and facets. Perhaps @ajcvickers' new feature testing thing. Outside of our limited testing, I only tested with EF6 generated schema.

@smitpatel smitpatel force-pushed the smit/scaffoldingM2M branch from c8fe604 to 7f18d5d Compare August 17, 2021 05:30
@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 17, 2021

This is a fix for #22475

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.

4 participants