amimof / huego

An extensive Philips Hue client library for Go with an emphasis on simplicity
MIT License
250 stars 36 forks source link

`omitempty` on boolean attributes prevents changing them to `false` #45

Open aleksejspopovs opened 3 years ago

aleksejspopovs commented 3 years ago

Some boolean attributes are annotated with json:"attributename,omitempty", which means they are omitted from requests when set to the default value for their type, i.e. false. This means that it is impossible to set a boolean attribute to false using a Create or Update method.

AutoDelete in Schedule is one example of an affected attribute. It defaults to true when not provided, and there currently appears to be no way in huego to set it to false.

Here's a quick program that reproduces this:

package main

import (
    "fmt"
    "encoding/json"
    "github.com/amimof/huego"
)

func main() {
    bridge := huego.New("192.168.0.221", "[redacted]")

    schedule := huego.Schedule{
        Name: "Test schedule",
        Command: &huego.Command{
            Address: "/api/0/lights/4/state",
            Method: "PUT",
            Body: map[string]bool{
                "on": true,
            },
        },
        LocalTime: "PT00:00:03",
        AutoDelete: false,
    }

    schedule_json, err := json.Marshal(schedule)
    if err != nil {
        fmt.Printf("error: %v\n", err)
        return
    }

    fmt.Printf("JSONd schedule looks like this:\n%v\n", string(schedule_json))

    response, err := bridge.CreateSchedule(&schedule)
    if err != nil {
        fmt.Printf("error: %v\n", err)
        return
    }

    fmt.Printf("response: %v\n", response)
}

and here's what it outputs on my machine:

JSONd schedule looks like this:
{"name":"Test schedule","description":"","command":{"address":"/api/0/lights/4/state","method":"PUT","body":{"on":true}},"localtime":"PT00:00:03"}
response: &{map[id:2]}

Checking the Bridge API manually confirms that this schedule has autodelete: true, and three seconds later it's gone.

jtyr commented 2 years ago

This is the fundamental issue with this Golang implementation of of the Hue API. It doesn't allow to update only specific values - it always sending all fields of the struct type. Good example is the Config type that is used to reading the config from the bridge (that works) as well as to modify values on the bridge (doesn't work). For example, it's impossible to set the bridge's name:

bridge := huego.New("192.168.x.y", "asdfasdf...")

c := &huego.Config{
    Name: "My Bridge",
}

// This fails because we are pushing fields that are not modifiable (e.g. bridgeid)
_, err := bridge.UpdateConfig(c)
if err != nil {
    panic(err)
}

The related test is therefore fundamentally invalid as you cannot push the same data as you read from the bridge.

In order to fix this, there should be two types - one for read, one for write. The one for write should use *string instead of string to have the default value as nil, that will be filtered by the JSOM marshaling. Like this you will always send only fields that were explicitly set by the user.

amimof commented 2 years ago

@aleksejspopovs @jtyr

Thanks for your input. This is actually something that has been pointed out several times and I'm working on finding a solution without breaking compatibility. One solution is to have each struct implement a json marshaler which adds missing fields. For example in schedule.go this would look like this:

func (s *Schedule) MarshalJSON() ([]byte, error) {

    type Alias Schedule
    d, err := json.Marshal(&struct{
        *Alias
        AutoDelete *bool    `json:"autodelete,omitempty"`
    }{
        Alias: (*Alias)(s),
        AutoDelete: &s.AutoDelete,
    })
    if err != nil {
        return nil, err
    }

    return d, nil
}

And then in UpdateSchedule in bridge.go:

data, err := schedule.MarshalJSON()
if err != nil {
    return nil, err
}

Would love to hear your thoughts on this

jtyr commented 2 years ago

@amimof Plese see PR https://github.com/amimof/huego/pull/49 for possible solution.