Allow a NULL tags property to be nil when read from the database

[brandur]

This one's so tiny that it's minutiae, but as I was looking at the
null-ability of various job row fields today while implementing Ruby, I
realized that it doesn't really seem necessary to be marshaling a NULL
tags value to an empty []string{} slice. In Go, []string{} and
[]string(nil) are functionally identical in every way that I can think
of (append on nil works, len on nil works), so I think it makes
sense just to avoid the allocation and reflect what the database's
actual value is.

[1] https://go.dev/tour/moretypes/15

[bgentry]

A couple of minor potential differences:

1.Does this alter the behavior of json serializing a Job if the user chooses to do that for some reason? Say for error reporting as part of a payload to a bug tracker, or some other instrumentation. Do we care or should we have coverage for this?
2. Does this in any way affect APIs that subsequently use a job with nil tags? I think the answer is “no” because tags aren’t ever written back to the database after initial job insertion, but wanted to confirm.
3. The only other impact I could think of is any consumers downstream of a subscription might see a different result after this change and poorly written ones might somehow have an issue, but this isn’t a concern as of this initial release.

I think my conclusion is that we should move forward with this but wanted to confirm your thoughts on the above.

[brandur]

1.Does this alter the behavior of json serializing a Job if the user chooses to do that for some reason? Say for error reporting as part of a payload to a bug tracker, or some other instrumentation. Do we care or should we have coverage for this?

Yeah it would — the nil would get written instead of an empty array. I think this is better though because it more accurately reflects the state of the database.

2. Does this in any way affect APIs that subsequently use a job with nil tags? I think the answer is “no” because tags aren’t ever written back to the database after initial job insertion, but wanted to confirm.

I don't think so because as noted, behavior of append and len on the nil are functionally identical in Go. I can think of any ways that it'd change anyway.

3. The only other impact I could think of is any consumers downstream of a subscription might see a different result after this change and poorly written ones might somehow have an issue, but this isn’t a concern as of this initial release.

Yep +1. That's why I want to make sure to lock changes like this in early — although minor, I'd still consider this a breaking change later on because it's possible to write code that'd be broken.

[brandur]

BTW I should've mentioned:

The other strong contender here is to make tags NOT NULL and DEFAULT [] in the database, so it becomes always not-nil and an empty array on any job row that comes back.

I don't mind this way either and almost went this route (tend to prefer NOT NULLs anyway). I just think that we should be doing as little post-load mangling as possible — if we have a NULL in the DB then leave it as a nil in code too. If we have an array in the DB, then make it an array in code too.

Let me know your thoughts on that. Going to merge this for now in the interest of cutting a release, but happy to change to that too.

[bgentry]

The other strong contender here is to make tags NOT NULL and DEFAULT [] in the database, so it becomes always not-nil and an empty array on any job row that comes back.

I kinda like that as it will enforce consistency in all clients without requiring special handling in some languages. I can PR that later today.

[brandur]

Cool. Works for me.