Skip to content

Update enum quoting and default schema use - #605

Merged
austindrenski merged 4 commits into
npgsql:devfrom
austindrenski:schema-enums
Oct 26, 2018
Merged

Update enum quoting and default schema use#605
austindrenski merged 4 commits into
npgsql:devfrom
austindrenski:schema-enums

Conversation

@austindrenski

@austindrenskiaustindrenski commented Aug 17, 2018

Copy link
Copy Markdown
Contributor

Changes

  • If no schema is specified in .ForNpgsqlHasEnum(...), then use the default schema annotation.
  • Properly quote schema-qualified enums for literal generation and mapping (override StoreType).
  • Cache mappings from CLR values to PostgreSQL labels.

Fixes: #554
Fixes: #593

@YohDeadfall

YohDeadfall commented Aug 31, 2018

Copy link
Copy Markdown
Contributor

Does this request fix #623? If not, please fix it in a different PR, not here. This would make it easier to track changes.

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@YohDeadfall I mistakenly thought there would be some overlap in the issues, but they are indeed distinct. None of the changes included here are intended to address #623 (other than improving enum support, generally).

@YohDeadfallYohDeadfall left a comment

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.

Otherwise looks good.

Comment threadsrc/EFCore.PG/Storage/Internal/Mapping/NpgsqlEnumTypeMapping.cs Outdated
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

Updated to Npgsql 4.1.0-ci.1270 to use the default name translator from npgsql/npgsql#2137.

@austindrenski
austindrenskiforce-pushed the schema-enums branch 2 times, most recently from cbc1f47 to b93e1eeCompareSeptember 1, 2018 22:58
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated to reflect #621.

@roji

roji commented Sep 4, 2018

Copy link
Copy Markdown
Member

@austindrenski I'll give this a full review, but can you please make clearer exactly what this PR covers? Is the description above still valid or has the scope of this PR changed?

Regardless, one thing that immediately caught my eye:

Default schemas will affect downstream created types that do not specify schemas.

This doesn't seem like the correct EF Core behavior. Executing the following:

builder.Entity().ToTable("blogs");
builder.HasDefaultSchema("my_schema");
builder.Entity().ToTable("blogs2");

Results in two tables being created under my_schema. As a general rule, everything that happens in OnModelCreating() configures the model and has no order whatsoever; the default schema is a property on the model that simply gets set. Then, we the database is actually created, the model is translated to migrations, but the original order in OnModelCreating() has already been lost.

@roji

roji commented Sep 4, 2018

Copy link
Copy Markdown
Member

BTW does this depend on npgsql/npgsql#2121, whose PR npgsql/npgsql#2123 still seems to be WIP?

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

can you please make clearer exactly what this PR covers?

Sure, I'll clean that up today. This PR grew and shifted a bit since I opened it, so the current description is a bit of lazy editing on my part.

This doesn't seem like the correct EF Core behavior.

Early on for EF Core, I thought I read some docs that discussed order being relevant in configuration, but I can't seem to find anything like that now. (Could have been a third-party blog, TBH.)

What you describe makes more sense (and matches the internals too). I can go back through and move the default schema detection later in the process.

BTW does this depend npgsql/npgsql#2121

No, not anymore. An earlier draft was, but I reworked this one to use the simple "split on the first dot" approach while I work through npgsql/npgsql#2121 (which has turned out to be a bit more confusing than I first imagined).

@austindrenski
austindrenskiforce-pushed the schema-enums branch 2 times, most recently from 04dd4d3 to 46298b7CompareSeptember 4, 2018 14:33
@austindrenskiaustindrenski changed the title Fix up various enum mappings/annotations Update enum quoting and default schema useSep 4, 2018
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@roji The description is up to date, and I've rebased for the current dev. I've also rebased #626 which follows this PR with similar fixes for user-defined ranges.

I'd like to see this make it into v2.2.0, any chance you can give it a review before the next preview release?

@YohDeadfall Could you also give this another review?

Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs Outdated
Comment threadsrc/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs Outdated
@austindrenski
austindrenskiforce-pushed the schema-enums branch 2 times, most recently from c758832 to 5f8e109CompareOctober 1, 2018 13:58
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@YohDeadfall Appreciate the review—updated.

- Properly quote schema and type name
- Respect default schema annotation
- Cache enum values and simplify non-null literal generation
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

I'd like to merge this and #626 tomorrow at 1:00 PM (UTC).

If anyone wants more time for another round of review, just let me know.

/cc @roji@YohDeadfall

@austindrenski
austindrenski merged commit 89b9742 into npgsql:devOct 26, 2018
@austindrenski
austindrenski deleted the schema-enums branch October 26, 2018 00:35
Sign up for freeto 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.

3 participants

@austindrenski@YohDeadfall@roji