Schema enums follow up (part 2)

[austindrenski]

Follow-up to #605 and #626.

[YohDeadfall]

Just few nits.

[austindrenski]

@roji You asked previously about some code that looked like this:

// Alter operations may not have a default schema attached. If not, try to attach one for schema-qualified types.
if (operation[RelationalAnnotationNames.DefaultSchema] == null)
    operation[RelationalAnnotationNames.DefaultSchema] = model[RelationalAnnotationNames.DefaultSchema];

I think my previous answer was incomplete. Completely removing that code in #626 worked because #605 had some (unintentionally) hackish code that covered up the root of the issue by accessing the model as a fallback when a schema was unavailable for an enum.

It was a bit confusing to unwind, but I think the root of the issue is that we weren't yielding the default schema annotation from NpgsqlMigrationsAnnotationProvider.For(...) which meant that AlterDatabaseOperation wouldn't have access to it, and so a PostgresEnum or PostgresRange on that operation wouldn't have access to it either.

I've changed that here, such that NpgsqlMigrationsAnnotationProvider.For(...) now yields the default schema annotation, and everything is back to passing (without the hacks).

[roji]

It looks good, thanks! I'd just like to understand about the DefaultSchema annotation not being present in SqlServerMigrationsAnnotationProvider... Can you please try to take a look?

[austindrenski]

@roji I think this is very close to ready, but there are some changes since your last review that I'd like to get your OK on before merging.

Specifically:

• PostgresEnum.Labels now has a private setter, which is used in place of calling SetData directly.
• I've removed the schema-less overload for PostgresEnum.GetOrAddPostgresEnum(...). An equivalent form is still available via model.Npgsql().GetOrAddPostgresEnum(...).
• I've removed PostgresEnum.FindPostgresEnum, which was only used within PostgresEnum and wasn't necessary. If this type of functionality is needed down the road, we could provide it via something like model.Npgsql().FindPostgresEnum.
• I've removed PostgresEnum.BuildAnnotationName(...), per your review, as it was no longer necessary.
• I've updated PostgresEnum.GetData() to push the responsibility of returning a null-membered tuple down to PostgresEnum.Deserialize(...). We were doing a null-check there anyways, so this just reduces some branching.

These changes are focused on simplifying PostgresEnum so that responsibility for convenience wrappers resides with classes like NpgsqlModelAnnotations.

Also note I've split changes that were unrelated to the annotations into #724.

[roji]

@austindrenski thanks for continuing to work on this. I'm not going to review again because of the DefaultSchema binding question - see my comment above. Once we resolve all that I'll give this another full review.

[austindrenski]

@roji Handling this via NpgsqlMigrationsAnnotationProvider worked out really well. The enum migration operations are back to being self-contained, and PostgresEnum is much less mutable.

Once this is approved, I'll submit a pair of PRs to align PostgresExtension and PostgresRange with the new look of PostgresEnum.

I've put together a pair of PRs to update PostgresRange (#728) and PostgresExtension (#729).

[roji]

Opened #730 to discuss the ModelDiffer/custom migration question again.

[austindrenski]

First a more minor point. This PR indeed changes the binding of the default schema as discussed, but it also changes the API of PostgresEnum in general - the constructor now includes the previous "get or add" functionality, a new static CreateAnnotation() is an IAnnotation factory, Labels is no longer modifiable...

I recognize that these are departures from the previous design, and I can roll back some of it (e.g. file layout), but we do need some of these changes. For example, CreateAnnotation(...) is what allows us to robustly return annotations in the annotation provider that are potentially different from the annotation that is actually serialized in the model.

Regarding the mutability of Labels, my primary concern is what would actually be mutated? Is it the model, a migration operation, a scaffolding model? The impact of setting that property depends on what the underlying annotation is, and if the underlying annotation is intended to be mutable.

With those questions outstanding, and since we don't (yet!) support altering enums, I would rather keep these as immutable as possible, at least until we start to support alter operations on them.

Since we can't implement the standard EF Core behavior here (automatic rename on change of DefaultSchema), I suggest we not try to do anything fancy and leave things as simple as possible, and as close as possible to EF Core. This seems to be to do the same thing as Sequence - leave the schema null in the annotation, and return DefaultSchema from the schema property.

That works for me. But I think that this does require something like CreateAnnotations(...) to keep the annotation provider as clean as possible.

I'm open to other ways to accomplish this, but I settled on the current design because it reduced the need to duplicate annotation logic and kept the "serialization" logic inside of PostgresEnum.

---

I've updated the PR to rollback some of the obvious stuff and rework the point of access for the default schema. Let me know how that looks, or if you think more needs to be rolled back (i.e. if settable properties are critical).

I'll update the other PRs now, but don't worry about looking at them yet, I just want to make sure the patterns/logic look good in the other metadata classes too.

[roji]

I recognize that these are departures from the previous design, and I can roll back some of it (e.g. file layout), but we do need some of these changes. For example, CreateAnnotation(...) is what allows us to robustly return annotations in the annotation provider that are potentially different from the annotation that is actually serialized in the model.

My point was that this was not actually something we should be doing - I took back my idea of translating the model enum annotation into a different annotation for the migrations... The reasoning is that if the user doesn't specify a schema for the enum, then the generated migration should actually not contain the default schema (again, I reversed what I had previously written). Instead, the migration code should contain null for the schema - just like table and sequence do - and the binding should only occur very late, at the SQL migration phase. I recommend taking a look at Sequence.cs for inspiration on how PostgresEnum.cs should look like, even if they're not going to be identical.

Just to be extra clear: my change of mind here really stemmed from a misunderstanding of how EF Core works... If a user doesn't specify a schema, the resulting migration should not contain a schema - even if the actual object gets creating in the default schema - and if the default schema later changes, then EF Core moves the object from the old default schema to the new one, at that very moment. Keeping the schema null is in fact required in order to be able to detect objects which were created in the (previous) default schema; if the schema were baked into the annotation, we wouldn't be able to know whether the user specified it (in which case it shouldn't move) or whether it was just the default (in which case it should).

Regarding the mutability of Labels, my primary concern is what would actually be mutated? Is it the model, a migration operation, a scaffolding model? The impact of setting that property depends on what the underlying annotation is, and if the underlying annotation is intended to be mutable.

That's a valid question... I think that the mutability of the metadata (i.e. the annotations) isn't the same as our capability to carry out the corresponding change in the database. In other words, there may be scenarios where the metadata is simply built incrementally by the user, i.e. they interact directly with PostgresEnum to build the list of values based on some logic, rather than using the ForNpgsqlHasEnum() extension method. Don't forget that PostgresEnum, as well as NpgsqlModelAnnotations and the rest are completely public and accessible to the user. I think that's the point behind allowing mutation - just as a way of building the metadata on the model.

I'll really try to be available tonight (your morning) so we can have some online time together to discuss all this.

[austindrenski]

(again, I reversed what I had previously written). Instead, the migration code should contain null for the schema - just like table and sequence do - and the binding should only occur very late, at the SQL migration phase.

OK, it sounds like you're recommending this approach:

protected virtual void GenerateCreateEnum(
    [NotNull] PostgresEnum enumType,
    [NotNull] IModel model,
    [NotNull] MigrationCommandListBuilder builder)
{
    var schema = enumType.Schema ?? model.Relational().DefaultSchema;

If so, this approach will definitely work, and it doesn't require any modifications to the annotation provider (or individual annotations).

In other words, there may be scenarios where the metadata is simply built incrementally by the user

Ah, that sounds very obvious now, but I was working under the impression that these were more or less public-for-internal-use (e.g. us or other downstream providers) rather than public-for-public-use (e.g. end users).

If we intend end users to build these incrementally, then mutable properties make more sense.

[austindrenski]

Shoot. I've botched that last push. Hold off on review, I'll post back once fixed.

[austindrenski]

OK, cleaned up and ready for review.

[roji]

This is looking almost ready. I don't have time at the moment for a full review but will be a bit more available later - see a minor comment below. In general, I think we should try to make this as close as possible EF Core's Sequence implementation.

[roji]

Check out my comments again, I think we're pretty close. I'll be available in the coming hours to discuss.

[roji]

Somehow my comments got split into two reviews, strange...

[austindrenski]

OK, the only notable divergence in the public API is the removal of the schema-free overload to GetOrAddPostgresEnum.

Personally, I don't see the value in that overload since the annotation classes provide user-friendly overloads, but I can revert to include it if you think it still has some value.

[roji]

OK, barring the two minor comments I think we're good to go...! Thanks for all of your patience with this.

Do you want to submit the same for range and extensions? FYI I'm pretty much done on my side, as soon as we finish up all of this I'll proceed to releasing...

[austindrenski]

@roji Just updated. Are you able to initiate the release of 4.0.4 (driver + plugins)?

[roji]

Looks good @austindrenski, go ahead and merge. I'll start release for Npgsql 4.0.4.