[reconfigurator] Introduce IdMap and use it in BlueprintZonesConfig
#7327
Conversation
| { | ||
| "$ref": "#/components/schemas/IdMapBlueprintZoneConfig" | ||
| } | ||
| ] |
There was a problem hiding this comment.
I'm not sure why this changed, exactly. Does JsonSchema do something special for BTreeMap that it doesn't know how to do for IdMap?
I think this is semantically equivalent, though; allOf says to apply all of the inner subschemas, and since this subschema is the same as what the prior schema was, this is basically undoing an inlining?
There was a problem hiding this comment.
hmm, to me this looks like the same kind of semantically equivalent reference as all the newtype UUID kinds that show up.
I think if you want to avoid this you can use
https://graham.cool/schemars/implementing/#alwaysinlineschema-optional. Whether to avoid or not is your call -- having it is equivalent to a newtype in client-generated code I believe.
If you have it, though, it might be nice for clients to reuse the same IdMappable struct. Which actually suggests putting it in a crate on crates.io, similar to newtype-uuid.
... and now that I'm thinking about it, this seems like a really nice thing to have available more generally! I feel like this specific thing (wanting to store data by key, but also putting the key in the data) shows up repeatedly, and having a wrapper which enforces consistency seems extraordinarily useful! I know I'd have used it in a little hobby thing I was doing just a couple months ago.
There was a problem hiding this comment.
Keeping the newtype seems fine for internal stuff just like for newtype-uuid, I think!
this seems like a really nice thing to have available more generally! I feel like this specific thing (wanting to store data by key, but also putting the key in the data) shows up repeatedly, and having a wrapper which enforces consistency seems extraordinarily useful!
Fair enough! I also need to split out and publish slog-inline-chain which is similarly small and externally useful. I'm inclined to merge this and open issues for splitting those out - seem reasonable?
There was a problem hiding this comment.
Looks great. Thanks for all the work here @jgallagher.
I have no idea about the JsonSchema question. Maybe @ahl does.
| @@ -794,7 +795,7 @@ impl DataStore { | |||
| e.to_string() | |||
| )) | |||
| })?; | |||
| sled_zones.zones.insert(zone.id, zone); | |||
| sled_zones.zones.insert(zone); | |||
| Self { original_id: borrowed.id(), borrowed: Some(borrowed) } | ||
| } | ||
|
|
||
| pub fn into_ref(mut self) -> &'a T { |
There was a problem hiding this comment.
Nice closing of this loophole too!
@andrewjstone mentioned being bothered by our
BTreeMap<Id, ValueWithAnId>maps on #7315:This has bugged me too, and adding a blippy check crossed my mind. But I think it would be better to make this just not possible, so this PR introduces a newtype wrapper around
BTreeMap<T::Id, T>that enforces "key must be value's ID" via a mix of compile time API constraints (e.g.,insertonly takes the value and generates the key on its own) and runtime checks (e.g., when mutating a value stored in the map, it will panic if that mutation changes the ID of the value).The new code is all in
id_map.rs, and the rest of the changes are replacing theBlueprintZonesConfigmap with it. If this looks reasonable to folks, followup PRs to change the other similarBTreeMaps to useIdMapshould be straightforward.