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

Model batch CRUD + model middleware #371

Open
tanner0101 opened this issue Aug 4, 2020 · 3 comments
Open

Model batch CRUD + model middleware #371

tanner0101 opened this issue Aug 4, 2020 · 3 comments
Labels
bug Something isn't working enhancement New feature or request
Projects

Comments

@tanner0101
Copy link
Member

The way Model's batch CRUD methods currently work with ModelMiddleware is inconsistent with normal CRUD. When doing a normal, single-model CRUD operation, the actual operation happens when you pass along the request to next. However, during batch CRUD, the request to next is only adding the model to a buffer. This creates issues with code that expects the operation to have been completed by time the next responder finishes. For example, take the following code:

func create(model: User, on db: Database, next: AnyModelResponder) -> EventLoopFuture<Void> {
    next.create(model, on: db).flatMap { _ in
        self.updateIfNeeded(model: model, db: db)
    }.flatMap { _ in
        model.save(on: db)
    }
}

This middleware code is designed to run after a model is created. After the model is created, it does an update, and another save if needed. When using normal, single-model CRUD it works as expected. However, when using batch CRUD, since the create is not actually happening in next.create, the update / save runs before and is responsible for creating the model. When the batch create finally runs after all of this code, it will likely fail since the models have already been created.

@tanner0101 tanner0101 added bug Something isn't working enhancement New feature or request labels Aug 4, 2020
@tanner0101 tanner0101 added this to To Do in Vapor 4 via automation Aug 4, 2020
@madsodgaard
Copy link
Contributor

Well done noticing this!

I guess an issue is that to provide the same functionality we could end up just doing n amount of individual queries instead of utilizing database features to insert multiple rows at once, right? Not sure what the performance cost are, but I am guessing the database is faster than multiple individual INSERT queries.

@tanner0101
Copy link
Member Author

tanner0101 commented Aug 6, 2020

Thanks to @jdmcd for reporting. I was thinking we could offer a separate method for batch create specifically. Something like:

func batchCreate(models: [Model], on db: Database, next: AnyModelResponder) -> EventLoopFuture<Void>

This would allow us to actually do the create operation in the AnyModelResponder callback and then everything should work as intended. I think having access to the entire array could prove beneficial as well. For instance, the middleware could choose to add or remove models from a batch create operation, change it to an individual create, easily set a property on all models to a derived value for that operation, etc.

@tanner0101
Copy link
Member Author

We should also take batch delete into account here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Vapor 4
  
To Do
Development

No branches or pull requests

2 participants