My 2ยข. Null and empty are different things and should be communicated properly in the API. It's not the API's responsibility to optimize for line-count of specific implementations. Dealing with possibly null values is annoying in Go, but doable.
That said, it's worth thinking carefully about whether null values are meaningfully different than empty ones in our application. That is to say, is there ever a situation where a client should act differently upon receiving a null value than an empty one? Likewise, does the server treat them differently? If not, then I strongly suspect you have identified a non-nullable default-empty field. If we're erroneously sending meaningful null values as empty strings, that's a bug in the system that sends it. We should fix bugs, even if that breaks version compatibility. For our go clients, we should ensure that it properly interprets all valid API values, which may include nulls if it's determined that one or more fields have a meaningful distinction on that point. If that's arduous, we should write code to ameliorate said arduousness. On Thu, Nov 16, 2017 at 1:20 PM, Robert Butts <[email protected]> wrote: > #2 pushes the support burden to clients. We'll be requiring clients to > check for null everywhere, which in Go requires 4 lines, quadrupling the > code. Sometimes clients need to distinguish between null and empty, but > when they don't, it's 1/4 the Go code to have non-pointers that won't crash > your app if unchecked. > > In retrospect, #1 and #2 both break the Version promise. #1 breaks the > Server API, and #2 breaks the Client API. Which leaves #3 and #4. > > #3 still breaks the Version promise: Go's `omitempty` omits JSON fields if > the value is "empty", i.e. zero or the empty string. That will make TO > Server responses that are legitimately "" or the number 0 be omitted, which > would be fine for the TO Client, but will break anyone directly hitting > HTTP and distinguishing missing/null and 0. > > Which leaves #4. I'm not seeing a way to do anything else, without breaking > the Version. Unless someone sees something I don't? I don't like the code > overhead either, but I'm not seeing a way around it. > > It's reasonable for clients to need to distinguish empty and null values, > but we haven't had any complaints yet, so it doesn't seem urgent. I'd vote > we separate the structs until someone has a need, and revisit the client > then. Possibly making separate `GetServers` and `GetServersWithNull` funcs, > so users can keep using the old func unless they need to distinguish nulls. > > It may be slightly less overhead than it sounds, too. In Go, we can use > Anonymous Members to do "inheritance": > > ``` > type BasicServer struct { > HostName string > } > type Server struct { > BasicServer > HTTPSPort int > } > type ServerWithNull struct { > BasicServer > HTTPSPort *int > } > ``` > > As a tangent, +95% of the nullable db fields are from our schema being > denormalized, which causes not only issues like this, but data runtime bugs > in the CDN. If we can ever find the resources, pulling our Server and > Deliveryservice fields that only apply to certain types into their own > tables, will not only make a ton of runtime bugs impossible, but will also > make problems like this go away, making our code safer and cheaper to > maintain. > > > On Thu, Nov 16, 2017 at 12:36 PM, Dan Kirkwood <[email protected]> wrote: > >> Hey, all.. we've discovered a discrepancy that was introduced >> recently in master in the new Traffic Ops Go api. I apologize for >> the length of this, but am leaving out some details to keep it a bit >> shorter... >> >> Here's the problem: Many columns in the db that can be null are now >> represented as empty strings in the json output. In the Perl/Mojo >> version, they are represented in the json output as null, but in the >> Go version, they are empty strings. >> >> This can present problems with code that uses the json output if it >> distinguishes between null and "". We've seen this in testing when >> the Traffic Portal used the empty string value to update the >> ip6address column in the server table. >> >> I made an attempt to fix this problem with this PR: >> https://github.com/apache/incubator-trafficcontrol/pull/1549 -- it was >> merged accidentally, but then reverted with this PR: >> https://github.com/apache/incubator-trafficcontrol/pull/1557. >> >> I think my approach was ideal, but was a bit heavy-handed: any >> existing code (e.g. Traffic Monitor, Traffic Stats, or any external >> code using Traffic Ops Client) would potentially need to change to >> check for nil pointers where they would not have before. >> >> I see 4 approaches that could be taken: >> 1. Leave as-is. null db entries would be received in Go as empty >> strings and any client using them would have to be able to deal with >> them. New API endpoints in the Go code would (mostly) do the same. >> 2. Change to use pointers for these columns. Those pointers would >> be nil and any code (new and existing) would need to change to check >> for nil pointers in those cases. >> 3. Change so that those columns are omitted from the json data if empty. >> 4. Leave the existing structs in place for code that currently uses >> it and create new parallel structs with these columns represented as >> pointers. traffic_ops_golang and any new code should use these. That >> presents an additional support burden.. >> >> Personally, I think #2 is the best option long-term, but realize it >> does have drawbacks. Please let me know what you think.. >> >> Thanks.. Dan >>
