Skip to content

Migrations: Preserve entity type display name in snapshot - #3810

Closed
bricelam wants to merge 1 commit into
dotnet:devfrom
bricelam:generic2
Closed

Migrations: Preserve entity type display name in snapshot#3810
bricelam wants to merge 1 commit into
dotnet:devfrom
bricelam:generic2

Conversation

@bricelam

Copy link
Copy Markdown
Contributor

This also removes any semantics about a shadow entity names.

Fixes #3545

@smitpatel

This also removes any semantics about a shadow entity's name.

Fixes #3545

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

worth putting a comment for this?

@smitpatel

Copy link
Copy Markdown
Contributor

:shipit:

@bricelam

Copy link
Copy Markdown
Contributor Author

Hmm, I hate that this adds an annotation to virtually every entity type in the snapshot...

Any better ideas @ajcvickers @AndriySvyryd @anpete @divega @rowanmiller?

The root of the problem is that the default table name (and thus the key, index, and foreign key names) are based off EntityType.DisplayName() which in turn is based off ClrType (when available) or Name. For "unbound" types, this means that we need to convert Names like My.Ugly+Type<System.Guid, My.Ugly+Struct<int>> into Type<Guid, Struct<int>> which requires string parsing and adds unnecessary semantics to EntityType.Name.

This PR takes away the semantics and captures DisplayName in the snapshot for "unbound" entity types. This removes the need to parse, but adds extra stuff to the snapshot.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this.DisplayName() ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Which is the lesser of two evils..?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 for this

@anpete

anpete commented Nov 20, 2015

Copy link
Copy Markdown
Contributor

I'm not sure I fully understand this, can we discuss in person?

@ajcvickers

Copy link
Copy Markdown
Contributor

@bricelam This is a consequence of having shadow state entity types in the snapshot, right? In other words, the model used to create the snapshot has real entity types, but the snapshot model loads these as shadow types, which then causes a difference in behavior with regards to names.

@bricelam

Copy link
Copy Markdown
Contributor Author

@ajcvickers Exactly. By converting CLR-backed entity types to shadow entities, we loose some information used by DisplayName.

@anpete

anpete commented Nov 20, 2015

Copy link
Copy Markdown
Contributor

Could we write the table name annotation instead?

@bricelam

Copy link
Copy Markdown
Contributor Author

@anpete Yes, but we'd also have to update constraint names to be based on that (instead of entity type name) which means when you rename a table, all the constraints touching it would also be renamed resulting in a larger diff than expected.

@ajcvickers

Copy link
Copy Markdown
Contributor

@bricelam Could we annotate with the display name only for generic types?

@anpete

anpete commented Nov 20, 2015

Copy link
Copy Markdown
Contributor

And possibly we should call the annotation something else

@bricelam

Copy link
Copy Markdown
Contributor Author

@ajcvickers The existing code has a few other quarks that I don't like. For example, if I name a shadow entity Post+Contributor, the default table name would be Contributor because we apply CLR type semantics to the name.

@anpete

anpete commented Nov 20, 2015

Copy link
Copy Markdown
Contributor

@bricelam Could we have a simpler way of producing a table name from a type name string? I.e. not use DisplayName?

@bricelam

Copy link
Copy Markdown
Contributor Author

This PR simplifies it to...

  1. Use ClrType.DisplayName() (a pretty version of Type.Name)
  2. If no ClrType use DisplayName annotation
  3. If no annotation, use Name verbatim.

@bricelam

Copy link
Copy Markdown
Contributor Author

We could change EntityType.Name... E.g. To trip out generics, but this would prevent you from adding the entity types My.Thing<int> and My.Thing<string> to the same model.

@bricelam

Copy link
Copy Markdown
Contributor Author

We could also strip out the namespaces of the generic type arguments which would simplify the conversion:
Name: My.Thing<Guid> (instead of My.Thing<System.Guid> like it currently is)
TableName: Thing<Guid> (same as it currently is)

...but this still applies semantics to Name.

@bricelam

Copy link
Copy Markdown
Contributor Author

Recalling. We decided to serialize TableName instead of an annotation and change the constraint names to be based on table name instead of entity type name.

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.

6 participants