Switch from add to replace/update properties #32

lornajane · 2024-03-30T16:45:11Z

I'm proposing this change as a response to the ongoing discussion in #30 . Using "add" can be confusing and leaves us without being able to support some use cases, such as setting an array to a particular set of elements. Looking around for similar-ish approaches, we found that JSONPath uses "update" for our existing "add" operation, and "replace" to achieve another sort of adding something. I've therefore proposed a specification update to adopt the new terminology.

It's a big change and while sooner is of course the best time to be making these changes, I'd appreciate as many eyes and as much feedback as possible. Comments and questions welcome.

philsturgeon · 2024-04-02T12:53:35Z

Typo in lorna's message there, she meant to say:

we found that JSON Patch

Basically nicking the relevant and useful concepts from JSON Patch RFC 6902 and their list of operations: https://www.rfc-editor.org/rfc/rfc6902.html#section-4

versions/1.0.0.md

philsturgeon · 2024-04-02T12:59:38Z

versions/1.0.0.md

@@ -111,10 +111,11 @@ This object represents one or more changes to be applied to the target document

 Field Name | Type | Description
 ---|:---:|---
-<a name="actionTarget"></a>target | `string` | **REQUIRED** A JSONPath query expression referencing the target objects in the target document.
-<a name="actionDescription"></a>description | `string` | A description of the action. [CommonMark syntax](https://spec.commonmark.org/) MAY be used for rich text representation.
-<a name="actionUpdate"></a>update | Any | 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`.


Is it worth keeping update aroud for a minute as deprecated or is this early and experimental enough that whatever?

This is always a hard decision to make. At this point in time, I'd vote to just kill it. We won't always have this luxury, but we should make use of it while we can.

We'll always have the commit history, but this does make me wonder if we should propose a 0.1.0 version and then start following a more formal release process. We're already seeing usage ....

.... Alternatively, how about moving forward from our current situation by making this a 1.1.0 and still a draft?

Question retracted, you folks came up with a god solution on the call and I forget what it was, but I'm not standing in the way of anything 🫡

This proposal will definitely be 2.0 in recognition of how breaking it is.

kevinswiber · 2024-04-02T19:27:11Z

Really appreciate having some parallels with JSON Patch!

versions/1.0.0.md

Thanks! Co-authored-by: Darrel <[email protected]>

kevinswiber

LGTM

kevinswiber · 2024-06-18T19:02:26Z

versions/1.0.0.md

@@ -111,10 +111,11 @@ This object represents one or more changes to be applied to the target document

 Field Name | Type | Description
 ---|:---:|---
-<a name="actionTarget"></a>target | `string` | **REQUIRED** A JSONPath query expression referencing the target objects in the target document.


I noticed that anchors are removed. I think they don't quite work well with GitHub's formatting, anyway, but it's worth pointing out. Not sure if this is used with other tooling. Not a blocker.

lornajane · 2024-06-20T12:56:16Z

@kevinswiber Thanks for the approval, but do you have any thoughts on how to version this in a way that won't confuse existing users? That's what got me really stuck.

kevinswiber · 2024-06-20T19:28:25Z

@kevinswiber Thanks for the approval, but do you have any thoughts on how to version this in a way that won't confuse existing users? That's what got me really stuck.

@lornajane Seeing as how Overlays has been in a pre-release phase, I'm not compelled to allow for backwards-compatibility. Breaking backwards-compatibility has a negative impact on early users but a positive impact on future users. I look at an example of a project that made this hard decision, Node.js. Before their first major release, they'd break backwards compatibility all the time. This was becoming more difficult as adoption was growing. In retrospect, I believe it was the right move. It led to greater stability when even more users were being impacted by changes.

I believe @ThomasRooney has a different perspective worth including in the conversation, as they're dealing with existing users.

ThomasRooney · 2024-06-20T21:40:24Z

I believe @ThomasRooney has a different perspective worth including in the conversation, as they're dealing with existing users.

Thanks for asking. My persective is that making a breaking change without a corresponding version change is always bad, the moment that specification has been adopted. I understand that this specification remains a "draft", but it is also:

Over 3 years old.
Already in use by a non-trivial number of organisations, given it solves a significant existing need. Whilst a few of these organisations (e.g. Speakeasy, Bump) work in public, through my work with Speakeasy I can definitely state that there are a number of organisations that have also adopted the overlay specification internally that would also be affected.

There's nothing wrong with breaking changes, but I strongly feel that if a breaking change is deemed important, it would be kinder to:

Leave the current document as 1.0.0.
Make any breaking changes in 2.0.0

Or, alternatively:

Reduce the current specification version to 0.1.0.
Make any breaking changes wanted in 0.2.0.

That way, any of the organisations that manage documents with overlay: 1.0.0 get an easy migration path: we'd just automatically alter the version of any overlay documents we interact with from 1.0.0 to 0.1.0. This pathway would remain viable for however many months (years?) it'll take to make a non-draft 1.0.0.

I personally think the change from update to add isn't actually a positive one anyway, because:

An openapi overlay is data-format-agnostic. I.e. it doesn't just apply to JSON documents, but to whatever format an openapi document is kept in. This, whilst primarily YAML, also includes pkl, cue, as well as in-memory representations. Hence I have a bit of aversion to leaning on JSON Patch add nomenclature to respresent the "shallow merge + array append" that is actually happening.
Probably the most common thing we see overlays used for is overriding something like description. This is primarily by a DX/technical writing team trying to produce a good documentation site or SDK type info that's produced downstream of the OAS. Whilst this is usually done by a shallow merge of the parent object, we've also seen target be applied to the scalar string value. add feels ambiguous when applied to scalars (strings, numbers), whereas update does not. E.g. consider the case where a description node is targetted to be overridden, add sounds to me like it should append to the existing string, rather than override it.

For the underlying problem:

In my opinion, adding a replace action would be a nice bit of syntactic sugar to reduce ambiguity, especially for cases where an array item is being replaced.
Would also love to have a move action for something like adding a oneOf (or converting a oneOf into an allOf/anyOf) in a JSON Schema. This is a relatively common action for us as we help represent different use-cases with more constrained json schemas to give better DX for our customers APIs, but we currently risk a higher degree of brittleness to do this with update overrides.

handrews · 2024-06-20T22:40:23Z

@ThomasRooney

An openapi overlay is data-format-agnostic. I.e. it doesn't just apply to JSON documents, but to whatever format an openapi document is kept in. This, whilst primarily YAML, also includes pkl, cue, as well as in-memory representations.

This is a good point to support my recent proposal that OAS 4.0 "Moonwalk" should define an in-memory structure and how it can (and can't) be manipulated, so that we're less format-dependent for programmatic editing. Which doesn't change anything for this PR given how long it will be before 4.0 is a practical concern. But I feel like the Overlays project should impact that design.

ralfhandl · 2024-09-02T11:53:41Z

versions/1.0.0.md

+      - description: Test server
+        url: https://testing.example.com/
+
+Array elements MAY be deleted using the `remove` property.  Use of array indexes to remove array items should be avoided where possible as indexes will change when items are removed.


What is the recommended way of removing certain array elements?

None of them are really great, honestly. We should put some examples in the learn site.

ralfhandl · 2024-11-01T08:48:38Z

Needs to be retargeted after setting up the 2.0 dev branch

lornajane added 2 commits March 30, 2024 16:27

Update overlays spec to use add/replace action names

a4fa9c6

Add an example of replacing an array

4d96e95

lornajane mentioned this pull request Mar 30, 2024

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

Open

philsturgeon reviewed Apr 2, 2024

View reviewed changes

darrelmiller reviewed Apr 13, 2024

View reviewed changes

versions/1.0.0.md Outdated Show resolved Hide resolved

lornajane and others added 2 commits May 6, 2024 18:34

Update versions/1.0.0.md

af6605e

Thanks! Co-authored-by: Darrel <[email protected]>

Fix the description of replace

f4a0658

lornajane mentioned this pull request May 6, 2024

Example showing setting default response #17

Draft

lornajane requested a review from kevinswiber June 18, 2024 17:05

kevinswiber approved these changes Jun 18, 2024

View reviewed changes

ralfhandl reviewed Sep 2, 2024

View reviewed changes

ralfhandl added this to the Release 2.0 milestone Sep 9, 2024

ralfhandl marked this pull request as draft November 1, 2024 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from add to replace/update properties #32

Switch from add to replace/update properties #32

lornajane commented Mar 30, 2024

philsturgeon commented Apr 2, 2024 •

edited

Loading

philsturgeon Apr 2, 2024

kevinswiber Apr 2, 2024

lornajane May 6, 2024

lornajane May 6, 2024

philsturgeon Sep 2, 2024

lornajane Sep 2, 2024

kevinswiber commented Apr 2, 2024

kevinswiber left a comment

kevinswiber Jun 18, 2024

lornajane commented Jun 20, 2024

kevinswiber commented Jun 20, 2024

ThomasRooney commented Jun 20, 2024 •

edited

Loading

handrews commented Jun 20, 2024

ralfhandl Sep 2, 2024

lornajane Sep 2, 2024

ralfhandl commented Nov 1, 2024

Switch from add to replace/update properties #32

Are you sure you want to change the base?

Switch from add to replace/update properties #32

Conversation

lornajane commented Mar 30, 2024

philsturgeon commented Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinswiber commented Apr 2, 2024

kevinswiber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lornajane commented Jun 20, 2024

kevinswiber commented Jun 20, 2024

ThomasRooney commented Jun 20, 2024 • edited Loading

handrews commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralfhandl commented Nov 1, 2024

philsturgeon commented Apr 2, 2024 •

edited

Loading

ThomasRooney commented Jun 20, 2024 •

edited

Loading