Skip to content
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.

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

omitempty on boolean attributes prevents changing them to false #45

Open
aleksejspopovs opened this issue Jul 4, 2021 · 3 comments
Open

Comments

@aleksejspopovs
Copy link

aleksejspopovs commented Jul 4, 2021

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
Copy link

jtyr commented Oct 13, 2021

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
Copy link
Owner

amimof commented Oct 15, 2021

@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
Copy link

jtyr commented Oct 15, 2021

@amimof Plese see PR #49 for possible solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants