Refactor annotation code generation #21329

roji · 2020-06-18T23:12:29Z

IAnnotationCodeGenerator now exposes an API to generate all fluent API calls, not one-by-one as before. This allows generating multiple annotations as a single fluent API call.
Use this in SQL Server to properly generate a fluent API for identity (increment/seed) and HiLo.
The AnnotationCodeGenerator base class exposes the same one-by-one API as before, so this is totally backwards compatible (unless the interface is implemented directly).
Use IAnnotationCodeGenerator when generating the model snapshot, to have proper fluent API calls instead of raw annotations. This also DRYs lots of logic between CSharpSnapshotGenerator and CSharpDbContextGenerator.
IAnnotationCodeGenerator centralizes ignoring annotations that shouldn't be generated in code (also DRY between CSharpSnapshotGenerator and CSharpDbContextGenerator, Remove extra model annotations from snapshot #15675).
This is also a step towards helping non-C# language support, since it moves generic logic out of C#-specific types to AnnotationCodeGenerator, which isn't.
For now, I've only moved logic from CSharpDbContextGenerator/CSharpSnapshotGenerator to AnnotationCodeGenerator where annotations are translated into a fluent API/data annotation. We could consider also moving other cases - e.g. IsConcurrencyToken which isn't annotation-based - but that means AnnotationCodeGenerator becomes something like a general CodeGenerationHelper.

Notes

AnnotationCodeGenerator currently references CoreAnnotationNames which is internal (warning is suppressed). In Design we weren't enforcing the analyzer warning, but this is in Relational... We should consider making annotation names public.
Annotations with null values were ignored in various places, I centralized this in AnnotationCodeGenerator.RemoveIgnoredAnnotations. I hope that's correct.
Should we always generate fluent API calls with all defaults explicitly, to bake in the values (e.g. pass seed/increment 1 in UseIdentityColumn)? The more we bake in, the better, it seems.
This doesn't currently support scaffolding data annotations on keys/foreign keys/indexes (e.g. a future SQL Server [Clustered] attribute). This shouldn't be too hard, but I propose scoping it out until we decide to actually implement one.
The annotation ignore list is currently global and not per-type (model, entity, property...) - that's the way it already was in MigrationsCodeGenerator. I think it should be fine, just sayin'.

Supercedes #21210

AndriySvyryd · 2020-06-19T00:42:17Z

AnnotationCodeGenerator currently references CoreAnnotationNames which is internal (warning is suppressed). In Design we weren't enforcing the analyzer warning, but this is in Relational... We should consider making annotation names public.

Core annotations are an implementation detail and shouldn't be treated as other annotations, instead the configuration should only be accessed through public API. In the future they won't be returned together with other annotations - #19806. So it might make sense to add them to the ignore list pre-emptively.

It's ok to depend on CoreAnnotationNames.AllNames just for the purpose of filtering them out.

roji · 2020-06-19T08:41:50Z

Core annotations are an implementation detail and shouldn't be treated as other annotations, instead the configuration should only be accessed through public API. In the future they won't be returned together with other annotations - #19806. So it might make sense to add them to the ignore list pre-emptively.

I guess that mean that annotation conventions (e.g. PropertyAnnotationChanged) wouldn't be fired for them, requiring new conventions for facets? Would we also do this for relational annotations (why not)? Am curious what actual problems this extra complexity is meant to solve...

In any case, it's going to be a problem for the current approach in this PR:

This currently gradually filters out annotations as they are generated or ignored. For example, if data annotations are on, CSharpDbContextGenerator simply runs the annotations through the code generator's GenerateDataAnnotation, to get rid of annotations that would be expressed via attributes - what's left is fluent API. If all core annotations are pre-emptively removed, this filtering approach doesn't work.
Similarly, a certain MaxLength may be by-convention for a certain type in a certain provider, and it's very easy to do that right now in the code generator (RemoveConventionalAnnotations). If MaxLength becomes something else that isn't an annotation, this doesn't work.

There's ways around this (pass dataAnnotations and withConventional flags to the generator to control what is generated), but in general there is something very useful in treating configuration data via a single data model (annotations), for consumers that treat need to generally handle configuration.

Anyway, let me know and if this is the way we're going, I'll change this PR accordingly.

src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs

AndriySvyryd · 2020-06-19T18:18:26Z

I guess that mean that annotation conventions (e.g. PropertyAnnotationChanged) wouldn't be fired for them, requiring new conventions for facets?

That's already the case

Would we also do this for relational annotations (why not)?

With #19806 *AnnotationChanged shouldn't be fired for any known annotation, however the relational annotations will always be annotations.

Am curious what actual problems this extra complexity is meant to solve...

The core model might have other implementations (like compiled model) that choose to represent those facets without using annotations for perf.

In any case, it's going to be a problem for the current approach in this PR:

This currently gradually filters out annotations as they are generated or ignored. For example, if data annotations are on, CSharpDbContextGenerator simply runs the annotations through the code generator's GenerateDataAnnotation, to get rid of annotations that would be expressed via attributes - what's left is fluent API. If all core annotations are pre-emptively removed, this filtering approach doesn't work.

Similarly, a certain MaxLength may be by-convention for a certain type in a certain provider, and it's very easy to do that right now in the code generator (RemoveConventionalAnnotations). If MaxLength becomes something else that isn't an annotation, this doesn't work.
There's ways around this (pass dataAnnotations and withConventional flags to the generator to control what is generated), but in general there is something very useful in treating configuration data via a single data model (annotations), for consumers that treat need to generally handle configuration.

Just treat them as any other core facet, this shouldn't require changing your design.

I appreciate that having a uniform abstraction makes the code better. But that's not what the annotations were designed for, they are meant for extending the core model.

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs

src/EFCore.Relational/Design/AnnotationCodeGenerator.cs

smitpatel · 2020-06-19T19:49:16Z

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs

-        /// </summary>
-        /// <param name="annotations"> The annotations to remove from. </param>
-        /// <param name="annotationNames"> The ignored annotation names. </param>
-        protected virtual void IgnoreAnnotations(


Isn't this a breaking change?

There are several in this PR, since the actual design is changing quite a bit. But this is in the design assembly, where we usually have lower standards. If we feel like we need to maintain backwards compat here somehow I can look into bringing it back.

Then it contradicts

The AnnotationCodeGenerator base class exposes the same one-by-one API as before, so this is totally backwards compatible (unless the interface is implemented directly).

I am fine either if we want to break/be back-compat/obsolete but it should be consistent.

I think breaking changes in Design aren't the same as in explicitly provider-facing types such as AnnotationCodeGenerator.

smitpatel · 2020-06-19T19:54:16Z

src/EFCore.Relational/Design/AnnotationCodeGenerator.cs

+        /// <param name="property"> The <see cref="IProperty" />. </param>
+        /// <param name="annotation"> The <see cref="IAnnotation" />. </param>
+        /// <returns> <see langword="null" />. </returns>
+        public virtual AttributeCodeFragment GenerateDataAnnotation([NotNull] IProperty property, [NotNull] IAnnotation annotation)


Why is this public?

The idea is to allow providers to generate arbitrary data annotation attributes for annotations - this is the hook for that.

Ah sorry, you mean why not protected. Yeah, this and all the previously public GenerateFluentApi should be protected - though that would be a small breaking change for the latter (which I think we should do).

I mean why are we adding API for individual annotation when we are moving towards framework of all annotations together.

Most annotations still have a single fluent API (or data annotation), so this is a trivial way for providers to implement that. I'd only expect providers to override the multi-annotation API (GenerateFluentApiCalls) when multiple annotations need to go into the same fluent API call, as with identity increment/seed. This also preserves compatibility with the existing API to a large extent.

roji · 2020-06-19T20:46:27Z

Just treat them as any other core facet, this shouldn't require changing your design.

The design need to change because the core annotations no longer appear in the the dictionary I'm passing around, right? (because they've already been removed by RemoveIgnoredAnnotation, or after #19806, not even returned from GetAnnotations). So there needs to be a different mechanism for knowing whether a given annotation has already generated as a data annotation, etc.?

(not pushing back on anything, just making sure we're aligned)

AndriySvyryd · 2020-06-19T22:30:07Z

The design need to change because the core annotations no longer appear in the the dictionary I'm passing around, right? (because they've already been removed by RemoveIgnoredAnnotation, or after #19806, not even returned from GetAnnotations). So there needs to be a different mechanism for knowing whether a given annotation has already generated as a data annotation, etc.?

(not pushing back on anything, just making sure we're aligned)

That's correct, but you are already handling other facets that were never stored as annotations (e.g. .IsRequired() / [Required]), so that mechanism is in place.

roji · 2020-06-19T22:38:03Z

That's correct, but you are already handling other facets that were never stored as annotations (e.g. .IsRequired() / [Required]), so that mechanism is in place.

Am I? The idea for now was only to handle annotations. But if we want to move non-annotation handling into this as well (especially since core annotations aren't really annotations), we can do that. In that case the type should probably be renamed to something like CodeGenerationHelper - it seems like we're breaking it anyway?

AndriySvyryd · 2020-06-19T22:45:10Z

Am I? The idea for now was only to handle annotations. But if we want to move non-annotation handling into this as well (especially since core annotations aren't really annotations), we can do that. In that case the type should probably be renamed to something like CodeGenerationHelper - it seems like we're breaking it anyway?

Yes, it's handled in CSharpDbContextGenerator. If the idea for AnnotationCodeGenerator is to only handle annotations and the core annotations aren't part of this then it shouldn't handle them unless we do what you propose and handle all facets there.

roji · 2020-06-19T22:48:20Z

If the idea for AnnotationCodeGenerator is to only handle annotations and the core annotations aren't part of this then it shouldn't handle them unless we do what you propose and handle all facets there.

That was the original idea, yes (see note in the original post). But I'm fine with going further and moving non-annotation-based code generation into this as well, should we do this? In that case, do we rename to CodeGenerationHelper or similar? Note that code the gets generated differently between scaffolding and model snapshot would still stay in the respective generators.

AndriySvyryd · 2020-06-19T22:51:29Z

That was the original idea, yes (see note in the original post). But I'm fine with going further and moving non-annotation-based code generation into this as well, should we do this? In that case, do we rename to CodeGenerationHelper or similar? Note that code the gets generated differently between scaffolding and model snapshot would still stay in the respective generators.

I don't think there's enough benefit in doing this

roji · 2020-06-19T22:54:35Z

OK, I will leave core annotations and facet handling out of this type entirely, and handle only relational/provider-specific.

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs

src/EFCore.Relational/Design/AnnotationCodeGenerator.cs

Closes #16922 Closes #15675

AndriySvyryd · 2020-06-20T22:11:57Z

Another thing: make sure that ProductVersion is still being added to the snapshot, add a test if it's not.

Fixup to #21329

roji · 2020-06-21T23:22:23Z

@AndriySvyryd good catch, opened #21360.

Fixup to #21329

bricelam

Good stuff

bricelam · 2020-06-22T16:38:18Z

src/EFCore.Relational/Design/AnnotationCodeGeneratorDependencies.cs

+        /// <summary>
+        ///     The type mapper.
+        /// </summary>
+        public IRelationalTypeMappingSource RelationalTypeMappingSource { get; }


Missing With?

Am surprised the API consistency tests didn't catch this. Will fix and look into the tests too.

roji requested review from bricelam and AndriySvyryd June 18, 2020 23:12

This was referenced Jun 18, 2020

Consider making annotation names public #21331

Closed

Draft for SQL Server identity fluent API in model snapshot #21210

Closed

AndriySvyryd reviewed Jun 19, 2020

View reviewed changes

src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs Outdated Show resolved Hide resolved

AndriySvyryd reviewed Jun 19, 2020

View reviewed changes

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs Outdated Show resolved Hide resolved

AndriySvyryd reviewed Jun 19, 2020

View reviewed changes

src/EFCore.Relational/Design/AnnotationCodeGenerator.cs Outdated Show resolved Hide resolved

AndriySvyryd reviewed Jun 19, 2020

View reviewed changes

src/EFCore.Relational/Design/AnnotationCodeGenerator.cs Outdated Show resolved Hide resolved

smitpatel reviewed Jun 19, 2020

View reviewed changes

roji requested a review from AndriySvyryd June 20, 2020 13:11

AndriySvyryd reviewed Jun 20, 2020

View reviewed changes

src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs Outdated Show resolved Hide resolved

AndriySvyryd reviewed Jun 20, 2020

View reviewed changes

src/EFCore.Relational/Design/AnnotationCodeGenerator.cs Outdated Show resolved Hide resolved

AndriySvyryd approved these changes Jun 20, 2020

View reviewed changes

roji force-pushed the AnnotationCodeGeneration branch from 527aee2 to 96becc2 Compare June 20, 2020 20:15

Refactor annotation code generation

c4a2bad

Closes #16922 Closes #15675

roji force-pushed the AnnotationCodeGeneration branch from 96becc2 to c4a2bad Compare June 20, 2020 20:23

roji merged commit 61f3c0b into master Jun 20, 2020

roji deleted the AnnotationCodeGeneration branch June 20, 2020 21:17

roji added a commit that referenced this pull request Jun 21, 2020

Generate ProductVersion in model snapshot again

03c61a4

Fixup to #21329

roji mentioned this pull request Jun 21, 2020

Generate ProductVersion in model snapshot again #21360

Merged

roji added a commit that referenced this pull request Jun 22, 2020

Generate ProductVersion in model snapshot again (#21360)

91f1937

Fixup to #21329

bricelam reviewed Jun 22, 2020

View reviewed changes

ajcvickers mentioned this pull request Jun 25, 2020

Entity Framework Weekly Status Updates (2020) #19549

Closed

bricelam mentioned this pull request Jul 31, 2020

Design: Generate more code using IAnnotationCodeGenerator #21882

Closed

bricelam added breaking-change providers-beware labels Jul 31, 2020

roji mentioned this pull request Mar 19, 2021

Migrations: Provider-rendered annotations in code #6546

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor annotation code generation #21329

Refactor annotation code generation #21329

roji commented Jun 18, 2020

AndriySvyryd commented Jun 19, 2020 •

edited

Loading

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020 •

edited

Loading

smitpatel Jun 19, 2020

roji Jun 19, 2020

smitpatel Jun 19, 2020

roji Jun 19, 2020 •

edited

Loading

smitpatel Jun 19, 2020

roji Jun 19, 2020

roji Jun 19, 2020

smitpatel Jun 19, 2020

roji Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 20, 2020

roji commented Jun 21, 2020

bricelam left a comment

bricelam Jun 22, 2020

roji Jun 26, 2020

Refactor annotation code generation #21329

Refactor annotation code generation #21329

Conversation

roji commented Jun 18, 2020

Notes

AndriySvyryd commented Jun 19, 2020 • edited Loading

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 19, 2020

roji commented Jun 19, 2020

AndriySvyryd commented Jun 20, 2020

roji commented Jun 21, 2020

bricelam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd commented Jun 19, 2020 •

edited

Loading

AndriySvyryd commented Jun 19, 2020 •

edited

Loading

roji Jun 19, 2020 •

edited

Loading