Skip to content

Conversation

ThomasRooney
Copy link

Closes #121

@lornajane
Copy link
Contributor

After talking about with in the meeting this week, we discussed that instead of doing new destinations, we would aim to handle the rename case by adding support for new source information - a use case we already discussed and that has applications beyond the rename.

The idea is that instead of only doing:

actions:
  - target: $.paths['/foo'].get
    update:
      description: This is the new description

We would also support using something like the existing value of the target, so you could do:

actions:
  - target: $.paths['/foo'].get
    update:
      description: $target.summary

To re-use the content of another field in this one. Also $env and $doc (meaning the API description the overlay is being applied to. For renames, you would add a followup remove action.

Consideration will be needed for values where the value itself might reasonably contain a $, such as where the value is $ref.

@mikeschinkel
Copy link

Today @lornajane and I discussed this on call with both of us finding the exact syntax proposed not idea.

@lornajane took the position that update should be enhanced to support this used case, but that would necessitate a 2.0.0 release. She wanted to enhance update to make it more powerful so that it could address this use-case as well as others (@lornajane please correct my characterization if I got it wrong.)

My perspective is that we could instead offer a rename property in addition to update that could allow for keeping same major version — i.e. no backward compatible breakage — and having a rename would make it more obvious to those looking for such a feature. It would also decouple the use-cases so that update would not have to be compatible with a rename use-case, existing uses-cases, and future use-cases; rename could instead be bespoke for its needs and only its needs.

A quick strawman example might look like this (and since I am doing this in a rush before the next meeting I will most likely not get this right, but hopefully I will.):

overlay: 1.0.0
info:
  title: Rename path items
  version: 1.0.0
actions:
  - target: $.paths
    update: 
      "/newitem": {}
  - target: $.paths
    rename:
      existing: /items
      new: /newitems
    remove: true

Please comment and bikeshed away.

@lornajane
Copy link
Contributor

Perhaps I did not explain myself well although I did try in the discussion. I suggested that we could support the use case that this pull request addresses by adding support for input sources. This does not require a major version bump, unless we need to add extra options for handling content such as $ signs within OpenAPI project (since the proposed new syntax would support $target and $doc) - the additional options probably want to go inside update and that change would cause a major version bump. Mike suggested adding additional operations with their own data types for each supported use case, so I did not proceed with the work to propose the additional input sources.

However we have both thoroughly invaded @ThomasRooney 's original pull request - I'll add some comments in line with the changes.

Copy link
Contributor

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

I'm very keen to find a way to enable this use case, but I'm not in favour of the proposed implementation.

@@ -115,8 +115,9 @@ This object represents one or more changes to be applied to the target document
| ---- | :----: | ---- |
| <a name="action-target"></a>target | `string` | **REQUIRED** A JSONPath expression selecting nodes in the target document. |
| <a name="action-description"></a>description | `string` | A description of the action. [[CommonMark]] syntax MAY be used for rich text representation. |
| <a name="action-update"></a>update | Any | If the `target` selects an object node, the value of this field MUST be an object with the properties and values to merge with the selected node. If the `target` selects an array, the value of this field MUST be an entry to append to the array. This field has no impact if the `remove` field of this action object is `true`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this behaviour? Remove will still cause the other fields to have no impact?

| <a name="action-remove"></a>remove | `boolean` | A boolean value that indicates that the target object or array MUST be removed from the the map or array it is contained in. The default value is `false`. |
| <a name="action-destination"></a>destination | `string` | A JSONPath expression selecting destination nodes in the target document. If set, this indicates that each node in the target expression (after applying the update) should be appended as a child to the destination node. If not set, the target is mutated in-place. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I support solving the use case, but I am not in favour of this syntax. In 1.0, the changes are applied to the nodes that match the target expression. In this instance, the target isn't the target, but is more like the source which has potential to be confusing.

Is there another way we could indicate which field to update, and where to get the data from to put there?

"/newitem": {}
- target: $.paths["/item"]
destination: $.paths["/newitem"]
remove: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to see copy and remove as two different steps - it's likely that we'll do action as update or remove (or whatever else) in a future version and I think it's clearer.

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.

Use existing content of OpenAPI as input to allow renaming/editing
3 participants