vapor / fluent-kit

Swift ORM (queries, models, and relations) for NoSQL and SQL databases
MIT License
215 stars 115 forks source link

Improve FieldKey ergonomics #596

Open nastynate13 opened 9 months ago

nastynate13 commented 9 months ago

Is your feature request related to a problem? Please describe.

Swift developers tend to avoid using string literals

@Field(key: "literal")

instead opting for an enum based approach

enum FooKey: String  {
    case typeSafeKey = "type_safe_key"
 }

This solves quite a few problems - namely:

  1. Reusability
  2. Flexibility
  3. Eliminates common typo-related bugs.
  4. Type safety

When using Fluent, keys for a Model are represented by a FieldKey which conveniently conforms to ExpressibleByStringLiteral. This affords the following options:

  1. Use a string literal for the FieldKey

@Field(key: "literal")

This approach can certainly be convenient when prototyping, however it still suffers from every issue named above.

  1. Use FieldKey as the RawValue type of an enum case
enum ModelKey: FieldKey {
  case typeSafeKey = "type_safe_key"
}

@Field(key: ModelKey.typeSafeKey.rawValue)

Although this solves many of the above problems, a new problem of verbosity is introduced, so I'd argue that this approach remains "not great enough" and ultimately feels like we're fighting the api a bit.

  1. Use a static let on the enum to define the FieldKey
enum ModelKey  {
  static let typeSafeKey: FieldKey = "type_safe_key"
}

@Field(key: ModelKey.typeSafeKey)

This is less verbose at the call site which is nice. However, by using a static variable we don't get type inference and instead have to declare the FieldKey type for each key. We also lose the inherent benefits of using enum cases to define our keys. This includes switching, key iteration (via CaseIterable), implicit raw value conversion (useful for non-compound names), compile time checking of raw value duplicates, and overall reduced verbosity at the creation site.

  1. Extend Field Key directly
extension FieldKey {
    static var typeSafeKey: Self { "type_safe_key" }
}

This feels like the best approach because it affords us the ability to use leading dot syntax for any FieldKey - a big win for ergonomics and code duplication for keys with the same raw value. There are still caveats to this approach though:

1) Because keys are no longer scoped to a type for a particular model, this could lead to confusion about whether the key you intend to define for a model has indeed already been defined to name a field in another model.

2) Having all keys defined in the same global space could quickly become cluttered and difficult to manage or track for larger projects. Scoping a model's keys to the model it is keying is arguably a more natural, manageable, and portable fit.

3) We miss out on many of the aforementioned benefits of defining the FieldKey as a Enum case rather than a static var.

4) Although this appears to be a leading candidate as an approach to defining keys, nowhere in the docs is this demonstrated or suggested and therefore it might not be fundamentally apparent, especially to those new to Vapor.

We now have an additional problem which I'll label "Uncertainty". Although there are various alternatives that solve part of the problem, - extending FieldKey being the leading candidate IMO - there is no standard or suggested approach.

Describe the solution you'd like

I would love to see a solution that solves :

  1. Reusability
  2. Flexibility
  3. Eliminates common typo-related bugs.
  4. Type safety
  5. Uncertainty.
  6. Fewer key strokes.
  7. Clarity
final class FooModel: KeyedModel {
  static let schema = "foos"

  @ID()
  var id: UUID?

  @Field(key: .typeSafeKey)
  var typeSafeKey: String

  @Field(key: .this)
  var this: String

  @Field(key: .feels)
  var feels: String

  @Field(key: .better)
  var better: String

  enum Keys: String {
    case typeSafeKey = "type_safe_key"
    case this
    case feels
    case better
  }

}

This approach brings us the same leading dot syntax that static extensions on FieldKey affords and keys that are scoped to the model they define for more clarity.

Currently I have this implemented using an opt in approach that uses 3 new protocols.

protocol StringlyKeyed {
  associatedtype Keys: RawRepresentable where Keys.RawValue == String
}

protocol KeyedModel: Model, KeyedFields {}
protocol KeyedFields: Fields, StringlyKeyed {}

I do not purport to say this is a great solution or implementation, but I prefer the clarity of this approach to the alternatives. Model.Keys can be loosely compared to a CodingKey for Codable types.

The implementation I'm using is admittedly rushed and hacky but I've added it below just as a proof of concept.

extension FieldProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}
extension OptionalFieldProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}
extension EnumProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}
extension OptionalEnumProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}
extension GroupProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}
extension ParentProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}
extension OptionalParentProperty where Model: KeyedFields {
  internal convenience init(key: Model.Keys) {
    self.init(key: .string(key.rawValue))
  }
}

These extensions alone do not allow for using identical dot syntax in other places where a FieldKey is expected - namely migrations (i.e. SchemaBuilder- which - afaik - has no knowledge of the type of model it is building presumably for very good reason / by design). In order for this to be comprehensive enough across the api to be coherent, I've added a way to integrate SchemeBuilder so we could write this:

struct CreateFoo: AsyncMigration {

func prepare(on database: Database) async throws {
  try await database.schema(FooModel.self)
    .id()
    .field(.typeSafeKey, .string, .required)
    .field(.this, .string, .required)
    .field(.feels, .string, .required)
    .field(.better, .string, .required)
    .create()
  }
}

Here is an ad hoc implementation that uses a wrapper type:


// A wrapper type that allows a SchemaBuilder to access a KeyedModel's associated Keys type allowing for leading dot syntax
final class KeyedSchemaBuilder<Model: KeyedModel>: StringlyKeyed {
  typealias Keys = Model.Keys

  fileprivate let builder: SchemaBuilder
  fileprivate let database: Database

  init(_ modelType: Model.Type, database: Database, space: String? = nil) {
    self.builder = database.schema(Model.schema, space: space)
    self.database = database
  }
}

// Creates a KeyedSchemaBuilder
extension Database {
  func schema<T: KeyedModel>(_ modelType: T.Type, space: String? = nil) -> KeyedSchemaBuilder<T> {
    return .init(modelType.self, database: self, space: space)
  }
}

// Wrapper functions that map a KeyedModel.Keys to every corresponding SchemaBuilder method that takes `FieldKey`s
extension KeyedSchemaBuilder {

  func field(
    _ key: Keys,
    _ dataType: DatabaseSchema.DataType,
    _ constraints: DatabaseSchema.FieldConstraint...
  ) -> Self {
    self.builder.field(.definition(
      name: .key(.string(key.rawValue)),
      dataType: dataType,
      constraints: constraints
    ))
    return self
  }

// this could be extended to handle various levels of nesting for nested Groups
  func group<U: RawRepresentable>(
    _ key: Keys,
    _ key2: U,
    _ dataType: DatabaseSchema.DataType,
    _ constraints: DatabaseSchema.FieldConstraint...
  ) -> Self where U.RawValue == String {
    let joined: String = [key.rawValue, key2.rawValue].joined(separator: "_")
    self.builder.field(.definition(
      name: .key(.string(joined)) ,
      dataType: dataType,
      constraints: constraints
    ))
    return self
  }

  @discardableResult
  public func unique(on keys: Keys..., name: String? = nil) -> Self {
    self.builder.constraint(.constraint(
      .unique(fields: keys.map { .key(.string($0.rawValue)) }),
      name: name
    ))
    return self
  }

  @discardableResult
  public func compositeIdentifier(over keys: Keys...) -> Self {
    self.builder.constraint(.constraint(.compositeIdentifier(keys.map { .key(.string($0.rawValue)) }), name: ""))
    return self
  }

  @discardableResult
  public func deleteUnique(on keys: Keys...) -> Self {
    self.builder.deleteConstraint(.constraint(.unique(fields: keys.map { .key(.string($0.rawValue)) })))
    return self
  }

  @discardableResult
  public func foreignKey(
    _ key: Keys,
    references foreignSchema: String,
    inSpace foreignSpace: String? = nil,
    _ foreignKey: Keys,
    onDelete: DatabaseSchema.ForeignKeyAction = .noAction,
    onUpdate: DatabaseSchema.ForeignKeyAction = .noAction,
    name: String? = nil
  ) -> Self {
    self.builder.schema.createConstraints.append(.constraint(
      .foreignKey(
        [.key(.string(key.rawValue))],
        foreignSchema,
        space: foreignSpace,
        [.key(.string(foreignKey.rawValue))],
        onDelete: onDelete,
        onUpdate: onUpdate
      ),
      name: name
    ))
    return self
  }

  @discardableResult
  public func foreignKey(
    _ keys: [Keys],
    references foreignSchema: String,
    inSpace foreignSpace: String? = nil,
    _ foreignKeys: [Keys],
    onDelete: DatabaseSchema.ForeignKeyAction = .noAction,
    onUpdate: DatabaseSchema.ForeignKeyAction = .noAction,
    name: String? = nil
  ) -> Self {
    self.builder.schema.createConstraints.append(.constraint(
      .foreignKey(
        keys.map { .key(.string($0.rawValue)) },
        foreignSchema,
        space: foreignSpace,
        foreignKeys.map { .key(.string($0.rawValue)) },
        onDelete: onDelete,
        onUpdate: onUpdate
      ),
      name: name
    ))
    return self
  }

  @discardableResult
  public func updateField(
    _ key: Keys,
    _ dataType: DatabaseSchema.DataType
  ) -> Self {
    self.builder.updateField(.dataType(
      name: .key(.string(key.rawValue)),
      dataType: dataType
    ))
    return self
  }

  @discardableResult
  public func deleteField(_ name: Keys) -> Self {
    self.builder.deleteField(.key(.string(name.rawValue)))
    return self
  }

}

// Wrapper functions for every SchemaBuilder method
extension KeyedSchemaBuilder {
  @discardableResult
  public func id() -> Self {
    self.builder.field(.id, .uuid, .identifier(auto: false))
    return self
  }

  @discardableResult
  public func field(
    _ key: FieldKey,
    _ dataType: DatabaseSchema.DataType,
    _ constraints: DatabaseSchema.FieldConstraint...
  ) -> Self {
    self.builder.field(.definition(
      name: .key(key),
      dataType: dataType,
      constraints: constraints
    ))
    return self
  }

  @discardableResult
  public func field(_ field: DatabaseSchema.FieldDefinition) -> Self {
    self.builder.schema.createFields.append(field)
    return self
  }

  @discardableResult
  public func unique(on fields: FieldKey..., name: String? = nil) -> Self {
    self.builder.constraint(.constraint(
      .unique(fields: fields.map { .key($0) }),
      name: name
    ))
    return self
  }

  @discardableResult
  public func compositeIdentifier(over fields: FieldKey...) -> Self {
    self.builder.constraint(.constraint(.compositeIdentifier(fields.map { .key($0) }), name: ""))
    return self
  }

  @discardableResult
  public func constraint(_ constraint: DatabaseSchema.Constraint) -> Self {
    self.builder.schema.createConstraints.append(constraint)
    return self
  }

  @discardableResult
  public func deleteUnique(on fields: FieldKey...) -> Self {
    self.builder.deleteConstraint(.constraint(.unique(fields: fields.map { .key($0) })))
    return self
  }

  /// Delete a FOREIGN KEY constraint with the given name.
  ///
  /// This method allows correctly handling referential constraints with custom names when using MySQL 5.7
  /// without being forced to also know the full definition of the constraint at the time of deletion. See
  /// ``DatabaseSchema/ConstraintDelete/namedForeignKey(_:)`` for a more complete discussion.
  @discardableResult
  public func deleteForeignKey(name: String) -> Self {
    self.builder.deleteConstraint(.namedForeignKey(name))
    return self
  }

  @discardableResult
  public func deleteConstraint(name: String) -> Self {
    self.builder.deleteConstraint(.name(name))
    return self
  }

  @discardableResult
  public func deleteConstraint(_ constraint: DatabaseSchema.ConstraintDelete) -> Self {
    self.builder.schema.deleteConstraints.append(constraint)
    return self
  }

  @discardableResult
  public func foreignKey(
    _ field: FieldKey,
    references foreignSchema: String,
    inSpace foreignSpace: String? = nil,
    _ foreignField: FieldKey,
    onDelete: DatabaseSchema.ForeignKeyAction = .noAction,
    onUpdate: DatabaseSchema.ForeignKeyAction = .noAction,
    name: String? = nil
  ) -> Self {
    self.builder.schema.createConstraints.append(.constraint(
      .foreignKey(
        [.key(field)],
        foreignSchema,
        space: foreignSpace,
        [.key(foreignField)],
        onDelete: onDelete,
        onUpdate: onUpdate
      ),
      name: name
    ))
    return self
  }

  @discardableResult
  public func foreignKey(
    _ fields: [FieldKey],
    references foreignSchema: String,
    inSpace foreignSpace: String? = nil,
    _ foreignFields: [FieldKey],
    onDelete: DatabaseSchema.ForeignKeyAction = .noAction,
    onUpdate: DatabaseSchema.ForeignKeyAction = .noAction,
    name: String? = nil
  ) -> Self {
    self.builder.schema.createConstraints.append(.constraint(
      .foreignKey(
        fields.map { .key($0) },
        foreignSchema,
        space: foreignSpace,
        foreignFields.map { .key($0) },
        onDelete: onDelete,
        onUpdate: onUpdate
      ),
      name: name
    ))
    return self
  }

  @discardableResult
  public func updateField(
    _ key: FieldKey,
    _ dataType: DatabaseSchema.DataType
  ) -> Self {
    self.builder.updateField(.dataType(
      name: .key(key),
      dataType: dataType
    ))
    return self
  }

  @discardableResult
  public func updateField(_ field: DatabaseSchema.FieldUpdate) -> Self {
    self.builder.schema.updateFields.append(field)
    return self
  }

  @discardableResult
  public func deleteField(_ name: FieldKey) -> Self {
    self.builder.deleteField(.key(name))
    return self
  }

  @discardableResult
  public func deleteField(_ name: DatabaseSchema.FieldName) -> Self {
    self.builder.schema.deleteFields.append(name)
    return self
  }

  @discardableResult
  public func ignoreExisting() -> Self {
    self.builder.schema.exclusiveCreate = false
    return self
  }

  public func create() async throws {
    self.builder.schema.action = .create
    try await self.database.execute(schema: self.builder.schema).get()
  }

  public func update() async throws {
    self.builder.schema.action = .update
    try await self.database.execute(schema: self.builder.schema).get()
  }

  public func delete() async throws {
    self.builder.schema.action = .delete
    try await self.database.execute(schema: self.builder.schema).get()
  }

}

Describe alternatives you've considered

  1. Maintain the status quo in which there is flexibility for using string literals and/or porting your own type to FieldKey or statically extending FieldKey.

  2. Improve documentation to demonstrate and or suggest statically extending FieldKey.

  3. Some other splendid, yet heretofore unimagined solution..

0xTim commented 7 months ago

Thank you for this very detailed write up!

The pattern I've settled around was your second option, as described here https://www.kodeco.com/23848990-database-migrations-with-vapor/page/2?page=2#toc-anchor-008

I do tend to suggest people follow that as a way of name-spacing and date-spacing their FieldKeys. We should definitely have something in the docs for this as it comes up often.

This also gives us some things to take into the design of Fluent 5

nastynate13 commented 7 months ago

Thanks for taking the time to read through! This is helpful. Somewhat related, I also wanted to quickly suggest sugar for Enum that leverages CaseIterable.

Since the current requirement for database backed Enums is only types conforming to RawRepresentable where RawValue is String, why not encapsulate this in a protocol?

protocol DatabaseEnum: CaseIterable, RawRepresentable where Self.RawValue == String {
  static var fieldKey: String { get }
}
extension FooEnum: DatabaseEnum {
  static var fieldKey: String { "fooEnum_type" }
}

// It would be nice to be able to write something like this
 let fooEnumType = try await database.enum(FooEnum.self)
  .allCases()
  .create()

Implementation below:

protocol DatabaseEnum: CaseIterable, RawRepresentable where Self.RawValue == String {
  static var fieldKey: String { get }
}
extension Database {
  func `enum`<T: DatabaseEnum>(_ type: T.Type) -> CaseBuildingEnumBuilder<T> {
    .init(type, builder: self.enum(type.fieldKey))
  }
}
final class CaseBuildingEnumBuilder<DataType: DatabaseEnum> {
  let dataType: DataType.Type
  let builder: EnumBuilder
  init(_ dataType: DataType.Type, builder: EnumBuilder) {
    self.builder = builder
    self.dataType = dataType.self
  }
  func allCases() -> EnumBuilder {
    for value in dataType.allCases {
      _ = builder.case(value.rawValue)
    }
    return builder
  }
}

Without this (or something similar), defining the initial migration for the enum fields manually can be a little tedious an error prone, esp. for enum heavy projects that would like to take advantage of database backed enums.

0xTim commented 7 months ago

I think the main issue here is when you add a new case to an enum in a separate migration different systems can get out of sync, so a clean DB would fail when it tries to add the new case a second time

nastynate13 commented 7 months ago

Aha.. yes, understood!

gwynne commented 1 week ago

I would also add that doing more with FieldKeys is out of scope for Fluent 4. I don't know yet whether Fluent 5 will still have a FieldKey concept, but my current sense of it is that it might not. I'll leave this issue open as a feature request for Fluent 5 for now.