Update range mapping for schema-qualified and quoted ranges

[austindrenski]

Changes

• If no schema is specified in .ForNpgsqlHasRange(...), then use the default schema annotation.
• Properly quote schema-qualified ranges for literal generation and mapping (override StoreType).

[austindrenski]

This should follow #672 for changes in the NodaTime plugin.

[YohDeadfall]

Please address my comment. Otherwise, LGTM.

[roji]

Finally had some time to review stuff...

Apart from the specific comments, I'm not too enthusiastic about including the delimiters in the store type - in previous work I tried to keep the StoreType clean, and add the delimiters only as needed when generating migrations SQL. This was the previous logic when creating managing enums, which you changed in #605. The new logic creates an odd discrepancy between the tables/sequences/indices (which have separate schema and name) and enums/ranges (which don't, and therefore have the schema packed and delimited into the name). Of course, if we want to support enums/ranges in non-default schemas we don't have much choice - EF Core has no concept of a type's schema, and treats a store type as a single string (e.g. in AddColumnOperation, RelationalTypeMapping) so have to delimit and pack the type's schema and name into a single string StoreType.

We may want to raise this with the EF Core people - it may make sense for them to add type schemas where necessary in the core, obviating the current logic. Since enums/ranges in user-defined schemas seem pretty rare, I'd at least want to hear their opinion before implementing this new logic (which has already been merged for enums). I think it wouldn't be so bad for enums/ranges in user-defined schemas to not be supported before EF Core 3.0, if it means we get it right.

[austindrenski]

@roji Updated to address your review. I've also opened #685 as a follow-up to #605 based on your review.

We may want to raise this with the EF Core people - it may make sense for them to add type schemas where necessary in the core, obviating the current logic.

Agreed. I'll open an issue and link back to this.

Since enums/ranges in user-defined schemas seem pretty rare, I'd at least want to hear their opinion before implementing this new logic (which has already been merged for enums). I think it wouldn't be so bad for enums/ranges in user-defined schemas to not be supported before EF Core 3.0, if it means we get it right.

I can't speak to rarity in general...but I do work frequently with databases that have schema-qualified enums, so the previous logic was a real headache in day-to-day usage.

I would be happy to see this logic be reimplemented upstream so that enums/ranges behave more like tables/indexes/etc, but I don't think that means we need to wait for the 3.0 release. In my opinion, the components at play here are implementation details and changes occurring from 2.2 to 3.0 are unlikely to have a serious impact on consumers.

[austindrenski]

While putting together #626 I found that an AlterDatabaseOperation doesn't receive the DefaultSchema annotation from the model:

https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/blob/e7bea847e458f9aa68ff14c663ba0f9f46359537/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs#L647-L662

One possible fix is to do something like this (included here in 077d561):

protected override void Generate(
    AlterDatabaseOperation operation,
    IModel model,
    MigrationCommandListBuilder builder)
{
    Check.NotNull(operation, nameof(operation));
    Check.NotNull(builder, nameof(builder));

    // 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];

    GenerateEnumStatements(operation, model, builder);
    GenerateRangeStatements(operation, model, builder);

    foreach (var extension in PostgresExtension.GetPostgresExtensions(operation))
        GenerateCreateExtension(extension, model, builder);

    builder.EndCommand();
}

This is needed because of the way we store enum/range annotations. HasDefaultSchema could be called after the annotation key was created without a schema.

Thus in the current implementation (i.e. here in this PR), PostgresRange.Schema can return a default schema for a range whose annotation (e.g. key) does not include that schema.

While this isn't currently causing issues, it does seems serious from a correctness standpoint. I'm starting to look into how we could store the enums without a reliance on the name/schema. I haven't tested this yet, but I suspect this could also affect other objects stored as annotations by name/schema.

edit:

I played around with a couple of alternatives (e.g. storing an annotation whose value is a collection of PostgresEnum), but it makes for a more complicated implementation. I'm going to leave this all as is for now, since the only time the inconsistency would come into play is if someone went out of their way to look up an annotation by name (rather than prefix) and tried to parse the annotation directly (rather than by using the getters).

[austindrenski]

Updated for the changes from #698. Ready for another review, @roji @YohDeadfall.

[roji]

Looking good! Please check out the nits and possibly unneeded DefaultSchema annotation on the migration operation before merging.

Let's prioritize this to make sure it makes it into 2.2.

[austindrenski]

@roji Thanks for the re-review. I've updated accordingly.

The one questionable change is the XML doc referring to the default schema, I put in some language, but not sure if it was what you meant. Could you give that another quick look?

[austindrenski]

@roji It looks like we are missing a section for NpgsqlAnnotationNames.RangePrefix in NpgsqlAnnotationCodeGenerator, so I've added it here in 3f44487.

[roji]

Thanks for making the changes. Look at my two minor comments, otherwise LGTM!

[roji]

@austindrenski this PR broke design-time operations, see ad4e0d6 for the fix.

[austindrenski]

Oh yikes.....sorry about that. Thanks for fixing it!

[roji]

No problem :) It's a bit troubling to not have end-to-end testing to catch this kind of thing - we're not perfect yet :)