Skip to content

Overlay proposal #1864

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

Merged
merged 4 commits into from
Jan 2, 2020
Merged

Overlay proposal #1864

merged 4 commits into from
Jan 2, 2020

Conversation

darrelmiller
Copy link
Member

@darrelmiller darrelmiller commented Mar 14, 2019

To improve the process around working on the overlay feature, the contents of the Overlay issue have been moved into a markdown file that lives in the proposals folder in the master branch. The goal is to bring this document to the point where it can be copied and pasted into the official spec when finalized. For overlays, this may be a different spec document than the OpenApi spec document.

@MikeRalphson
Copy link
Member

Sorry to bikeshed, but I'm not really a fan of the name proposals for this directory. For one thing the contents are not named for the version of the spec. they apply to and for another I'm unclear as to the lifetime of the contents. Is it intended to retain proposals indefinitely in this directory even once they have progressed to the spec. proper, to avoid breaking people's links?

@darrelmiller
Copy link
Member Author

@MikeRalphson Your bikeshedding is welcome and you raise valid questions. We should address them and change the name appropriately. I just didn't want to block the other work while we debated the name.

@namdeirf
Copy link
Contributor

Sorry I have been out of pocket for a while. I am looking into the specifics of the Overlay proposal and can't find it here https://github.com/OAI/OpenAPI-Specification/tree/master/proposals . The alternative schema proposal is there, and the template for future proposals, but I don't see overlays. Sorry if I missed it.

@darrelmiller
Copy link
Member Author

@namdeirf The proposal is now there.

@MikeRalphson The proposals folder is intended to be long lived. It serves as a place to keep a record of why the change happened and the scenarios that it intends to address. Your point is valid that we should have metadata that indicates what version of the spec the proposal is targeted. At least the major version, maybe the minor but not the patch. In the future we could decide to create subfolders for spec versions to partition the proposals. However, I'm not sure that is desirable. Someone creating a proposal may not be aware of the future version timeline and the proposal may take a number of released versions before it is integrated.

I apologize for not addressing this issue earlier, I had seen your comment and then lost where it was until I cam to merge overlays.

Copy link
Member

@MikeRalphson MikeRalphson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor points, but otherwise LGTM.

@darrelmiller darrelmiller merged commit 4eee801 into master Jan 2, 2020
@darrelmiller darrelmiller deleted the overlays branch January 2, 2020 16:53
Copy link

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great proposal, Darrel. I just realized this PR is merged but I'm leaving my comment here in case it helps.

version: 1.0.0
updates:
- target: "@"
value:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad indentation (it's repeated on every example).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequences (aka arrays) don't need to be indented https://yaml.org/spec/1.2/spec.html#id2802662

Since people perceive the “-” indicator as indentation, nested block sequences may be indented by one less space to compensate
https://yaml.org/spec/1.2/spec.html#id2799181

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, I didn't mean the - target but the indentation on value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dammit. Sorry about that. I'll fix it.


#### <a name="overlayDocument"></a>Overlay Document

An overlay document contains a list of [Update Objects](#overlayUpdates) that are to be applied to the target document. Each [Update Object](#updateObject) has a `target` property and a `value` property. The `target` property is a [JMESPath](http://jmespath.org/specification.html) query that identifies what part of the target document is to be updated and the `value` property contains an object with the properties to be overlaid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I love JMESPath!

Copy link

@mattbishop mattbishop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the JMESPath / update approach, but overlay ordering needs to be addressed.


## Backwards compatibility

Overlays will be described in a new specification that can be used alongside an OpenAPI Description, therefore there will be no compatibility issues for the initial release. Overlay documents can be used against OpenAPI v2 and v3 descriptions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad to see this, though it might be good to only support v3 to help give people another reason to move to v3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it. I just didn't like the idea of creating an arbitrary constraint that I couldn't really justify. I'm on the fence. I suspect if we tell people they can't people will do it anyway.


- Tooling will need a JMESPath implementation.
- Large overlays may be slow to process.
- Multiple complex pattern based overlays may cause overlapping updates causing confusing outcomes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overlay order needs to be unambiguous. Moreover, previous overlays in a chain must be immutable. A change to an earlier overlay will likely ruin subsequent overlays. Perhaps an overlay can declare a hash of the document it is updating? This would probably need to be coupled with some kind of ordering convention in the overlay files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That last comment of mine is poorly worded. An Overlay document has an array of update objects. The order of applying those updates is defined based on the order of the array. Therefore within an Overlay document the order is well defined. However, there still could be overlapping updates. That could make the Overlay document tricky to understand. Best practice should be to try and avoid the situation.

Having multiple Overlay documents apply to a single OpenAPI description is a different challenge. Currently this is up to tooling to manage the order of application. I don't like the idea of linking or ordering Overlay documents explicitly in the document, but I could imagine the use of priority value that provides an ordering hint to tooling.

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

Successfully merging this pull request may close these issues.

None yet

5 participants