vapor / fluent-kit

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

Create on model collections does not assign database generated Ids #607

Closed dezinezync closed 5 months ago

dezinezync commented 5 months ago

Describe the issue

when using create(on:) on a collection of models, the database generated Ids are not assigned to the models

Vapor version

4.96.0

Operating system and version

macOS 14.4.1

Swift version

Apple Swift version 5.10

Steps to reproduce

In the following code, the models are correctly created on the mysql database, however, the database generated Ids do not get assigned to the respective models.

let models: [Model] = [one, two, three]

try await models.create(on: writeDB)

On the other hands, create each model one-by-one, will correctly assign the Ids as expected.

let models: [Model] = [one, two, three]

for model in models {
  try await model.create(on: writeDB)
}

Upon inspection of the code, the routine for batch inserts when using collections slightly differs in terms of how it tries to obtain the database generated Ids:

https://github.com/vapor/fluent-kit/blob/ed4cfa9edcadda3bf1b02a9842cfb60b8cf9b77b/Sources/FluentKit/Model/Model%2BCRUD.swift#L184

This never runs the following routine which may be the cause of the bug? 🤔

https://github.com/vapor/fluent-kit/blob/ed4cfa9edcadda3bf1b02a9842cfb60b8cf9b77b/Sources/FluentKit/Model/Model%2BCRUD.swift#L32-L37

Outcome

The Ids are not assigned to the models when using batch insert on a collection of models.

Additional notes

No response

dezinezync commented 5 months ago

I may be completely wrong about my assumptions here, I'd appreciate any pointers on getting this moving in the right direction. Thank you.

gwynne commented 5 months ago

This is a known limitation of using the collection version of create() (whose only benefit over doing it one model at a time is that fewer INSERT queries are issued). Unfortunately, this logic was originally designed around the incredibly foolish (to put it politely!) decision to make UUID the default ID type for models (thanks to Mongo), where the ID is generated by Fluent rather than the database.

Even more unfortunately, though, I can't honestly say I have any plans to fix this at this point; this is a very long-standing behavior (no matter how poorly chosen it may have been...), and development effort on Fluent is now directed towards Fluent 5. That being said, if someone were to open a working PR to fix the problem, I'd be inclined to accept it!

dezinezync commented 5 months ago

@gwynne as always, thank you for addressing my concerns so quickly.

Would it be possible to consider addressing this limitation in Fluent 5? I'm open to working around this for the time being in my code, by using single inserts.

gwynne commented 5 months ago

I can safely say with certainty that Fluent 5 will not have this issue (or a myriad of others stemming from similar design flaws) 🙂.