tanner0101 commented 5 years ago

FluentKit was created as a proof of concept for changes to how Models could work in Fluent 4. Since the proof of concept was a success, I want to open this issue as an early discussion into the pros / cons of the proposed changes.

The main change here is in how Models are defined. Models are no longer Codable types, but rather wrappers around a Storage type (note, models will still conform to Codable). This allows for Fluent to get and set model values dynamically.

I believe making models dynamic is critical to building out a lot of Fluent's highly requested features going forward. A lot of the features made possible by this change would hugely simplify actually using Fluent.

However, the migration for Fluent 3 users will be a burden. Let's use this thread to discuss whether we think the benefits of this change are worth the trade-offs.

To help people understand this change, here are some examples of Fluent 4 vs. Fluent 3. Note that the Fluent 4 APIs are still in proof of concept stage and could change.

Model definition

Fluent 4:

Model properties are now defined and accessed statically. Key paths to these property definitions are used to get and set properties.

final class Planet: Model {
    struct Properties: ModelProperties {
        let id = Field<Int>("id")
        let name = Field<String>("name")
        let galaxy = Parent<Galaxy>(id: Field("galaxyID"))
    }
    static let properties = Properties()
    var storage: Storage
    init(storage: Storage) {
        self.storage = storage
    }
    convenience init(name: String, galaxy: Galaxy) {
        self.set(\.name, to: name)
        self.set(\.galaxy, to: galaxy)
    }
}

let planet: Planet // pulled from DB
let name = try planet.get(\.name)

Fluent 3:

Models are Codable types.

final class Planet: Model {
    typealias Database = FooDatabase
    typealias ID = Int
    static let idKey: IDKey = \.id
    var id: Int?
    var name: String
    var galaxyID: Galaxy.ID
    var galaxy: Parent<Planet, Galaxy> {
        return parent(\.galaxyID)
    }
    init(name: String, galaxy: Galaxy) {
        self.name = name
        self.galaxyID = galaxy.id
    }
}

let planet: Planet // pulled from DB
let name = planet.name

Eager loading:

Eager loading means loading in related models in a performant way--usually relations that should be embedded in the root model. This is usually done when returning show or index routes in an application.

Expected json:

[
    {"id":1, "name": "Mercury", "galaxy": {"id": 2, "name": "Milky Way"}},
    {"id":2, "name": "Venus", "galaxy": {"id": 2, "name": "Milky Way"}},
    {"id":9, "name": "PA-99-N2", "galaxy": {"id": 1, "name": "Andromeda"}}
]

Fluent 4:

Use the new with() method for eager loading.

func index(_ req: HTTPRequest) -> EventLoopFuture<[Galaxy]> {
    return self.db.query(Galaxy.self).with(\.planets).all()
}

Fluent 3:

Use join + alsoDecode + a custom model

struct GalaxyWithPlanet {
    var id: Int
    var name: String
    var planet: Planet
}
func index(_ req: HTTPRequest) -> EventLoopFuture<[GalaxyWithPlanet]> {
    return self.db.query(Galaxy.self).join(\Planet.galaxyID, to: \.id).alsoDecode(Planet.self).all().map { res in
        return try res.map { (galaxy, planet) in
            return try GalaxyPlanet(id: galaxy.requireID(), name: galaxy.name, planet: planet)
        }
    }
}

Custom field data types

This example shows how to specify a custom data type for a field. In this case, using MySQL varchar(64).

Fluent 4:

Declare the desired data type in the property definition.

// fluent 4
final class Planet: Model {
    struct Properties: ModelProperties {
        let id = Field<Int>("id")
        let name = Field<String>("name", .mysql(.varchar(64)))
        let galaxy = Parent<Galaxy>(id: Field("galaxyID"))
    }
    static let properties = Properties()
    var storage: Storage
    init(storage: Storage) {
        self.storage = storage
    }
}

Fluent 3:

Use a custom migration.

final class Planet: Model {
    typealias Database = FooDatabase
    typealias ID = Int
    static let idKey: IDKey = \.id
    var id: Int?
    var name: String
    var galaxyID: Galaxy.ID
    var galaxy: Parent<Planet, Galaxy> {
        return parent(\.galaxyID)
    }
    init(name: String, galaxy: Galaxy) {
        self.name = name
        self.galaxyID = galaxy.id
    }
}
struct CreatePlanet: Migration {
    static func prepare(on conn: MySQLConnection) -> Future<Void> {
        return MySQLDatabase.create(Galaxy.self, on: conn) { builder in
            builder.field(for: \.id, isIdentifier: true)
            builder.field(for: \.name, .varchar(64))
            builder.field(for: \.galaxyID)
        }
    }
}

Partial updates:

A partial update means only updating a subset of a model's field at a given time, instead of replacing the entire model.

Fluent 4:

Dynamic models do partial updates by default.

let planet: Planet // pulled from DB
planet.set(\.name, to: newName)
// UPDATE planets SET name = ?
planet.save(on: self.db)

Fluent 3:

This is not possible. The entire model must be updated any time a property is changed.

let planet: Planet // pulled from DB
planet.name = newName
// UPDATE planets SET id = ?, name = ?, galaxyID = ?
planet.save(on: self.db)

Lazy Decoding

This change also helps performance quite a bit since fields are decoded lazily.

// fluent 4
let planet: Planet // pulled from DB
// fetches and decodes "name" from the db row
// this throws if there is no field called "name"
// or if it is an incompatible data type
let name = try planet.get(\.name)

That means CPU isn't wasted decoding things that are never used, which very often can be most fields on the model. This has a nice side effect of letting you do partial selects, too:

// fluent 4
let planets = self.db.query(Planet.self).keys(\.name).all()
for planet in planets {
    // works
    let name = try planet.get(\.name)
    // throws since ID was not selected
    let id = try planet.get(\.id)
}

jdmcd commented 5 years ago

I'm largely in favor of this, but I'm having a hard time getting over the .get and .set syntax. It feels so... gross. Is there a way to move those actions into the getter and setter of the individual properties?

twof commented 5 years ago

Where is the DB specified in the Fluent 4 model? Or do we no longer need to do that?

tanner0101 commented 5 years ago

Is there a way to move those actions into the getter and setter of the individual properties?

@mcdappdev computed property getters can't throw in Swift, so no :(

FWIW, this is how CoreData works, so if anything this change should actually make Fluent more familiar to iOS engineers.

import CoreData

let person: NSManagedObject // from db
person.value(forKeyPath: "name") as? String
person.setValue(name, forKeyPath: "name")

Where is the DB specified in the Fluent 4 model? Or do we no longer need to do that?

@twof Database no longer has associated types so you can use it abstractly now. For example:

final class GalaxyController {
    let db: Database
    func index(_ req: HTTPRequest) -> EventLoopFuture<[Galaxy]> {
        return self.db.query(Galaxy.self).all()
    }
}

When initializing this controller, you can pass in any type that conforms to Database, whether that be a connection pool, single connection, postgres, mysql, sqlite, mongo, or even a dummy / mock database. No need to be generic anymore, so Model doesn't need the associated type.

twof commented 5 years ago

"When initializing this controller, you can pass in any type that conforms to Database, whether that be a connection pool, single connection, postgres, mysql, sqlite, mongo, or even a dummy / mock database. No need to be generic anymore, so Model doesn't need the associated type."

Wow that's really nice. Swapping databases doesn't come up super often, but if it does that ought to make it mad easy.

twof commented 5 years ago

Moving my comment over from the other thread.

IMO it would be worth it to support both model styles and let folks upgrade their models to the Fluent 4 style as they decide the tradeoffs become worth it.

The other thing you lose is the ability to share models between the frontend and backend out of the gate, but by the time users decide to upgrade a model, they've probably already created a public model and shared that instead, so that shouldn't be a problem if you're supporting both styles.

Any idea how much work it would be to have both styles exist at the same time?

jdmcd commented 5 years ago

@tanner0101 that’s sad :( is there any way to make a variant of that method that doesn’t throw? And what about creating a new object? Do you have to set each value independently? Or can that still be wrapped up in an init?

tanner0101 commented 5 years ago

IMO it would be worth it to support both model styles and let folks upgrade their models to the Fluent 4 style as they decide the tradeoffs become worth it.

@twof I tried a couple approaches for this, but haven't found anything that works yet. :\ The wrench in the works seems to be that Model gets its Codable conformance dynamically now (not compiler synthesized, but synthesized by Fluent introspecting the model properties and doing the encoding / decoding). So the Codable struct / classes clash with that.

The other thing you lose is the ability to share models between the frontend and backend out of the gate

Yeah that is another unfortunate thing. Especially since it affects "getting started" use cases.

You can still have Codable structs that you share between your iOS app and your server, it's just that you will need to have a separate model and translate between them. Like is common in Fluent 3 anyway for cases where you want to change the data structure.

struct GalaxyJSON: Codable {
    var id: Int?
    var name: String
    var planets: [PlanetJSON]
}

let galaxy: Galaxy // fetched from DB
let json = try galaxy.decode(GalaxyJSON.self)
// or
let json = try GalaxyJSON(
    id: galaxy.get(\.id), 
    name: galaxy.get(\.name), 
    planets: galaxy.get(\.planets).map { ... }
)

I think that almost all real world apps will want this anyway because you really don't want to strictly tie your DB structure to your public JSON API in production use cases. But yeah, it's a trade-off for sure.

And what about creating a new object? Do you have to set each value independently? Or can that still be wrapped up in an init?

@mcdappdev you can create a convenience init if you want, like I did in the example:

final class Planet: Model {
    // ...
    convenience init(name: String, galaxy: Galaxy) {
        self.set(\.name, to: name)
        self.set(\.galaxy, to: galaxy)
    }
}

otherwise, you will need to create an empty object and set them manually:

let planet = Planet()
try planet.set(\.name, to: "Earth")
try planet.set(\.galaxy.id, to: 1)
planet.save(on: self.db)

tanner0101 commented 5 years ago

@twof on a positive note, one of the pros for "getting started" use cases is that you can now easily hide fields from being encoded. A common use case for this being passwordHash on a user model:

final class User: Model {
    struct Properties: ModelProperties {
        let id = Field<Int>("id")
        let name = Field<String>("name")
        let passwordHash = Field<String>("passwordHash", visibility: .hidden)
    }
    ...
}

In this case, passwordHash would still be sent to/from the DB, but when encoded to JSON, the user would look like:

{ "id": 1, "name": "Vapor User" }

0xTim commented 5 years ago

So initial thoughts:

The most important question, in terms of raw database terms, how does a table in Fluent 3 look compared to Fluent 4 - i.e. will upgrading require custom scripts when upgrading?

Aside from that, my main overriding concern is models have become massively more complex than they were. I also agree with @mcdappdev in that they just feel 'unSwifty' (and comparing to CoreData is not a comparison that should be made to justify it! 😅 ) I think Fluent (and Vapor) needs to decide at this point if it is a Swift-like server framework, or a high performance server framework. If it's aiming for a high performance framework then sure, make it as complicated as possible but I don't feel like that has been Vapor's sole goal.

To reiterate what I said on Discord - one of the big selling points of Vapor 3 was having hugely simplified code and models, where models could be only a few lines of code, especially with the different helpers to automatically set the database type and ID. If there's any way at all for the framework to hide the complexities to the end user, then it absolutely should. Even if that includes horrible hacks to enable Reflection.

Overall, it just seems like everything is more complicated. From the looks of it, doing something like return req.content.decode(Planet.self).save(on: req) doesn't look possible any more since you'd need to set each parameter of the model. It all seems like a bit of a step backwards for what I see as the majority of use cases (which could well be wrong, but from my experience is just basic CRUD APIs) to allow a small minority of use cases (things like partial retrieves and eager loading) prosper. I absolutely agree that Fluent should provide an API to do these, but I'm not sure it should be at the expense of the ease of use we've come to expect from Vapor. I really don't want to come across as negative here! I'm just concerned about the effect this may have on newcomers and put people off using the framework.

I think ideally there'd be a way to support both use cases, with something like a BasicModel that allows you to use properties like normal but doesn't allow any of the advanced features.

I'll add more thoughts as they come to me!

tanner0101 commented 5 years ago

The most important question, in terms of raw database terms, how does a table in Fluent 3 look compared to Fluent 4 - i.e. will upgrading require custom scripts when upgrading?

This change in how models are declared shouldn't have any effect on DB structure.

Aside from that, my main overriding concern is models have become massively more complex than they were.

Could you elaborate more on which parts are more complex? The way I see it there are trade-offs, but ultimately things become less complex.

Increased complexity (cons):

Getting / setting individual properties is more verbose: planet.name becomes planet.get(\.name).
Model definitions will need to change, which is a burden. But they aren't any less concise. In my example above, the model is only 1 line longer.

Decreased complexity (pros):

Many situations that required a custom migration can now be done with one small change.
Many situations that required manual joins and decoding can now be done automatically.
Many situations that required one-off structs can now re-use the model type:
- Public / private models with different structures.
- Models w/ or w/o relations loaded in
Declaring nested / custom types no longer requires complex ReflectionDecodable conformance. Things "just work".
Migrations that change / remove model properties can now be done without needing a copy of the old model structure:

let planet: Planet
// assume mass has been removed from the model, but we 
// still need to interface with it to perform this migration:
let mass = try planet.storage.get("mass", as: Double.self)
print(mass) // Double

Improved performance (pros):

Un-used properties are no longer decoded.
Only changed properties are sent over the wire during model saves.

From the looks of it, doing something like return req.content.decode(Planet.self).save(on: req) doesn't look possible any more since you'd need to set each parameter of the model.

Model still conforms to Codable. You can still decode instances of Models from things like JSON. (And encode them, too)

jdmcd commented 5 years ago

Regarding the last point there, is it still possible to chain on save() to that decoding call? Because that’s one of my concerns as well.

twof commented 5 years ago

Model definitions will need to change, which is a burden. But they aren't any less concise. In my example above, the model is only 1 line longer.

The issue is not the length. The issue is that implementation details are bleeding into userland, and models are significantly less abstract. Previously users could write plain old swift objects just like they were used to, conform to a few protocols, and trust everything would work. There was no need to think about implementation because users weren't being exposed to it.

Now if I was going to be teaching someone Fluent, I've got to explain why things are different than what they're used to in 3 or 4 different regards. The mental models people have to establish before they can use models in a comfortable way are more complex.

I want to be clear that I that I believe this level of flexibility and performance (the new Fluent 4 level) is the correct direction to go in. I'm fully aware of the pros and cons and I am on board to pursue this direction. However, a huge reason many folks choose Vapor over other frameworks is due to Swiftyness and up front ease of use. You'd be abandoning that use case if fluent-kit replaced fluent and with it any new users who would have adopted Vapor had it supported that use case.

This is something that's important to me (idk about others). I don't know the internals of Fluent or Fluent-Kit well, so I'm going to go learn them and figure out what it would take to run both styles of Model in parallel.

0xTim commented 5 years ago

@mcdappdev I've been told that chaining a save() onto a decode() will work fine.

I think @twof hits the nail on the head, it's the lack of Swift types and having to learn new ways of accessing properties on models that seem a) confusing and b) very unSwifty for anyone looking at it the first time.

Looking at it, it doesn't look like there can be any helpers (such as PostgreSQLModel) that we had in Fluent 3 to cut down on all the boilerplate

martinlasek commented 5 years ago

To put my two cents in - it took a few minutes to read all comments 😊

My first impression

"ohw bummer.. it's becoming more complex". Especially with a "beginner mind". I think @twof has pointed out the "why" very good.

When speaking of simplicity I think it's less about code length and more what you know intuitively. In Fluent 3 you could just define your properties as you would do with normal structs and classes - the swift default way. Nothing new to learn.

New vs. Old

With Fluent 4 you now have to learn that defining properties was taken on a new level, more abstract level and not only would you have to learn that but you also have to know about "more advanced" coding-concepts like "Generics" and "Keypath".

Issue

With Vapor 3 the beginner-friendliness decreased quite a bit with the introduction of futures. For stuff that you would want to do after 30 minutes using vapor for the first time in your life - querying a database for a model and return it as JSON - it required you to face your first challenge: Futures. Even more if server-side is the first time you are actually touching swift.

In my opinion that beginner-friendliness would decrease even more now with the new way of how you define models. I mean that's something you would want to do after 30 minutes using Vapor for the first time in your life, too, right?

In conclusion:

To me it feels like "starting out with vapor" would overwhelm a lot of developer with having to wrap their heads around a lot of new (non-straight forward) concepts that at times require more advanced knowledge about swift development. Which leads to being un-swifty when thinking of that swift aims to appeal to beginners as you can build things pretty quick but can become more advanced later in the learning-process (not having to at the beginning).

I usually don't like to declare something as an issue without a solution to it. However I can't come up with something that would reduce the complexity "at the beginning" while introducing simplicity "later on".

That's how I feel about the new Model. It became more complex in the beginning to become more simple later on. Thinking of how peeps actually "learn" I think having it rather "simple in the beginning" and more complex "later on" is more bearable - less frustrating thus reduces the likeliness of giving up.

PS

I have the feeling you (@tanner0101) have put in a lot of thoughts in that new concept to solve a lot of current issues/requested features. And I really don't want to sound negative either - like @0xTim said - the question really is what direction does vapor want to go. Does maybe solving the current issue introduces another one (of same size or bigger) ?

✌🏻😊

groue commented 5 years ago

Hello,

I'm chiming in because I expect to use Vapor in the near future, and because I am interested in "Swifty" ORM apis myself, as the main author of GRDB, the SQLite toolkit for applications.

I'm wondering if the changes described in the "Eager loading" and "Lazy Decoding" chapters are for the best. Something has been lost: the ability to catch errors early when the decoded model does not match the query.

In Fluent 3, eager loading would use a "custom model". Thanks to this custom model, the user is confident about the structure of the loaded values. Once loaded without error, the values can be consumed with no thinking at all: Swift provides static guarantees that the expected data is available. You can pass custom models around without try/catch management.

In Fluent 3, there was no lazy decoding, which means (sheer deduction, forgive me if I'm wrong) that one had to define a custom model for partial selections which do not load all columns. (Maybe optional properties were an option too). But again, once loaded without error, this custom model could also be passed around without error checking, because it was guaranteed to contain the fetched columns, and to not contain the columns that were not fetched.

Early failure is no longer possible with Fluent 4. Because new models do not use Swift static checks, no function can be sure a model contains the needed information. All functions that read inside a model may throw. Fixing bugs has turned into a test coverage game.

Maybe a recommended practice, to avoid the proliferation of throwing methods, will be to "map" Fluent models into custom structs immediately after the fetch. This would restore both early errors and static checks. In such a setup, Fluent models would be pushed in a "DAO" layer, not really designed for application consumption. And this would surely raise the amount of boilerplate code.

tanner0101 commented 5 years ago

I think making Vapor easy to pick up and learn is important, yes. But I don't think that should come at the expense of usability for real, production-size apps. Fluent 3's Codable models are great for getting started, but in my experience they really fall apart and become difficult to use once you start creating big apps.

So when you ask "what direction does Vapor want to take", I definitely don't want Vapor to just be useful for toy / getting started projects. I want it to be powerful enough to tackle any project.

At the end of the day, the core team's motto has always been: Build something that we would want to use. That is at the heart of this change to how models are defined. While Fluent 3's API seemed great and simple at first, once we started applying it to large cases the problems began to show and it started to feel clunky and hard to use.

I feel like this is a critical piece that is being missed in this conversation, so I will repeat it again:

This change is meant to greatly simplify using Fluent overall (at the expense of a slightly more complex Model definition)

For example, again, doing simple eager loading in Fluent 3:

struct GalaxyWithPlanet {
    var id: Int
    var name: String
    var planet: Planet
}
let galaxies = self.db.query(Galaxy.self)
    .join(\Planet.galaxyID, to: \.id)
    .alsoDecode(Planet.self)
    .all().map 
{ res in
    return try res.map { (galaxy, planet) in
        return try GalaxyPlanet(
            id: galaxy.requireID(), 
            name: galaxy.name, 
            planet: planet
        )
    }
}

IMO this is unacceptably complex for something that you might need to do even 5-6 times, nested, for large models / routes.

W/ proposed Fluent 4, it's just one line of code:

let galaxies = db.query(Galaxy.self).with(\.planets).all()

We are making a tradeoff of the Model being more complex, but getting huge simplicity wins where we actual use the model--which tends to make up a lot more of the code than just the model definition. And again, this isn't just about eager loading. There are tons of other examples (some of which I described above) that are radically simplified w/ Fluent 4's models.

Furthermore, compare to how other ORMs work:

Laravel (Eloquent):

$galaxies = App\Galaxy::with(['planets'])->get();

SQLAlchemy:

galaxies = session.query(Galaxy).options(joinedload(Galaxy.planets)).all()

Django:

galaxies = Galaxy.objects.prefetch_related('planets').all()

Rails (Active Record):

galaxies = Galaxy.includes(:planets).all

Express.js (Sequelize):

Galaxy.findAll({ include: [{ model: Planet, as: 'Planets' }] }).then(galaxies => { })

There's a clear pattern here in how other, popular web frameworks have solved this problem. And all of them are enabled by having a dynamic model type.

Hopefully this and the previous comments address the broad argument that this change "makes things more complex". I think going forward it would be more productive to discuss particular aspects / concerns about the proposed Model API so that they can be addressed one by one.

For example, there could be ways to make Fluent 4's model definition more closely resemble Fluent 3, at the cost of some additional internal complexity / performance. For example, maybe something like this (no nested Properties type) would be an improvement:

final class Planet: Model {
    let id = Field<Int>("id")
    let name = Field<String>("name")
    let galaxy = Parent<Galaxy>(id: Field("galaxyID"))

    var storage: Storage
    init(storage: Storage) {
        self.storage = storage
    }

    convenience init(name: String, galaxy: Galaxy) {
        self.set(\.name, to: name)
        self.set(\.galaxy, to: galaxy)
    }
}

In Fluent 3, there was no lazy decoding, which means (sheer deduction, forgive me if I'm wrong) that one had to define a custom model for partial selections which do not load all columns. (Maybe optional properties were an option too). But again, once loaded without error, this custom model could also be passed around without error checking, because it was guaranteed to contain the fetched columns, and to not contain the columns that were not fetched.

You can still do something like this in Fluent 4. The Model offers a method for decoding itself into a keyed container (struct).

struct GalaxyJSON: Codable {
    var id: Int
    var name: String
}

let galaxies = try db.query(Galaxy.self).all().wait()
print(galaxies) // [Galaxy]
let json = try galaxies.map { try $0.decode(GalaxyJSON.self) }
print(json) // [GalaxyJSON]

However, I would recommend against doing this Codable mapping unless you know for sure that you will use all of the data in the struct. A couple of reasons:

Decoding keyed containers is expensive. (If you encode the Model directly to a format like JSON, you encode the DB row directly without ever needing to do keyed decoding.)
Decoding each property lazily as needed saves memory and CPU time. For example, if a function using Galaxy is refactored at some point to no longer use name, that key may no longer need to be decoded at all, increasing the performance and reducing memory usage for the route in question.

Maybe a recommended practice, to avoid the proliferation of throwing methods, will be to "map" Fluent models into custom structs immediately after the fetch.

You need an extra Codable struct here, but that's really no different than Fluent 3 where you already need to map to several, separate Codable structs to do most things.

Fixing bugs has turned into a test coverage game.

I'm not sure I understand this part. If your Model.Properties do not match your database schema, you will get an error just like if your Model's codable properties do not match in Fluent 3.

jdmcd commented 5 years ago

+1 for removing the nested Properties. I think that alone removes a lot of cognitive strain for beginners while keeping the benefit of the stated goals.

May I suggest, once again, that we should be looking to find a way to make get() non-throwing - or perhaps offer a version that doesn't throw. I think a ton of the concerns around readability/usability can be fixed if we can wrap .get() and .set() inside of a property's getter and setter.

Before:

post.name.set("Jimmy")
try post.name.get() // Jimmy

After:

// Post.swift
final class Post: Model {
    let id = Field<Int>("id")
    let name = Field<String>("name") {
        get {
            return name.get()
        }
        set {
            name.set(newValue) // I forget the exact syntax here
        }
    }

    // rest of the model
}

post.name = "Jimmy"
post.name // Jimmy

tanner0101 commented 5 years ago

You could do something like this:

final class Planet: Model {
    struct Properties: ModelProperties {
        let id = Field<Int>("id")
        let name = Field<String>("name")
        let galaxy = Parent<Galaxy>(id: Field("galaxyID"))
    }

    static let properties = Properties()

    var id: String {
        get { return try! self.get(\. id) }
        set { self.set(\. id, to: newValue) }
    }

    var name: String {
        get { return try! self.get(\.name) }
        set { self.set(\.name, to: newValue) }
    }

    var galaxy: String {
        get { return try! self.get(\.galaxy) }
        set { self.set(\. galaxy, to: newValue) }
    }

    var storage: Storage
    init(storage: Storage) {
        self.storage = storage
    }
    convenience init(name: String, galaxy: Galaxy) {
        self.name = name
        self.galaxy = galaxy
    }
}

let planet: Planet // pulled from DB
print(planet.name)

I'm not a huge fan of that since we have to throw away potential missing value / decoding errors though. It also increases the boilerplate for models a lot.

However, there are some interesting things happening with Swift like: Pitch: Key-Path Member Lookup. If that pitch were to be accepted, I think it could be possible for us to simulate properties (in a type-safe way) that ultimately redirect to the function calls get / set.

I think, generally, we should have faith that Swift will improve and not try to design Vapor's APIs around what is the most concise / easy today. For example, we know that Swift will get async / await sometime soon, which is why we embraced Futures as a temporary solution, even though they are relatively verbose and not easy to use. The win here is that Vapor's architecture, and all the packages designed to work w/ Vapor, will eventually have all the incredible benefits of non-blocking with a beautiful synchronous API. All good things come with time.

I think the same thing applies with models. It's likely that Swift will eventually get better reflection, attributes, dynamism, etc. When Swift does get those things, we can have all the benefits of choosing the most powerful solution and get the best API. And we as part of the server-side Swift community can help pitch, propose, and implement those features. Swift is a very young language that has only really been used for application /client programming up until this point. It makes sense that some things don't translate well for server use cases yet--and that's OK. Swift's mission is to become better at those things.

groue commented 5 years ago

Fixing bugs has turned into a test coverage game.

I'm not sure I understand this part. If your Model.Properties do not match your database schema, you will get an error just like if your Model's codable properties do not match in Fluent 3.

Yes, but you will get a late error, not an early error. The late error happens only when you attempt to access a missing information, long after the data has been fetched. Some code paths will not express the error, some bugs will become latent.

This is the direct consequence of the new dynamic api. What is no longer checked for free by the compiler is just no longer free. Experience (from frameworks written in dynamic languages) shows that the cost is new tests.

In case you ask: what the compiler no longer provides is the guaranteed access to a property: planet.name doesn't throw, when planet.get(\.name) can.

tanner0101 commented 5 years ago

Some code paths will not express the error, some bugs will become latent.

Ah okay, I see what you mean now. Yeah that seems like a fundamental downside to lazy decoding. A good point worth adding to the downsides of this change, thank you. 👍

groue commented 5 years ago

Furthermore, compare to how other ORMs work:

[...] Rails (Active Record):
galaxies = Galaxy.includes(:planets).all
[...]

I'm not here to advertise vaporware, but I do certainly hope that GRDB will soon let people write:

struct GalaxyInfo: Decodable {
    let galaxy: Galaxy
    let planets: [Planet]
}

// Single point of failure
let infos: [GalaxyInfo] = try Galaxy
    .including(all: Galaxy.planets) // Galaxy.planets is a "HasMany" association
    .asRequest(of: GalaxyInfo.self)
    .fetchAll(db)

// No need to catch errors from now on:
use(infos)

And there is nothing dynamic here.

This is not totally vaporware, because support for "to-one" associations is already implemented and working well:

// Currently works
struct PlanetInfo: Decodable {
    let planet: Planet
    let galaxy: Galaxy
}

let infos: [PlanetInfo] = try Planet
    .including(required: Planet.galaxy) // Planet.galaxy is a to-one "BelongsTo" association
    .asRequest(of: PlanetInfo.self)
    .fetchAll(db)

use(infos)

My point is that dynamism is maybe not as required as it may seem. And as you wrote above, Swift does improve over time. Rust's Diesel is very interesting, too.

tanner0101 commented 5 years ago

Here's a potential idea following on the remove Model.Properties idea, but inverting it. Instead of having users declare their own instance of the model class each time, we could have users declare only the model properties. Then Fluent could re-use a generic model definition. Naming aside, this could look like:

struct Planet: Model {
    static let default = Planet()
    let id = Field<Int>("id")
    let name = Field<String>("name")
    let galaxy = Parent<Galaxy>(id: Field("galaxyID"))
}

Instances of planet would become:

let planet: Instance<Planet> // pulled from DB
let name = try planet.get(\.name)

Querying would look the same, but return a different type:

final class GalaxyController {
    let db: Database
    func index(_ req: HTTPRequest) -> EventLoopFuture<[Galaxy.Instance]> {
        return self.db.query(Galaxy.self).with(\planets).all()
    }
}

Alternate ideas for naming Instance:

Ref / Reference
Object
Entity
Row

tanner0101 commented 5 years ago

@groue how do you apply filters to that data? For example, can you do something like:

// Currently works
struct PlanetInfo: Decodable {
    let planet: Planet
    let galaxy: Galaxy
}

let infos: [PlanetInfo] = try Planet
    .including(required: Planet.galaxy) // Planet.galaxy is a to-one "BelongsTo" association
    .asRequest(of: PlanetInfo.self)
    .filter(\.name == "Earth")
    .fetchAll(db)

use(infos)

If so, how do you get the string "name" from \.name?

jdmcd commented 5 years ago

@tanner0101 I could get behind that. How would you save a new object with that model?

tanner0101 commented 5 years ago

@mcdappdev something like this would work:

let planet = Planet.new() // Instance<Planet>
planet.set(\.name, to: "Earth")
planet.set(\.galaxy.id, to: 42)
planet.save(on: db)

We might be able to get a more convenient init working, though. Maybe something like:

let planet = Planet([\.name: "Earth", \.galaxy.id: 42])
planet.save(on: db)

I would have to try a bit to see if I could get that one working, though.

jdmcd commented 5 years ago

Nevermind. I'm now against it :)

tanner0101 commented 5 years ago

Haha. You could also add a convenience init if you wanted:

extension Instance where Model == Planet {
    convenience init(name: String, galaxy: Galaxy) {
        self.init()
        self.set(\.name, to: name)
        self.set(\.galaxy, to: galaxy)
    }
}

let planet = Planet.Instance(name: "Earth", galaxy: ...)

I'm not really in love with that either, though.

jdmcd commented 5 years ago

Oh ok, that's slightly better. I guess one of my main worries is that without a proper init it'll be really easy to add a new property and then forget to set it somewhere. So if we can support that that'll alleviate that concern.

groue commented 5 years ago

@groue how do you apply filters to that data? For example, can you do something like:
let infos: [PlanetInfo] = try Planet
    .including(required: Planet.galaxy) // Planet.galaxy is a to-one "BelongsTo" association
    .asRequest(of: PlanetInfo.self)
    .filter(\.name == "Earth")
    .fetchAll(db)
If so, how do you get the string "name" from \.name?

GRDB doesn't use key paths, and prefers strings or coding keys instead. But this is not your question. As you have correctly analysed, the .asRequest(of: PlanetInfo.self) part has to go last, so that the Planet type is not lost (unless you opt in for strings, in which case the order does not matter).

tanner0101 commented 5 years ago

@groue what would the definition of Planet look like in this case?

groue commented 5 years ago

what would the definition of Planet look like in this case?

In its shortest form, for read/write access:

// A regular struct...
struct Planet {
    var id: Int64
    var name: String
    var galaxyID: Int64
}

// ... now with database support
extension Planet: FetchableRecord, PersistableRecord, Codable {
    static let galaxy = belongsTo(Galaxy.self) 
}

Have a look at GRDB, one day, it is a cool (client-focused) library! But I'm sure Fluent is pretty cool, too. I'm not here to advertise my work. Just to spot a few questions in the original post.

tanner0101 commented 5 years ago

@groue thanks, I will definitely take a look. It's nice to have a Swift example besides CoreData to reference (even if it is client-focused). I do like this model, but the one thing I don't think Fluent could live without is key paths. Those are crucial to our query building API:

db.query(Planet.self).filter(\.mass > 3).with(\.galaxy).sort(by: \.name, .descending).all()

Maybe with Swift 5's ABI being stable there is a way Fluent could have the best of both worlds. I will do some more digging...

martinlasek commented 5 years ago

Removing `Properties`

I'm all in on to "closely resemble Fluent 3, at the cost of some additional internal complexity / performance." and removing the nested properties 🙌

Updated opinion

I didn't address it previously but I do like the reduction of the code-clutter when crafting more complex queries. 😊

Not having the properties struct had a big impact on my thoughts about the new model. It starts growing on me.

PS

Out of curiousity @tanner0101 - defining a parent relation does not need a type for the id?

let galaxy = Parent<Galaxy>(id: Field("galaxyID")) // your example

vs

let galaxy = Parent<Galaxy>(id: Field<Int>("galaxyID")) // my question :)

jdmcd commented 5 years ago

@MartinLasek Yeah just an idea though, would consider it at the project level but definitely not at the Fluent level. I guess it's a tradeoff between complexity (I use that word lightly as .set and .get aren't exactly complex) at the model level or the access level

twof commented 5 years ago

Given that this change is targeted at folks using Vapor on large, serious projects, it'd be great to get feedback from more folks at Nodes. @calebkleveter would be good too.

tanner0101 commented 5 years ago

Out of curiousity @tanner0101 - defining a parent relation does not need a type for the id?

@MartinLasek You can write Parent<Galaxy>(id: Field<Int>("galaxyID")), too. Both work. Swift is able to infer <Int> because it knows Galaxy.ID == Int.

Given that this change is targeted at folks using Vapor on large, serious projects, it'd be great to get feedback

@twof I agree. I'd like to get people trying this out sooner than later to see how it holds up.

tanner0101 commented 5 years ago

I put up a proof of concept of the generic instance idea here: https://github.com/vapor/fluent-kit/pull/12

It was pretty easy to implement, and the diff is even less lines of code. Maybe this can be a good compromise?

guseducampos commented 5 years ago

I put my two cents:

From a beginner perspective (let's say someone who comes from iOS or from another backend framework Laravel, Flask, Rails,etc) I believe this new way how to define and use fluent's models it doesn't feel "natural". a lot of people would expect to make the mutations directly on the properties instead of use keypaths and write plains swift models without nested intermediate types for declare the properties to use.

But the benefits that this new api give to us is great and I think the tradeoff of this new level of complexity worth it, I know that with this new design we lose a little bit of the type safety from fluent 3 but I think is better to use the best design based on what the language offer instead of trying to force to use something that at the end of the day just will cause more problem that what is trying to solve. Hopefully one day swift will provide great reflections capabilities and helps to comeback an even better api like Fluent 3.

Saying this IMHO we should keep with this api

final class Planet: Model {
    struct Properties: ModelProperties {
        let id = Field<Int>("id")
        let name = Field<String>("name")
        let galaxy = Parent<Galaxy>(id: Field("galaxyID"))
    }
    static let properties = Properties()
    var storage: Storage
    init(storage: Storage) {
        self.storage = storage
    }
    convenience init(name: String, galaxy: Galaxy) {
        self.set(\.name, to: name)
        self.set(\.galaxy, to: galaxy)
    }
}

I would rather to keep my initializers inside of the type for what are intended for, instead of write it inside of a where extension from another type (like Instance example). Also I would rather use directly my types instead of have a kind of wrapper like Instance, I think this add more complexity than we actually need.

ScottRobbins commented 5 years ago

Do you have to specify the keys when querying? Will it default to not lazy-loading properties unless keys are specified?

tanner0101 commented 5 years ago

@ScottRobbins hmm not sure what you mean by that, can you elaborate?

ScottRobbins commented 5 years ago

You had an example like

// fluent 4
let planets = self.db.query(Planet.self).keys(\.name).all()
for planet in planets {
    // works
    let name = try planet.get(\.name)
    // throws since ID was not selected
    let id = try planet.get(\.id)
}

If I changed this to

// fluent 4
let planets = self.db.query(Planet.self).all() // no more .keys
for planet in planets {
    let name = try planet.get(\.name)
    let id = try planet.get(\.id)
}

Does that end up making 2 queries?

Edit: Realized half of my questions you answered right above where i was reading this example and I missed it.

tanner0101 commented 5 years ago

@ScottRobbins ah I see what you mean. Two things:

1: If you don't write .keys, then all of them will be selected by default. Note that this doesn't mean SELECT *, it means SELECT id, name, etc 2: Lazy decoding only means decoding data from the row (converting it from your DB's format to Swift types). If the column wasn't fetched in the query, then the decoding will fail with "no column named x found". It's actually impossible for us to do a query to get a single key lazily because the get method is synchronous. We can only access data that is already available.

tanner0101 commented 5 years ago

@guseducampos thanks for the comments. My replies inline:

But the benefits that this new api give to us is great and I think the tradeoff of this new level of complexity worth it, I know that with this new design we lose a little bit of the type safety from fluent 3 but I think is better to use the best design based on what the language offer instead of trying to force to use something that at the end of the day just will cause more problem that what is trying to solve. Hopefully one day swift will provide great reflections capabilities and helps to comeback an even better api like Fluent 3.

Yeah those are my thoughts exactly. I think Pitch: Key-Path Member Lookup specifically could make the syntax a lot more bearable, and that seems like it could be merged in the near future (if the proposal goes well). I've added a small note about how that could affect the API positively in #12.

Saying this IMHO we should keep with this api

I agree with your reasons here. But also, I think that using @keyPathMemberLookup will not be easy to add if we don't own the type. The best we could do with a protocol (maybe) is provide a default implementation of the subscript method, but people would still need to add the attribute to each of their models.

I would rather to keep my initializers inside of the type for what are intended for

An alternative is doing something like this:

struct Foo: Model {
    static let `default` = Foo()
    let id = Field<Int>("id")
    let bar = Field<String>("bar")
    let baz = Field<Int>("baz")
    static func new(bar: String, baz: Int) -> Row<Foo> {
        let new = self.new()
        new.set(\.bar, to: bar)
        new.set(\.baz, to: baz)
        return new
    }
}

I know it's not as nice as a direct init, but it works.

Also I would rather use directly my types instead of have a kind of wrapper like Instance, I think this add more complexity than we actually need.

12 has renamed the type from `Instance` -> `Row`. The added concision and specificity makes the change more bearable to me at least. Thoughts?

ScottRobbins commented 5 years ago

1: If you don't write .keys, then all of them will be selected by default. Note that this doesn't mean SELECT *, it means SELECT id, name, etc 2: Lazy decoding only means decoding data from the row (converting it from your DB's format to Swift types). If the column wasn't fetched in the query, then the decoding will fail with "no column named x found". It's actually impossible for us to do a query to get a single key lazily because the get method is synchronous. We can only access data that is already available.

That makes sense, thank you 👍

kevinrenskers commented 5 years ago

I was asked on Twitter to provide my feedback, even though I think it has all been said already in this thread :) My thoughts as an experienced Swift dev, but only just started using Vapor: I'd gladly have a slightly more complex model definition if using the models gets easier. Right now my biggest problem with Vapor, by far, is the lack of eager loading of relationships. Also it's currently too hard to specify things like indexes or unique fields, and non-public fields means having to have a separate public version of your model, and translate data from one to the other. This seems like it could solve all (or most) of those things. I am super excited to see these changes, even though yes planet.get(\.name) is not as nice as planet.name.

It's been mentioned that this suggested change would make Vapor harder to learn for beginners, but in my opinion as a Vapor beginner, I'd rather have to learn once how to write a model, but then actually using them would be a lot easier, compared to all the work I currently have to do to return a full model with all its nested relationships as JSON.

For example, this is what a Django model looks like:

class Book(models.Model):
    title = models.CharField(max_length=255)
    date_created = models.DateTimeField(auto_now_add=True)
    date_modified = models.DateTimeField(auto_now=True)
    owner = models.ForeignKey(User, related_name='books', on_delete=models.CASCADE)

Yes, not as concise as a Vapor 3 model with pure Strings and Ints and all that, but then I need to add a Migration extension to handle the relations on a DB level, the onDelete handling, etc. There is currently no way to describe model properties in an easy manner, to add extra behavior to them. This proposal would bring a lot of power to models I think. I'm all for it!

calebkleveter commented 5 years ago

I'm wondering if putting the storage in a Schema and then setting the Schema of a model might open some doors 🤔.

final class PlanetSchema: Schema {
    let id = Field<Int>("id", identifier: true)
    let name = Field<String>("name")
    let galaxy = Field<Galaxy>(field: Field("galaxyID"))

    var storage: Storage

    init(storage: Storage) {
        self.storage = storage
    }
}

final class Planet: Model {
    let schema: PlanetSchema

    // `Column<T>` typaliased to `Result<Error, T>`
    var id: Column<Int> {
        return self.schema.get(\.id)
    }

    var galaxy: Column<Galaxy> {
        return self.schema.get(\.galaxy)   
    }

    var name: Column<String> {
        get { return self.schema.get(\.name) }
        set { self.schema.set(\.name, to: newValue) }
    }

    init(schema: PlanetSchema) {
        self.schema = schema
    }
}

jdmcd commented 5 years ago

Is it possible to use keypaths inside of Field? So: Field<Int>(\.id, identifier: true)? Can it even be self-referential like that? I just struggle with the stringly typed API there, even if it is only in that one place.

Also, I've seen this syntax referred to a few times now: planet.get(\.name). Will that be available in addition to planet.name.get()? Because I think the latter is a lot more natural.

tanner0101 commented 5 years ago

For context, I've collected what a theoretical Planet model would look like in some other popular ORMs. Interestingly, there seems to be two main approaches.

1. Empty subclass + migration

One method is to use an empty sub-class of a model class. This method relies on defining a separate migration since there is no schema info on the model itself. Everything is looked up dynamically (no type safety of course).

ORMs: Eloquent, ActiveRecord

2. Schema declaration

The method other is to have the model be a schema declaration. Here you declare what the table should look like, which allows the framework to generate the migration on your behalf. I'm not sure what type is actually returned here when you query the model, that is less easy to find in documentation. Given that these are all dynamic languages, it's probably just an untyped dictionary.

ORMs: Django, SQLAlchemy, Sequelize

Which is Fluent?

What we have in Fluent 4 (and #12) seems like a reasonable mix between these strategies that adds type safety. Our models are simple definitions of the schema / table, like some of the other ORMs. But with Swift, we can take that a step further and use the model schema generically to make accessing the db output type-safe (Row<Planet>).

Examples

Below are the model examples for Planet in each ORM.

Rails (ActiveRecord)

class Planet < ApplicationRecord
  self.table_name = "planets"
  belongs_to :galaxy
end

class CreatePlanets < ActiveRecord::Migration[5.0]
  def change
    create_table :planets do |t|
      t.string :name
      t.belongs_to :galaxy, index: true
    end
  end
end

Laravel (Eloquent)

namespace App;

use Illuminate\Database\Eloquent\Model;

class Planet extends Model
{
    protected $table = 'planets';
    public function galaxy()
    {
        return $this->belongsTo('App\Galaxy');
    }
}

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class CreatePlanetsTable extends Migration
{
    public function up()
    {
        Schema::create('planets', function (Blueprint $table) {
            $table->increments('id');
            $table->string('name');
            $table->bigInteger('galaxyID');
        });
    }

    public function down()
    {
        Schema::drop('planets');
    }
}

SQLAlchemy

from sqlalchemy import Column, Integer, String
class Planet(Base):
    __tablename__ = 'planets'

    id = Column(Integer, primary_key=True)
    name = Column(String)
    galaxy_id = Column(Integer, ForeignKey('galaxy.id'))
    galaxy = relationship("Galaxy", back_populates="galaxies")

Django

from django.db import models

class Planet(models.Model):
    id = models.IntegerField(max_length=30)
    name = models.CharField(max_length=30)
    galaxy = models.ForeignKey(Galaxy, related_name='galaxies', on_delete=models.CASCADE)

Sequelize

const Planet = sequelize.define('planets', {
  id: Sequelize.INT,
  name: Sequelize.TEXT
})
Planet.belongsTo(Galaxy); // Will also add galaxyId to Planet model

Fluent 4

struct Planet: Model {
    static let `default` = Planet()
    static let entity = "planets"
    let id = Field<Int>("id")
    let name = Field<String>("name")
    let galaxy = Parent<Galaxy>(id: .init("galaxyID"))
}

tanner0101 commented 5 years ago

@mcdappdev

Is it possible to use keypaths inside of Field

Yes. One side of the relation needs to declare the string of course, but the other side could use a key path to reference it. For example, we could have the Child declare:

let galaxy = Parent<Galaxy>(id: "galaxyID")

But the parent declares:

let planets = Children<Planet>(\.galaxy)

Also, I've seen this syntax referred to a few times now: planet.get(.name). Will that be available in addition to planet.name.get()? Because I think the latter is a lot more natural.

It's either one or the other. The benefit of the get(...) syntax is that we may get dot-syntax in the future with @keyPathMemberLookup. So I think we should definitely go that route. (See the note in https://github.com/vapor/fluent-kit/pull/12 for more info)

jdmcd commented 5 years ago

Alright, it's really starting to grow on me. I will say though that I prefer static letdefault= Planet() be spelled as static let shared = Planet(). That may just be a mental holdover from my Objc days though :)

What do we think the chances of Key-Path member lookup being merged before Fluent 4 is? Because if we get that then I think we get the best of both worlds - natural/typed API with dynamic backing.

vapor / fluent-kit

fluent 4 api discussion #11

Model definition

Eager loading:

Custom field data types

Partial updates:

Lazy Decoding

My first impression

New vs. Old

Issue

In conclusion:

PS

Removing `Properties`

Updated opinion

PS

12 has renamed the type from `Instance` -> `Row`. The added concision and specificity makes the change more bearable to me at least. Thoughts?

1. Empty subclass + migration

2. Schema declaration

Which is Fluent?

Examples

Rails (ActiveRecord)

Laravel (Eloquent)

SQLAlchemy

Django

Sequelize

Fluent 4

vapor / fluent-kit

fluent 4 api discussion #11

Model definition

Eager loading:

Custom field data types

Partial updates:

Lazy Decoding

My first impression

New vs. Old

Issue

In conclusion:

PS

Removing Properties

Updated opinion

PS

12 has renamed the type from Instance -> Row. The added concision and specificity makes the change more bearable to me at least. Thoughts?

1. Empty subclass + migration

2. Schema declaration

Which is Fluent?

Examples

Rails (ActiveRecord)

Laravel (Eloquent)

SQLAlchemy

Django

Sequelize

Fluent 4

Removing `Properties`

12 has renamed the type from `Instance` -> `Row`. The added concision and specificity makes the change more bearable to me at least. Thoughts?