Support atomic batch deletion

[chrisandreae]

The VM::AR controller destroy action now optionally parses multiple ids for
deletion, and the action is aliased as a collection route as well as a member
route.

Deletion is performed one at a time within the same transaction.

[thefloweringash]

We need to sort out the "show with multiple ids" situation before we can ship this.

[thefloweringash]

Is this the first time we've formalized this requirement?

We could defer the id serializer to the controller, which would avoid encoding this requirement and be more precise about the particular id type in use. The cost of this, of course, is in integration complexity which is already very high.

[chrisandreae]

I think it's been implicit up to now. The only reason I'm adding it here is because I couldn't straightforwardly make an ArrayOf.new(Anything). That said, I think it would be just fine and sufficient to say "Array of Integer or String" and not have any further constraints, which would effectively be the same as before. Maybe that would be better.

[chrisandreae]

I loosened the restriction back to integer-or-string

[thefloweringash]

Do we explicitly support fetch by multiple id? We have clients that make use of this, and either it works because the backend doesn't enforce types and thus viewmodel_id is already sometimes an array, or it doesn't work and the clients somehow don't hit that call path.

I think show should support fetch by multiple id. Perhaps you can do so by copy/pasting the viewmodel_ids parser from delete?

[chrisandreae]

We explicitly support index filtered by multiple id, by having a default :id array-typed request filter defined. I think that's a more natural place for it than introducing a new route "show, but show for specific set".

[thefloweringash]

We explicitly support index filtered by multiple id, by having a default :id array-typed request filter defined.

Alright, things won't break, 👍.

Awkward that superficially similar concerns are implemented in entirely different layers and code bases, but that's not a reason to hold this back. I'm not even sure we can do better.

[chrisandreae]

We could conceivably pull the request filtering framework out of the application into the gem. The main challenge I think would be factoring it out so that the scope part went in the gem, and the search part remained in the application, since the whole ES integration is app side.

[butchler]

FYI I was successfully able to use the new DELETE /api/<entity name>?id[]=...&id[]=... endpoint in my frontend branch. So it should be fine to deploy this any time, and it doesn't have to wait until my branch frontend branch is ready to deploy.

[butchler]

@chrisandreae Random thought: The frontend currently sends the list of IDs via query params when making a DELETE request. In theory, this means if someone tries to delete a few thousand entities or something, they might end up creating a request URL that is too long and the browser rejects it or something. I don't know if this will ever be a problem in practice, and I don't think we need to go out of our way to resolve it until it becomes a problem. But if this does end up becoming a problem, is there something we could do about it?

It's probably technically possible to send a body with a DELETE request, but according to stackoverflow it's the kind of thing that's technically possible but generally discouraged because it might cause interoperability issues. I guess we could use a POST request instead if we need to?

[butchler]

Another random thought: How will this work for controllers that override the destroy method? For example, it looks like the users controller overrides destroy to do a soft delete instead of a hard delete. Does that mean the users controller won't support bulk deletion by default unless we explicitly add it on the backend? Does it mean that it will support bulk deletion, but will end up doing a hard delete instead of a soft delete? Something else?

[chrisandreae]

but generally discouraged because it might cause interoperability issues. I guess we could use a POST request instead if we need to?

I think it's like GET with body: Rails allows it and it should just work on the server side, but it's more a question of (a) will the frontend make it hard to do, and (b) will any intermediary HTTP caches get in the way.

Rails/Rack also has a built-in work-around for this issue: you can make a POST with a extra parameter called _method (or an extra header called X-HTTP-Method-Override) which is the HTTP method you actually want it to be, and it'll be converted into the right thing by the Rack middleware.

Does that mean the users controller won't support bulk deletion by default unless we explicitly add it on the backend?

It's this one: when we update the app to use this library version, we'll want to make sure that all the places we override destroy follow the new contract where id could be a list.

[butchler]

I haven't looked into whether or not it would be hard to make a DELETE request with a body on the client, but I think the _method workaround should work if we need to use it. So I think we can just use query params for now, and if we run into issues related to the URL length we can use the _method workaround later.