Glance

v2 - replacing array elements with PATCH results in 400 error

Bug #1521607 reported by Jamie Hannaford on 2015-12-01

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	Glance	In Progress	Medium	Dharini Chandrasekar

Bug Description

I have the following image:

{
    "status": "active",
    "name": "foo",
    "tags": [
        "1",
        "3",
        "2"
    ],
    "container_format": "ami",
    "created_at": "2015-11-12T14:26:08Z",
    "size": 983040,
    "disk_format": "ami",
    "updated_at": "2015-12-01T12:25:42Z",
    "visibility": "public",
    "self": "/v2/images/386f0425-3ee8-4688-b73f-272328fe4c71",
    "min_disk": 20,
    "protected": false,
    "id": "386f0425-3ee8-4688-b73f-272328fe4c71",
    "architecture": "x86_64",
    "file": "/v2/images/386f0425-3ee8-4688-b73f-272328fe4c71/file",
    "checksum": "061d01418b94d4743a98ee26d941e87c",
    "owner": "057aad9fa85b4e29b23e7888000446ef",
    "virtual_size": null,
    "min_ram": 0,
    "schema": "/v2/schemas/image"
}

When I send this PATCH request to update it:

[{"op":"replace","path":"/tags/0","value":"10"}]

I get back the following 400 error:

"""
<html>
<head>
  <title>400 Bad Request</title>
</head>
<body>
  <h1>400 Bad Request</h1>
  Invalid JSON pointer for this resource: '/tags/0'<br /><br />

</body>
</html>
"""

"/tags/0" is a correct pointer, however, which should be supported in a "replace" op. Why doesn't it work?

Revision history for this message

Niall Bunting (niall-bunting) wrote on 2015-12-01:

This is what works for me:

[{"path": "/tags", "value": ["test"], "op": "replace"}]

Revision history for this message

Jamie Hannaford (jamie-hannaford) wrote on 2015-12-01:

That might be the case, but the fact still stands that

[{"path": "/tags/0", "value": "test", "op": "replace"}]

is a valid PATCH document, which is currently being rejected by the API. If Glance v2 supports the JSON patch standard (rfc6902), surely it should support all of it?

See this also:

https://github.com/json-patch/json-patch-tests/blob/master/tests.json#L155-L173

Revision history for this message

wangxiyuan (wangxiyuan) wrote on 2015-12-02:

In v2. image tags has its API : v2/images/image_id/tags/tag_value (PUT or DELETE).

I'm not sure image-update is the right way.

here are two way to fix IMO:

1. support "PATCH" "tags/0"

2.stop use PATCH to update tags.

Revision history for this message

Jamie Hannaford (jamie-hannaford) wrote on 2015-12-02:

I agree with the two options:

1. Implement JSON PATCH properly, allowing array elements to be updated individually. Perhaps you could use a library like https://github.com/stefankoegl/python-json-patch to do the heavy lifting for you?

2. Disable updating tags via PATCH. You would need to add `"readOnly": true` for that property in the schema. You'd also have to hope that there will never be arrays that can be updated for that object in the future.

My personal opinion is for #1. Partially implementing an IETF standard doesn't make sense to me.

Revision history for this message

Niall Bunting (niall-bunting) wrote on 2015-12-03:

I think the confusion is because there is no sub place for tags. Its like the name attribute you would not expect to do "/name/2" to get the second letter of the name.

This is the same with tags, it is a flat namespace. Therefore you can't get a sub part of a tag as it is all defined on the same level.

Revision history for this message

Jamie Hannaford (jamie-hannaford) wrote on 2015-12-07:

I'm not sure what you mean by "sub place" or "flat namespace" -- I think they're irrelevant terms here.

Yes, you would not access the name attribute with "/name/2" because it is not an array. The "tags" attribute, however, is -- so you would definitely expect to do that. It really shouldn't matter how Glance handles the array logic -- if you implement a standard, you should implement all of it or make the attribute `"readOnly": true` in the schema.

Ian Cordasco (icordasc) on 2016-01-11

Changed in glance:
status:	New → Triaged
importance:	Undecided → Medium

Revision history for this message

Ian Cordasco (icordasc) wrote on 2016-01-11:

I've reviewed this and I have to agree with Jamie. The problem with option 2, however, is backwards compatibility. We wouldn't be able to just shut that off entirely at the moment (not until we start working on API v3). That leaves me fairly certain that we need to either implement the entirety of JSON PATCH ourselves, or introduce a library which does this for us so we don't have to do that work.

I'll leave that decision to whomever decides to take this on.

Revision history for this message

Brian Rosmaita (brian-rosmaita) wrote on 2016-06-27:

This is an historical note.

Glance has never supported JSON patch. It supports a "useful and compatible subset of the functionality" of JSON patch [0].

Additionally, it does not support full JSON pointers.

The problem may be that at some point the API reference doc was rewritten with reference to JSON patch and JSON pointers, not with reference to what Glance actually supports.

That being said, it would be better if Glance fully supported RFC6092, using a standard library (as Glare is doing). But I think that would require a spec.

[0] http://specs.openstack.org/openstack/glance-specs/specs/api/v2/http-patch-image-api-v2.html

Revision history for this message

Jamie Hannaford (jamie-hannaford) wrote on 2016-06-28:

Well, if I'm being completely honest, it was not compatible. I don't understand why a project would want to implement a subset because a cognitive burden is placed on end-users to not only understand JSON PATCH (which is very complicated to begin with), but also on the delta between what's in the spec and what Glance pick and mixes for its codebase.

Because things don't work as expected, and because of this cognitive burden, I didn't extract any value out of the implementation (it certainly wasn't useful for me). I think that if projects want to use JSON PATCH, that's fine, but they must implement it wholly. Otherwise you're introducing a great deal of uncertainty and complexity into something just for implementation reasons, which is irrelevant to end-users.

Dharini Chandrasekar (dharini-chandrasekar) on 2016-10-18