Fix broken disk list #9553
Fix broken disk list #9553
Conversation
For shame!
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
It took me a minute to figure out what this fixes since there wasn't a description of the bug in the PR description, but upon looking at the change, the bug was pretty obvious. Nice fix!
I had some small style thoughts but I think none of them are important and some of them might be bad ideas.
| results: Vec<( | ||
| model::Disk, | ||
| Option<DiskTypeCrucible>, | ||
| Option<DiskTypeLocalStorage>, | ||
| )>, |
There was a problem hiding this comment.
part of me wonders if we might be better off having some kinda
#[derive(Selectable)]
struct RawDiskListEntry {
disk: model::Disk,
crucible: Option<DiskTypeCrucible>,
local: Option<DiskTypeLocalStorage>,
}so that we can use named fields in the match later, but perhaps that's not worth the effort... i dunno.
| PaginatedBy::Name(pagparams) => paginated( | ||
| dsl::disk, | ||
| dsl::name, | ||
| &pagparams.map_name(|n| Name::ref_cast(n)), |
There was a problem hiding this comment.
unimportant turbo-nit: this could be
| &pagparams.map_name(|n| Name::ref_cast(n)), | |
| &pagparams.map_name(Name::ref_cast), |
| let results = match pagparams { | ||
| PaginatedBy::Id(pagparams) => { | ||
| paginated(dsl::disk, dsl::id, &pagparams) | ||
| } | ||
| PaginatedBy::Name(pagparams) => paginated( | ||
| dsl::disk, | ||
| dsl::name, | ||
| &pagparams.map_name(|n| Name::ref_cast(n)), | ||
| ), | ||
| } | ||
| .left_join( | ||
| disk_type_crucible_dsl::disk_type_crucible | ||
| .on(dsl::id.eq(disk_type_crucible_dsl::disk_id)), | ||
| ) | ||
| .left_join( | ||
| disk_type_local_storage_dsl::disk_type_local_storage | ||
| .on(dsl::id.eq(disk_type_local_storage_dsl::disk_id)), | ||
| ) | ||
| .filter(dsl::time_deleted.is_null()) | ||
| .filter(dsl::attach_instance_id.eq(instance_id)) | ||
| .select(( | ||
| model::Disk::as_select(), | ||
| Option::<DiskTypeCrucible>::as_select(), | ||
| Option::<DiskTypeLocalStorage>::as_select(), | ||
| )) |
There was a problem hiding this comment.
it occurs to me that we might be able to share more code between the two functions by pulling everything here except for the .filter(dsl::attach_instance_id.eq(instance_id)) bit out into a function returning a query, and then have both instance_list_disks_on_conn and disk_list call that function, and just stick slightly different filters (by either the instance ID or the project) on that query fragment.
on one hand, that also would have prevented the original bug, but on the other, well...figuring out the Diesel type of that query is probably gonna hurt, and i think the process_tuples_to_disk_list function taking a Vec of the correct-typed tuple is already a pretty okay way to stop you from forgetting to query one of the tables, and...how many more disk-list-like APIs are we ever gonna add?
so, the current thing is fine and this doesn't matter.
3 participants
For shame!