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

How to replace the value list of an array? #30

Open
hkosova opened this issue Feb 8, 2024 · 18 comments
Open

How to replace the value list of an array? #30

hkosova opened this issue Feb 8, 2024 · 18 comments

Comments

@hkosova
Copy link

hkosova commented Feb 8, 2024

I want to replace the value list of an array with a new list of values. However, the two overlay implementations I've tried treat the following example as "append to an array" rather than "replace an array".

I'm not sure if the implementation behavior is correct or a bug, so wanted to confirm the intended behavior with the spec authors.


OpenAPI document:

openapi: 1.0.0
info:
  title: Array replacement test
  version: 1.0.0
paths: {}
x-foo:
  x-bar:  # I want to replace all values of this array
    - 1
    - 2

Overlay:

overlay: 1.0.0
info:
  title: Array replacement test
  version: 0.0.1
extends: array-test.yaml

actions:
  - target: $['x-foo']
    update:
      x-bar:
        - 3
        - 4

The Action Object section says:

When the Overlay document is applied, the properties in the merge object replace properties in the target object with the same name

I understand this as x-foo being the "merge object", x-bar being a "property in the merge object", so the x-bar value from the overlay should replace (and not be appended to) the x-bar value in the document.


The workaround is to remove the array node then re-add it with the new values. This seems a bit verbose, but if that's the intended way to replace arrays then I'm OK with it.

actions:
  - target: $['x-foo']['x-bar']
    remove: true

  - target: $['x-foo']
    update:
      x-bar:
        - 3
        - 4
@lornajane
Copy link
Contributor

This is is a great question, and I think we don't have this use case covered. The only options are to remove or to update where update is described like this:

An object with the properties and values to be merged with the object(s) referenced by the target. This property has no impact if remove property is true.

I'm not sure if we should add a replace option to cover this case. Since the spec specifically states that the list of actions is ordered and must be performed sequentially, the approach you describe of deleting and then updating is the best way to do it now (whether we add replace or not).

@handrews
Copy link

@lornajane apologies if this has already been discussed as this is the first time I'm looking at Overlays, but have you looked at the JSON Patch (RFC 6902) operation set?

@lornajane
Copy link
Contributor

@handrews Thanks for the link, I will dig into it! Honestly, I've no idea about anything that might have previously been considered or discussed, I'm not an overlays spec contributor, I came to it late but I seem to be its most active user at the moment.

@lornajane
Copy link
Contributor

So RFC 6902 does have replace - our remove seems quite similar to theirs, and our update matches their add from what I can tell. Let's keep this open and see if we get a lot of interest in adding a replace operation.

@philsturgeon
Copy link

philsturgeon commented Mar 12, 2024

I was wondering about this too. My use case is ditching all the defined servers and putting my own in, because the source generic software has https://example.com and I want each client using this software to customise their OpenAPI and have their own servers.

One approach was this:

overlay: 1.0.0
info:
  title: Hide Non-Production Servers
  version: 0.0.1
actions:
  - target: '$.servers.*'
    description: Remove all other servers so we can add our own.
    remove: true

  - target: '$.servers'
    description: Pop our server into the empty server array.
    update:
      - description: Production
        url: https://api.protect.earth/

Using the Bump CLI (which is based on your excellent openapi-overlays-js repo) produces this:

servers:
  '0':
    description: Production
    url: 'https://api.protect.earth/'

Last time I had a problem you told me I was doing it wrong and chances are high I'm also doing this wrong, but I'd love to know how to do it properly.

I settled for udating all the entries and that'll be fine this time because I know there's only one, but it could get weird in other uses.

  - target: '$.servers.*'
    description: Pop our server into the empty server array.
    update:
      description: "Production"
      url: https://api.protect.earth

which produces this:

servers:
  - url: 'https://api.protect.earth'
    description: Production

@philsturgeon
Copy link

Ahaha I just figured mine out:

  - target: '$.servers[:1]'
    description: Pop our server into the empty server array.
    update:
      description: "Production"
      url: https://api.protect.earth

  - target: '$.servers[1:]'
    description: Remove all the other servers.
    remove: true

Might feel a bit weird but its exactly what I wanted.

@paulRbr
Copy link

paulRbr commented Mar 18, 2024

Using the Bump CLI (which is based on your excellent openapi-overlays-js repo) produces this:

servers:
  '0':
    description: Production
    url: 'https://api.protect.earth/'

I've tried your first approach (remove + update) and it produces the following (with the Bump.sh CLI):

servers:
  - description: Production
    url: 'https://api.protect.earth/'

I think it feels easier (both to write and to read) than your [:1] and [1:] array manipulation. Why aren't you pleased with your first approach @philsturgeon?

@philsturgeon
Copy link

@paulRbr to be clear using bump-cli/2.7.3-beta with this pair of actions:

  - target: '$.servers.*'
    description: Remove all other servers so we can add our own.
    remove: true

  - target: '$.servers'
    description: Pop our server into the empty server array.
    update:
      - description: Production
        url: https://api.protect.earth/

produces

servers:
  '0':
    description: Production
    url: 'https://api.protect.earth/'

but after updating to the latest and greatest bump-cli 2.8.0 it produces this:

servers:
  - description: Production
    url: 'https://api.protect.earth/'

Dont know whether one of the Bump team fixed it or @lornajane did, but this is of course much better and I no longer need my awkward workaround.

Assuming this behavior is expected and correct to the specification etc.

@paulRbr
Copy link

paulRbr commented Mar 19, 2024

Here are my evening thought on the issue of @hkosova, and the few comments between @handrews and @lornajane about JSON patch RFC.

In the case of this current Overlay specification, we have:

  • The update field of the Action object is perfect to merge/update existing target objects
  • The remove field of the Action object, is useful when you just want to remove an object from your target document

But what happens when you want to add something? It can either be in the sense of replacing (as described by @hkosova in this issue) or in the sense of adding a new object (to a target object that doesn't necessarily exists).

Here's an example overlay action:

- target: "$.info.description"
  update: |
    Provide a long human description in my target document

And an example target document:

info:
  version: 1.0.0
  title: "Travel API"

The specification doesn't make it clear whether the update action should add the description: field in the target document (under the info: object), or whether it shouldn't do anything (because the path was not found in the target document).

So after thinking about this, I believe the Overlay specification needs a new add action to be able to differentiate merging/updating from adding/replacing.

@philsturgeon
Copy link

In this particular example the JSONPath is pointing to something that does not exist, and I would expect it to silently fail or throw a warning because that was not found. This would have the desired effect:

- target: "$.info"
  update:
    description: |
      Provide a long human description in my target document

Other examples will certainly be less easily sidestepped.

@paulRbr
Copy link

paulRbr commented Mar 20, 2024

In this particular example the JSONPath is pointing to something that does not exist

Good point I didn't think about that.

Ok so, let me find another example where I think another action type is needed (on top of the example on arrays provided by @hkosova)

- target: "$.info"
  update:
    description: |
      Provide a long human description in my target document

And an example target document:

info:
  version: 1.0.0
  title: "Travel API"

I understand the update action will merge both Info objects resulting in the overlayed document:

  info:
    version: 1.0.0
    title: "Travel API"
    description: |
      Provide a long human description in my target document

But, if I want to replace the whole initial Info object? Wouldn't it be interesting to have a (fictive, this doesn't exist yet!) replace action, such as:

- target: "$.info"
  replace:
    description: |
      Provide a long human description in my target document

Which applied to the same target document, would produce this result:

  info:
    description: |
      Provide a long human description in my target document

@philsturgeon
Copy link

Examples are tricky because they can always be picked apart, but that particular example of modiying content in the info property could be considered "greedy". It's removing a whole lot of stuff without knowing exactly what is there, and if the intent was to remove other things and add the new description, then two specific actions might be more intentional.

Going back to the original example from @hkosova, I definitely see the need for replacing a list, without needing to remove everything first.

actions:
  - target: $['x-foo']['x-bar']
    remove: true

  - target: $['x-foo']
    update:
      x-bar:
        - 3
        - 4

This feels a lot like the difference of PUT and PATCH, sometimes you want to provide the source of turth and have everything not mentioned removed, and sometimes you want to sprinkle in some new bits to coexist with whats there.

The keywords could perhaps be merge and replace to be more specific than update which is often used to mean both.

@lornajane
Copy link
Contributor

I think there's grounds for depending on an existing standard, Henry suggested that JSON Patch has similar operations (I think it postdates the original overlays draft) - a side benefit might be that there's tooling around to do this type of operation that overlays tooling can build on? I also don't think the change is too drastic in comparison to what we have:

  • remove is unchanged
  • update becomes add (does this always make sense? Need to look at existing examples)
  • replace gets added

The other operation that we might want to support is a sort of weakly-write thing so you can write values if they don't exist, but you don't clobber anything - there's discussion on this as a "default" operation in #17 that I'd appreciate more eyes/feedback on.

@philsturgeon
Copy link

philsturgeon commented Mar 25, 2024

Definitely a fan of leaning on existing standards. Taking at least remove, add, replace from JSON Patch operations list would be great, and of move and copy end up in that seems fine (good probably).

There's a lot of smallprint on how those work, especially add seems a bit confusing.

4.1. add

The "add" operation performs one of the following functions,
depending upon what the target location references:

o If the target location specifies an array index, a new value is
inserted into the array at the specified index.

I guess if you add to JSON Path $.foo[1] then thats "adding something into the array at this index which actually means replacing it if theres something there"?

o If the target location specifies an object member that does exist,
that member's value is replaced.

For object members an add becomes a replace for sure.

There are some conceptual differences with the JSON Patch target location and the JSON Path used by overlays, which is that it uses the target location to define keys which should be added to, and JSON Path would simply say "that doesnt exist, i aint running it"

For example, an "add" with a target location of "/a/b" starting with this
document:

{ "a": { "foo": 1 } }

is not an error, because "a" exists, and "b" will be added to its
value. It is an error in this document:

{ "q": { "bar": 2 } }

The way JSON Path is used by most tools I've seen (Spectral and Overlay based mainly) you are pointing it to a thing that exists, then doing something with it, not using it to define a array index/object member in an existing JSON instance if that does not exist. I don't know if this would make it harder to use existing JSON Path tooling?

Anyhow, conceptually sounds good to base the actions on JSON Patch whether thats tightly copied or "loosly inspired by".

Either way, +1 to "remove, update, replace" as defined in your above list.

@lornajane
Copy link
Contributor

In an attempt to keep this moving, I've opened a proposed change to the specification in #32. Some reviews and engagement (even if it's just a +1) from everyone who has already chimed in on this thread would help!

@darrelmiller
Copy link
Member

darrelmiller commented Apr 13, 2024

I will try and provide a little bit of historical context, as best as I can remember it. We definitely did consider the operations provided by JSON Patch. I had experience with it having written a library for it many moons ago. We did used to have two operations, 'add' and 'merge'. The PR when we removed them is here #4

I believe we removed them because we identified that we didn't need to make the distinction. I think the idea was that if you target an array with an update value then we add to the array.

That's what this example shows

overlay: 1.0.0
info:
  title: Add an array element
  version: 1.0.0
actions:
- target: $.paths.*.get.parameters
  update:
    name: newParam
    in: query

@hkosova 's example could be addressed like this.

overlay: 1.0.0
info:
  title: Array replacement test
  version: 0.0.1
extends: array-test.yaml

actions:
  - target: $.x-foo.x-bar
    update: 3
  - target: $.x-foo.x-bar
    update: 4

There is an argument for saying why can't update be an array to add multiple elements. However, what happens if you want to add an array as a single element to an array?

This changes @philsturgeon 's scenario to be:

  - target: '$.servers.*'
    description: Remove all other servers so we can add our own.
    remove: true

  - target: '$.servers'
    description: Pop our server into the empty server array.
    update:
        description: Production
        url: https://api.protect.earth/

Note that the value of the update member is now an object, not an array. This is because you want to add the object to the array, not an array to an array.

I AM NOT saying where we left the spec is the best solution. It was the solution that we had landed on when we all moved on.

I don't have any objection to bringing the add back for some additional clarity. However, I would call out Phil's point that the the JSONPath target is different than the JSONPointer based add that is used by JSON Patch. So, while you may use the same names as JSON Patch, the behaviour would be subtly different.

It is an interesting alternative to bring the merge semantics into the add method and make the replace a complete replacement. That would eliminate the need for people doing remove and then add.

BTW, it is nice to see activity here again.

@darrelmiller
Copy link
Member

Here is an argument for being able to clearly distinguish between an add and an update. Speakeasy's implementation of OpenAPI has a compare function that takes two OpenAPI descriptions and emits an Overlay that would convert one into the other. Having Overlays as a kind of official diff format is an interesting proposition and knowing from the Overlay whether something is being added vs updated would be very helpful for a diff format.
Having said, that I'm not all that sure how good Overlays are as a semantically rich diff format.

@lornajane
Copy link
Contributor

Thanks! I'm interested to see how that shapes up.

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

6 participants