vapor / fluent-postgres-driver

🐘 PostgreSQL driver for Fluent.
MIT License
149 stars 53 forks source link

Nested enums with same name overwrite each other #82

Closed clayellis closed 6 years ago

clayellis commented 6 years ago

I'm seeing a behavior with PostgreSQLEnum that may need to be changed or documented as it is definitely something that isn't easily debugged.

If you have two nested enums with the same name, .e.g: Category.Kind and Transaction.Kind, the postgreSQLEnumTypeName for both will be the same (KIND). This means that when the migrations are run, the first will be overridden by the second. Inspecting the psql db and querying for types using \dT confirms this with a single listing for kind.

This is potentially misleading behavior as I should reasonably expect that since Category.Kind and Transaction.Kind are both unique enums in Swift, that the default migrations should provide two unique psql enums (perhaps category_kind and transaction_kind).

I haven't confirmed this behavior with other databases.

Current workarounds are:

  1. Provide a unique static postgreSQLEnumTypeName ("CATEGORY_KIND"), as well as a unique static migrationName ("Category.Kind"). Preferred, but it seems like this could be default behavior β€” each type, nested or not, should have their own unique migration name by default.
  2. Don't nest the enums and use a prefix (Category.Kind > CategoryKind). Not preferred because it's less Swifty.
  3. Choose unique names for each enum (Category.Kind, Transaction.Style). Not preferred because it's not very scalable across a large project, (the list of synonyms for "type/kind" is short.)
clayellis commented 6 years ago

Possible solution based on workaround 1

Ensure that postgreSQLEnumTypeName and migrationName are unique for all types β€” nested or not β€” by relying on the full reflected path of the type (minus the module name) which is guaranteed by the type system to be unique.

FluentPostgreSQL.PostgreSQLEnum.swift

// Current code would return `"KIND"` for both `Category.Kind` and `Transaction.Kind`.
public static var postgreSQLEnumTypeName: String {
    return String(reflecting: self) // "App.Category.Kind"
        .components(separatedBy: ".")
        .dropFirst() // Drop the module name "App"
        .joined(separator: "_") // "Category_Kind"
        .uppercased() // "CATEGORY_KIND"
}

Fluent.AnyMigration.swift

// Current code would return `"Kind"` for both `Category.Kind` and `Transaction.Kind`.
public static var migrationName: String {
    return String(reflecting: self) // "App.Category.Kind"
        .components(separatedBy: ".")
        .dropFirst() // Drop the module name "App"
        .joined(separator: ".") // "Category.Kind"
}

This does introduce a change to the default migrationName to include the type's full module path (minus the module: App.). So for instance, where Category.Kind used to have a default migrationName of Kind, it would now be Category.Kind. And the default postgreSQLEnumTypeName changes from KIND to CATEGORY_KIND.

These changes are actually positive IMO, as they add further clarity for nested types (models, migrations) while leaving top-level types unchanged.

@tanner0101 do you have an opinion or insight here as to how this might affect things going forward? I'm afraid this would be a breaking change for existing projects.