Consider updating library to use pointers for optional fields

[ojongerius]

Hi there,

So far using default values has worked for us but it will not work in the case where values can either be set or not, and not setting them can influence behaviour.

An example issue issue I ran into require_full_window which is a Boolean, and optional. If the option is not set Datadog uses custom behaviour, which is desirable in some cases.

Using the current API this field defaults to false, which is different to it not being set. I suspect, but have not checked if there are other cases where we ran into this issue.

A solution is to refactor the library to use pointers, as this allows us to check if a value is actually set. The upside is more precision, the downside is that clients using the library now have to do a little more work to use pointers.

A struct like Monitor currently looks like so:

// Monitor allows watching a metric or check that you care about,
// notifying your team when some defined threshold is exceeded
type Monitor struct {
    Creator Creator  `json:"creator,omitempty"`
    Id      int      `json:"id,omitempty"`
    Type    string   `json:"type,omitempty"`
    Query   string   `json:"query,omitempty"`
    Name    string   `json:"name,omitempty"`
    Message string   `json:"message,omitempty"`
    Tags    []string `json:"tags,omitempty"`
    Options Options  `json:"options,omitempty"`
}

And would look like this using pointers for optional fields only:

// Monitor allows watching a metric or check that you care about,
// notifying your team when some defined threshold is exceeded
type Monitor struct {
    Creator *Creator  `json:"creator,omitempty"`
    Id      *int      `json:"id,omitempty"`
    Type    string   `json:"type,omitempty"`
    Query   string   `json:"query,omitempty"`
    Name    *string   `json:"name,omitempty"`
    Message *string   `json:"message,omitempty"`
    Tags    []string `json:"tags,omitempty"`
    Options *Options  `json:"options,omitempty"`
}

Or like so using pointers for all fields:

// Monitor allows watching a metric or check that you care about,
// notifying your team when some defined threshold is exceeded
type Monitor struct {
    Creator *Creator  `json:"creator,omitempty"`
    Id      *int      `json:"id,omitempty"`
    Type    *string   `json:"type,omitempty"`
    Query   *string   `json:"query,omitempty"`
    Name    *string   `json:"name,omitempty"`
    Message *string   `json:"message,omitempty"`
    Tags    []string `json:"tags,omitempty"`
    Options *Options  `json:"options,omitempty"`
}

There are different ways to go about this:

Option	Pros	Cons
Do nothing	No work to be done in library	Users have to create their own structs
Pointers for optional only	Precision. Less tedious to use than making everything a pointer. Less work to implement.	Inconsistent, more work to check which fields are optional
Pointers for all	Precision, easier to implement	More work for users of library

Will Noris wrote an insightful blogpost about this at go-rest-apis-and-pointers, which relates to his adventures in issue 19 at go-github. Go-github did make the switch to use pointers for all the things, and created helper functions to make using it a little less tedious. The result is not very pleasant to read and use, but it is precise.
There is an interesting discussion making a similar choice in issue 2330 of swagger-api.

I'm leaning toward using pointers for optional fields. @zorkian thoughts?

Cheers,

Otto

[ojongerius]

@zorkian ping!

[zorkian]

Hey this is a fantastic question and thanks for the ping. I agree that we need to support the functionality, but I also think it's ugly as heck. We use lots of protobufs in Go at my dayjob and those get really messy. It's doable and it's fine and it seems to be the way that most APIs go but it leaves me wondering if there's not a better way.

If a field is truly optional then having it be a pointer seems reasonable. That way most of the time the user won't specify it but if they need to they can (with the added and frustrating verbosity). Hmm. Okay so I was messing around in the Playground and am wondering how bad something like this is -- I know the performance is crap but I'm not sure this API is ever used in performance-critical places (may be a pipe dream to wish it wasn't):

https://play.golang.org/p/-LJSAHFY6h

What do you think about that, does it make you super sad? We can even get fancier and use reflect to determine if they've passed things that are names of members of the struct. Whether or not this is a good idea I don't know, but it seems like it might (might) keep the verbosity in the API rather than pushing it to the end-users.

Thoughts?

[nyanshak]

I think making optional fields use pointers is reasonable. Would it be okay if I made a PR to make Yaxis struct's fields into pointers? Right now when I'm trying to use this, I can't distinguish between Yaxis.Min = 0 and Yaxis.Min = unset, meaning graphs that I'm making end up very broken.

[nyanshak]

@zorkian I think that something like that code snippet you posted would make optional structs cleaner. I'm not super familiar with Go style, so I don't know how other libraries have handled the same situation. That said, it looks like a good solution.

(to be clear, referring to this snippet: https://play.golang.org/p/-LJSAHFY6h)

[zorkian]

We should circle back to this and figure out what we want to do given that there's another request open #75 related to this.

[zorkian]

Given:

• Usage of non-pointer fields + omitempty leads to bugs (such as in JSON Marshalling of User Booleans Not Working Correctly #75),
• Google's Protobuf library uses pointer fields, so this is established practice,
• Go versioning kind of sucks.

I propose:

• We cut the current version (or a very soon version) of go-datadog-api and branch it onto a v1 branch and then stop touching it, with caveats that some things are known-broken (such as JSON Marshalling of User Booleans Not Working Correctly #75) and can't be fixed,
• We start development on a v2 (on master) and convert all fields to pointer types, and create helpers as necessary to make things easy to work with.

While I don't like the idea of pointer fields everywhere, it's at least something we're used to and seems to be a reasonable way of moving forward.

Thoughts on this? Any changes/suggestions?

[nyanshak]

I agree with the approach proposed (cut current(ish) version and start v2 branch), but I'd suggest some additional changes to more completely deal with the API quirks.

One example is that certain fields are optional, but also can be either a number or a string (which is a number) depending on which graph type they're included in. It's late here and I don't have my notes around, but it's something like:

# For a change graph
{
...
  "some_field": 1 // this one is a number
}

# For a timeseries graph
{
...
  "some_field": "1" // this one is a string number
}

I had previously submitted PRs to change from json.Int (or float) to json.Number in some cases, but I imagine this (v2 branch creation) would be an appropriate time to make these breaking changes.

[combor]

Hi,

I'd like to 👍 for fixing this issue as we hit it as well while working on alphagov/paas-cf#738. We are going to implement some dirty workaround in the meantime by using curl and API directly to update require_full_window which we need to be set to false.
Do you have any timeframe when it comes to implementing changes? Can we help somehow?

[zorkian]

I was letting ~enough time to go by for any objections/feedback but haven't heard anything, so let's go ahead with the plan to switch everything to pointers and cut it as v2. I'm at my day-job right now and won't have time to work on this today, so if you're particularly motivated you can start the conversion and we can review it.

If not, I should have time sometime within the next week to do the work.

[combor]

Cool, I'm busy as well this week but I might be able to do sth this weekend. No promises though :)

[ojongerius]

Just got off a 3 hour flight and have a rough cut I should be able to push out before the weekend.

[ojongerius]

Have raised #83 as proof of concept. See my notes in the PR, happy to discuss either here or there!

[ojongerius]

Copy of the most part of my comment in #83, so people know what's up with this issue:

I've coded something up that generates SetXx, HasXx, GetXx and GetXxOk. Using it would look like:

	m := datadog.Monitor{}
	c := datadog.Creator{}
	
        // Use SetXx:
        c.SetName("Joe Creator")
	m.SetName("Monitor all the things")
	m.SetMessage("Fat electrons in the lines")

	// Use HasXx:
	if m.HasMessage() {
		fmt.Printf("Found message %s\n", m.GetMessage())
	}

	// Or maybe you prefer a tuple with the value and a boolean if it was ever set
        // and use GetXxOk
	if v, ok := m.GetMessageOk() ; ok {
		fmt.Printf("Found message %s\n", v)
	}

I still like having the other helpers around, and keep support for doing:

	// You could still initialize the struct in one swoop
	_ = datadog.Monitor{
		Name: datadog.String("Monitor other things"),
		Creator: &datadog.Creator{
			Name: datadog.String("Joe Creator"),
		},
	}

I'll push up some changes to this PR hopefully today, else tomorrow.

[ojongerius]

Pushed changes up, see #83

[ojongerius]

#83 has been merged 🎉

Do note that this has been merged to v2, which you can use by importing import "gopkg.in/github.com/zorkian/go-datadog-api.v2". Well wait with cutting the main branch over to this one. See https://github.com/zorkian/go-datadog-api/blob/v2/README.md