Skip to content

devices: make LastSeen a pointer #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 9, 2025
Merged

devices: make LastSeen a pointer #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,7 @@ func Test_BuildTailnetURLDefault(t *testing.T) {
require.NoError(t, err)
assert.EqualValues(t, expected.String(), actual.String())
}

func ptrTo[T any](v T) *T {
return &v
}
2 changes: 1 addition & 1 deletion devices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ type Device struct {
IsEphemeral bool `json:"isEphemeral"`
IsExternal bool `json:"isExternal"`
ConnectedToControl bool `json:"connectedToControl"`
LastSeen Time `json:"lastSeen"`
LastSeen *Time `json:"lastSeen"` // Will be nil if ConnectedToControl is true.
Copy link
Member

@bradfitz bradfitz Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non-blocking)

our own Time wrapper type was already weird, but now this is kinda double weird

doesn't json/v2 let us do omitzero + custom marshaling on a normal time.Time?

cc @dsnet

Copy link
Member

@dsnet dsnet Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already support omitzero in v1 json.

Can't we just rely on time.Time.IsZero to indicate that ConnectedToControl is true?

Copy link
Member

@raggi raggi Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to teach users about this API pattern in Go

Currently this is all undocumented, which is a large part of the problem, and it needs documentation that includes both what it is, what it is not, and usage guidance, as it's a big challenge for folks to understand.

Copy link
Collaborator Author

@oxtoacart oxtoacart Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two levels of documentation, one is the actual API documentation which I think is quite clear, the other is documenting the Go API wrapper itself, which could use some work.

Educating people about Go usage patterns for zero values is definitely worth considering, but in this particular case we have a value that in practice could never used to be zero, but now can be. Turning it into a pointer is one way to emphasize that it's actually nullable from a JSON perspective.

Another pattern we could use would be something like sql.NullTime or sql.NullString. I went with a pointer here because we already use it on both request and response types in this library. It's particularly useful on PATCH requests where you need to distinguish between "I want to set this value to its zero value" vs "I don't want to touch this value".

Copy link
Collaborator Author

@oxtoacart oxtoacart Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsnet

Can't we just rely on time.Time.IsZero to indicate that ConnectedToControl is true?

You're asking whether we even need the ConnectedToControl field or should just infer it from LastSeen being empty? We went with an explicit field because it's more discoverable.

Copy link
Member

@raggi raggi Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We went with an explicit field because it's more discoverable.

Honestly most things users use this field for are harmful and I really strongly believe we should remove it because of that.

Copy link
Member

@raggi raggi Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two levels of documentation, one is the actual API documentation which I think is quite clear, the other is documenting the Go API wrapper itself, which could use some work.

I disagree that this is clear: it provides no guidance as to when this data is appropriate to consume and for what purposes. We have a long history of users making bad choices around that, including ourselves, despite explanations as to what the data is as is in the documentation there.

MachineKey string `json:"machineKey"`
NodeKey string `json:"nodeKey"`
OS string `json:"os"`
Expand Down
12 changes: 6 additions & 6 deletions devices_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func TestClient_Devices_Get(t *testing.T) {
Hostname: "test",
IsExternal: false,
ConnectedToControl: false,
LastSeen: Time{time.Date(2022, 3, 9, 20, 3, 42, 0, time.UTC)},
LastSeen: ptrTo(Time{time.Date(2022, 3, 9, 20, 3, 42, 0, time.UTC)}),
MachineKey: "mkey:test",
NodeKey: "nodekey:test",
IsEphemeral: false,
Expand Down Expand Up @@ -167,7 +167,7 @@ func TestClient_Devices_List(t *testing.T) {
IsEphemeral: false,
IsExternal: false,
ConnectedToControl: false,
LastSeen: Time{time.Date(2022, 3, 9, 20, 3, 42, 0, time.UTC)},
LastSeen: ptrTo(Time{time.Date(2022, 3, 9, 20, 3, 42, 0, time.UTC)}),
MachineKey: "mkey:test",
NodeKey: "nodekey:test",
OS: "windows",
Expand Down Expand Up @@ -242,9 +242,9 @@ func TestDevices_Unmarshal(t *testing.T) {
IsExternal: true,
KeyExpiryDisabled: true,
ConnectedToControl: false,
LastSeen: Time{
LastSeen: ptrTo(Time{
time.Date(2022, 4, 15, 13, 24, 40, 0, time.UTC),
},
}),
MachineKey: "",
Name: "hello.example.com",
NodeKey: "nodekey:30dc3c061ac8b33fdc6d88a4a67b053b01b56930d78cae0cf7a164411d424c0d",
Expand All @@ -270,9 +270,9 @@ func TestDevices_Unmarshal(t *testing.T) {
IsExternal: false,
KeyExpiryDisabled: true,
ConnectedToControl: false,
LastSeen: Time{
LastSeen: ptrTo(Time{
time.Date(2022, 4, 15, 13, 25, 21, 0, time.UTC),
},
}),
MachineKey: "mkey:30dc3c061ac8b33fdc6d88a4a67b053b01b56930d78cae0cf7a164411d424c0d",
Name: "foo.example.com",
NodeKey: "nodekey:30dc3c061ac8b33fdc6d88a4a67b053b01b56930d78cae0cf7a164411d424c0d",
Expand Down