-
Notifications
You must be signed in to change notification settings - Fork 15
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
Comments
This is is a great question, and I think we don't have this use case covered. The only options are to
I'm not sure if we should add a |
@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? |
@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. |
So RFC 6902 does have |
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 One approach was this:
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.
which produces this:
|
Ahaha I just figured mine out:
Might feel a bit weird but its exactly what I wanted. |
I've tried your first approach ( servers:
- description: Production
url: 'https://api.protect.earth/' I think it feels easier (both to write and to read) than your |
@paulRbr to be clear using
produces
but after updating to the latest and greatest bump-cli 2.8.0 it produces this:
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. |
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:
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 So after thinking about this, I believe the Overlay specification needs a new |
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:
Other examples will certainly be less easily sidestepped. |
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 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!) - 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 |
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.
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 |
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:
The other operation that we might want to support is a sort of |
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.
I guess if you
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"
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. |
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! |
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
@hkosova 's example could be addressed like this.
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:
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 BTW, it is nice to see activity here again. |
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. |
Thanks! I'm interested to see how that shapes up. |
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:
Overlay:
The Action Object section says:
I understand this as
x-foo
being the "merge object",x-bar
being a "property in the merge object", so thex-bar
value from the overlay should replace (and not be appended to) thex-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.
The text was updated successfully, but these errors were encountered: