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

Map Merging Preventing Updates #323

Open
Linuturk opened this issue Nov 14, 2018 · 6 comments
Open

Map Merging Preventing Updates #323

Linuturk opened this issue Nov 14, 2018 · 6 comments

Comments

@Linuturk
Copy link

Linuturk commented Nov 14, 2018

I have a struct similar to the following:

type Device struct {
	UUID gocql.UUID `json:"-" db:"uuid"`
	Name string     `json:"name" db:"name"`
	Tags map[string]string `json:"tags" db:"tags"`
}

I've satisfied the FindOne, Update, Delete, and Create methods on this type. I can successfully create, delete, and read a device, but I'm having a problem with the Tags field when Updates are processed.

func (d *Device) Update(obj interface{}, r api2go.Request) (api2go.Responder, error) {

	// assert a device
	device := obj.(*Device)

	// check the device type
	if !device.ValidType() {
		return api2go.Response{}, api2go.NewHTTPError(nil, "Invalid device type given", http.StatusBadRequest)
	}

	// update the device in the database
	err := DeviceTable.UpdateOnex(device)
	if err != nil {
		return api2go.Response{}, api2go.NewHTTPError(err, "Failed to update device", http.StatusInternalServerError)
	}

	return api2go.Response{Code: http.StatusNoContent}, nil
}

Both the Device "d" and the "obj" provided by api2go contain the updated fields. Unfortunately, api2go is pulling any existing data from my database, and overwriting that struct with the struct provided by the client. For example, if the database contains:

{
    "data": {
        "type": "devices",
        "id": "09631dba-c3f9-11e7-88e5-bb32f9c079f7",
        "attributes": {
            "name": "Static Test Device",
            "tags": {
                "building": "1",
                "lab": "1A",
                "rack": "7",
                "shelf": "3",
                "foo": "bar"
            }
        }
    }
}

and I send an update with this data:

{
    "data": {
        "type": "devices",
        "id": "09631dba-c3f9-11e7-88e5-bb32f9c079f7",
        "attributes": {
            "name": "Static Test Device Updated",
            "tags": {
                "building": "E",
                "lab": "Dark Room",
                "rack": "5",
                "shelf": "2"
            }
        }
    }
}

The tags section get merged instead of overwritten. I end up with:

{
    "data": {
        "type": "devices",
        "id": "09631dba-c3f9-11e7-88e5-bb32f9c079f7",
        "attributes": {
            "name": "Static Test Device Updated",
            "tags": {
                "building": "E",
                "lab": "Dark Room",
                "rack": "5",
                "shelf": "2",
                "foo": "bar" <<<<< shouldn't be there
            }
        }
    }
}

I've confirmed this by checking the obj and d variables before I do any database interactions. This might be related to #148

Is there any way I can prevent that map from being merged instead of overwritten?

@sharpner
Copy link
Member

Hm that's does not look good. Out of the box I know no way around it. I think for a quick win we need to implement the possibility for the user to change the "merging" behaviour. I see if I can get to it soon.

@Linuturk
Copy link
Author

👍 Looking forward to it.

@Linuturk
Copy link
Author

Linuturk commented Apr 12, 2019

Looks like the root of the problem is this line from the json.Unmarshal documentation:

To unmarshal a JSON object into a map, Unmarshal first establishes a map to use. If the map is nil, Unmarshal allocates a new map. Otherwise Unmarshal reuses the existing map, keeping existing entries. Unmarshal then stores key-value pairs from the JSON object into the map. The map's key type must either be a string, an integer, or implement encoding.TextUnmarshaler.

I can see this being in the code:

	// we have to make the Result to a pointer to unmarshal into it
	updatingObj := reflect.ValueOf(obj.Result())
	if updatingObj.Kind() == reflect.Struct {
		updatingObjPtr := reflect.New(reflect.TypeOf(obj.Result()))
		updatingObjPtr.Elem().Set(updatingObj)
		err = jsonapi.Unmarshal(ctx, updatingObjPtr.Interface())
		updatingObj = updatingObjPtr.Elem()

Based on that, one approach might be to check each struct field to see if it's a map, and if so, set the map to nil before the json.Unmarshal call? That seems a little hairy, but I'm not seeing a way to modify the json.Unmarshal behavior.

Actually, we should probably nil the maps coming from the FindOne() call instead.

@bbergshaven
Copy link

Hello @Linuturk and @sharpner , did you guys find any solution for this? We're also using this project in production and have run into the same problem.

Ps. Thanks for all your effort!

@sharpner
Copy link
Member

@bbergshaven no sorry, there was no change in that regard - I still think the correct way to fix this, is to let the user manually implement/overwrite the merging behavior, what do you think?

@bbergshaven
Copy link

Thanks for the swift reply.
After looking into the problem we ended up implementing a custom UnMarshaller on the specific type to solve the problem.

// Foo is the struct to be encoded.
type Signal struct {
	ID          string              `db:"id,omitempty"`
	Name        string              `db:"name" json:"name"`
        EnumValues  EnumMap             `db:"enum_values,omitempty" json:"enum-values"`
        Children    bool                `json:"children"`
}

// EnumMap enables custom JSON Unmarshalling to prevent PATCH bugs in API2GO.
// See: https://github.com/manyminds/api2go/issues/323.
type EnumMap map[int]string

// UnmarshalJSON implements the JSON Unmarshal interface for EnumMap.
func (m *EnumMap) UnmarshalJSON(data []byte) error {
	*m = nil
	return json.Unmarshal(data, (*map[int]string)(m))
}

This seems to work great and solve our problems.

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