Use SQL for Destination{Create,Update,Delete} #3356
Conversation
We can get the count directly without the sub-query. This does change the order of results but the only caller of this function does not care about the order because they are emitted as independent metrics.
Previously we were returning ErrNotFound if the ID did not match any destination, but we don't do that for any other entity, and no tests appear to expect that behaviour. Removing that for now for consistency, but we can re-introduce it by checking the result for the number of updated rows if we want to restore it.
dnephin
requested review from
BruceMacD,
jmorganca and
pdevine
as code owners
| return deleteAll[models.Destination](db, ByIDs(ids)) | ||
| } | ||
|
|
||
| return internal.ErrNotFound |
There was a problem hiding this comment.
Previously we were returning ErrNotFound if the ID was not found, but we don't do that for any other types. I suspect we should be consistent with the behaviour.
If we want to return a not found on a missing delete then we can check result.RowsAffected() > 0 from the result returned by tx.Exec.
| // destinationsUpdateTable is used to update the destination. It excludes | ||
| // the CreatedAt field, because that field is not part of the input to | ||
| // UpdateDestination. | ||
| type destinationsUpdateTable models.Destination |
There was a problem hiding this comment.
This feels a bit off, but I can't come up with a better way to do updates either in our current model
There was a problem hiding this comment.
It is unfortunate, there are some other options:
- We could
select created_atand set the value on the struct, before theUPDATE - We could not use
insert()and copy out the implementation intoCreateDestination.
Maybe option 2 is better, I'll see what it looks like
There was a problem hiding this comment.
I guess the problem with 2 is that we have to add CreatedAt back in for both Get and List, which means we end up having to deal with the special case in 3 places, instead of only Update being a special case.
Let's try it with the current implementation, and see if we run into this problem with any other models.
Summary
This PR converts the three destination write queries to use SQL.
DestinationUpdateis a bit of a special case. We don't receive the created_at time, so we have to exclude it from the query. I did that by creating a second set of methods forColumnsandValues. We could also select the row first, but it seems like we should be able to do an update and omit the rows that we don't care to update. If this becomes a more common pattern then maybe we can support that difference in the Table interface somehow.Also updated
CountDestinationsByConnectedVersionto remove a sub-select.Related Issues
Related to #2415