Repository navigation
JSON marshalling and omitempty for empty strings and bools #1332
Description
Certain gophercloud implementations omit empty strings and false bools.
Examples:
- invert in loadbalancer/v2/l7policies/requests.go . You can set true to invert, but cannot set false.
- Similar issue occurs for name and description fields, e.g. you can set non-empty description, but you cannot remove it.
ref: golang/go#13284
#1314
Activity
jtopjian commentedon Nov 30, 2018
@kayrus If you want to use this issue as a global catch-all issue for this type of problem, that works for me.
This is something we run into fairly frequently - it's usually an oversight in the initial modeling of the API call. But often the solution needs to be considered case-by-case.
For most occurrences, we want to change the type to a pointer-type. As you mentioned in #1314, there is a concern about breaking compatibility, but that's OK. In general, we try not to make breaking changes, but if the change is for the long-term better, then break away. Right now, Gophercloud makes no guarantees about backwards compatibility for reasons like this.
I wish it would be as simple as changing the type to a pointer-type, but there are edge cases where it won't work. I can't think of any off the top of my head, but they're around (thankfully not many).
So the best course of action is to take each occurrence one-by-one and fully understand the implications of each value:
bool,omitempty: will never be able to sendfalsestring,omitempty: will never be able to send""*bool,omitempty: will be able to send false, will be omitted if not set*string,omitempty: will be able to send"", will be omitted if not set*bool: will be able to send false, will sendnullif not set.*string: will be able to send"", will sendnullif not set.The last examples are the trickiest to determine if they are needed. These are for cases when the API wants 3 values: set, not set, null. The Neutron Port API is notorious for this pattern.
Please let me know if you have any questions :)
kayrus commentedon Nov 30, 2018
If backward compatibility could be ignored, then my idea is to update at least all
UpdateOpts'string,omitemptyto*string,omitempty. We should not touchCreateOpts, correct? It could be done within one PR.Then identify all the cases for
*booland walk through them one by one. Collect obvious green light fixes, create another PR.For the rest tricky bools, we can create a separate PR.
jtopjian commentedon Nov 30, 2018
I think that's a safe plan. If you're able to update the acceptance tests to prove these changes are valid, that would be the best way to confirm the changes were correct.