Nexus external API: Use SwitchSlot enum instead of strings#10014
Conversation
I do like consistency. I figure if we're going to make a breaking change, it might make sense to go ahead and make it all consistent today instead of potentially being annoyed by the inconsistency later down the road. How much bigger is the blast radius if we make the rest of the names consistent? |
In terms of this PR, I think the blast radius is minimal. Changing the types already required us to define new In terms of external consumers, I don't know - how painful are console / CLI updates when there are breaking (but minor) changes in the external API? |
|
If we want to standardize the field names, I do think it'd be easiest to just do it now (since we're already bumping all these API endpoints). I pushed that up as 4940f5e - easy to revert if it's not worth it, but it was also pretty easy to do. ( |
I'm guessing that the cost we pay in updating the cli and console will be paid back in terms of having an api and data structures that are clearer and more consistent. |
| fn format_switch_slot_as_name(switch_slot: SwitchSlot) -> Name { | ||
| format_switch_slot_as_str(switch_slot) | ||
| .parse() | ||
| .expect("stringified switch slots have valid `Name`s") | ||
| } |
There was a problem hiding this comment.
Dropping a comment from chat here, in the ultra-paranoid case that somehow our stringified version of the SwitchSlot deviates from what a legal Name is (or vice versa), this could cause Nexus to panic. However, a small unit test could guard us against that if we don't want to propagate Results up through the callers.
Reasoning here is similar to #10010: this isn't really a naturally-displayable type, and in this case we were previously abusing this to stringify it both in the db (fixed by #9984) and the external API (fixed by #10014, on which this PR is based). Maybe worth noting: I expected this to be entirely futureproofing, but by doing this I found a few external API types that I'd missed in an earlier draft of #10014 (types where we stringified `SwitchSlot`s for _output_ but never parsed them as _input_). All our remaining uses of the display impl are either for logging or constructing internal error messages; I changed all of these to use `Debug`, and added a manual `Debug` implementation that matches the old `Display` implementation. This should keep the error messages and logs consistent while not leading us to the pit of sadness of assuming we can format and parse these values as strings without consideration.
2 participants
This changes the type of a bunch of fields in the external API to
SwitchSlot(the enum formerly known asSwitchLocation). The updated fields:BfdStatus::switchLoopbackAddress::switch_locationSwitchPort::switch_locationLldpPortPathSelector::switch_locationLoopbackAddressPath::switch_locationSwitchPortSelector::switch_locationLoopbackAddressCreate::switch_locationBfdSessionDisable::switchBfdSessionEnable::switchPreviously, the type of each of these fields was either
StringorName, but in practice was either guaranteed to be (for types returned by Nexus) or required to be (for input parameters)"switch0"or"switch1", which are the same encodings of theSwitchSlotenum. That makes this a breaking change as far as the OpenAPI spec is concerned, but in practice it is a wire-compatible change for valid requests. (Invalid requests now rejected by the spec / dropshot's parser would previously have been rejected by Nexus somewhere else down the line.)I'm tempted to propose changing the names of all of these fields to
switch_slotfor consistency with each other and the type name, but that may be unnecessary churn with little benefit. I'll defer to @internet-diglett or folks who have more opinions about the external API (@david-crespo or @ahl maybe?); happy to do the work if it's useful, happy to skip it if it's annoying.Closes #3604.