Switch from add to replace/update properties

[lornajane]

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]

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

[philsturgeon]

Is this right? It says the values will be merged into the target then it the values will be replaced in the target.

[darrelmiller]

Agreed, the first sentence for replace seems to contradict the second.

[lornajane]

Updated, could you check this makes more sense?

[philsturgeon]

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

[kevinswiber]

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.

[lornajane]

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 ....

[lornajane]

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

[kevinswiber]

Really appreciate having some parallels with JSON Patch!

[kevinswiber]

LGTM

[kevinswiber]

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]

@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]

@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]

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:

1. Over 3 years old.
2. 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:

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

Or, alternatively:

1. Reduce the current specification version to 0.1.0.
2. 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:

1. 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.
2. 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:

1. 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.
2. 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]

@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.