Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve FieldKey ergonomics #596

Open
nastynate13 opened this issue Feb 17, 2024 · 4 comments
Open

Improve FieldKey ergonomics #596

nastynate13 opened this issue Feb 17, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@nastynate13
Copy link

nastynate13 commented Feb 17, 2024

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..

@nastynate13 nastynate13 added the enhancement New feature or request label Feb 17, 2024
@0xTim
Copy link
Member

0xTim commented Mar 25, 2024

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
Copy link
Author

nastynate13 commented Mar 26, 2024

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
Copy link
Member

0xTim commented Apr 8, 2024

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
Copy link
Author

Aha.. yes, understood!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants