#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
>
