vapor / fluent-mysql-driver

🖋🐬 Swift ORM (queries, models, relations, etc) built on MySQL.
MIT License
77 stars 52 forks source link

Make LastInsertIDInitializable public #199

Closed t-ae closed 3 years ago

t-ae commented 3 years ago

I'm planning to have indivisual ID types for each models. For example:

struct MyModelID: Codable, Hashable, RawRepresenatable, LosslessStringConvertible {
    var id: Int

    ...
}

final class MyModel: Model {
    @ID(custom: "id", generatedBy: .database)
    var id: MyModelID?

    ...
}

It seems most Fluent futures works well, but I found a problem.

id of MyModel is auto incrementing INT column, so it should be set after the model is saved. Unfortunately, if the type of id does not conform to LastInsertIDInitializable, the program crashes at here: https://github.com/vapor/fluent-mysql-driver/blob/900c4c62926c0ba6bc35af9bd64c2f9d4812ca93/Sources/FluentMySQLDriver/FluentMySQLDatabase.swift#L149-L153 To make matter worse, MyModelID cannot confrom to LastInsertIDInitializable because it is internal. https://github.com/vapor/fluent-mysql-driver/blob/900c4c62926c0ba6bc35af9bd64c2f9d4812ca93/Sources/FluentMySQLDriver/FluentMySQLDatabase.swift#L157-L159

I want it to be public.

Also I want to hear your opinion about using custom types as ID. I think its good idea but haven't gone deep yet.

0xTim commented 3 years ago

Why do you want to use a custom type for the ID? I can see this breaking a lot of Fluent, including the relationships and querying since Fluent won't know what underlying DB type to map to in the encoder/decoder

t-ae commented 3 years ago

The main reason is that it is easy to misuse if all models have same ID type. The IDs of ModelA and ModelB is similar but they are different. ModelA.find(idOfModelB, on: db) is totally incorrect operation. I want to forbid it with type system.

I can see this breaking a lot of Fluent, including the relationships and querying since Fluent won't know what underlying DB type to map to in the encoder/decoder

That's correct. I didn't noticed simply making LastInsertIDInitializable public causes several problem. There's no restriction LastInsertIDInitializable will be encoded to integer. As far as I know there's no way in type system to assure the underlying type so protocol approach won't work.

The only way that can achieve my demand is having ID type in this repository but it's going too far. I give up this issue. If someone has good idea, please tell me.