Skip to content
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

Clarify the update behaviour for arrays as well as objects #50

Merged
merged 8 commits into from
Sep 13, 2024

Conversation

lornajane
Copy link
Contributor

As discussed in a recent meeting see notes, clarify that for arrays, the update adds an entry but for objects the items are merged ( as is already described)

ralfhandl
ralfhandl previously approved these changes Sep 2, 2024
versions/1.0.0.md Outdated Show resolved Hide resolved
versions/1.0.0.md Outdated Show resolved Hide resolved
@lornajane lornajane requested a review from ralfhandl September 10, 2024 15:36
ralfhandl
ralfhandl previously approved these changes Sep 10, 2024
mikekistler
mikekistler previously approved these changes Sep 10, 2024
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@ralfhandl ralfhandl dismissed stale reviews from mikekistler and themself via cc25436 September 10, 2024 16:01
ralfhandl
ralfhandl previously approved these changes Sep 10, 2024
| <a name="action-remove"></a>remove | `boolean` | A boolean value that indicates that the target object is to be removed from the the map or array it is contained in. The default value is `false`. |

The result of the `target` JSONPath query expression must be zero or more objects or arrays (not primitive types or `null` values). If you wish to update a primitive value such as a string, the `target` expression should select the _containing_ object in the target document.
The result of the `target` JSONPath expression must be zero or more objects or arrays (not primitive types or `null` values). If you wish to update or remove a primitive value such as a string, the `target` expression should select the _containing_ object in the target document.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I see how a primitive value can be removed. To update a primitive value you would use update on the parent object with a value that contains the new value. But unless we adopt JSON Merge Patch semantics for "update", where "null" means remove, then I see no way to actually remove a primitive value with "update", and because we can't make it a target, we can't remove it with remove either. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct, I'd rather remove the restriction

The result of the target JSONPath expression must be zero or more objects or arrays (not primitive types or null values).

in v2.0. For v1.0 we are probably stuck with it.

How about

Suggested change
The result of the `target` JSONPath expression must be zero or more objects or arrays (not primitive types or `null` values). If you wish to update or remove a primitive value such as a string, the `target` expression should select the _containing_ object in the target document.
The result of the `target` JSONPath expression must be zero or more objects or arrays (not primitive types or `null` values). To update a primitive property value or remove a primitive array item such as a string, the `target` expression must select the _containing_ object in the target document.

Copy link
Contributor

Choose a reason for hiding this comment

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

How would I remove a primitive array item? I know I can remove an entire array node, or replace an entire array node with update that targets its parent object, but I don't see how to remove an individual item.

Copy link
Contributor

@ralfhandl ralfhandl Sep 11, 2024

Choose a reason for hiding this comment

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

You replace the array with a new one that does not contain the item to remove. Which means you have to know the full array content.

I definitely want to drop this whole paragraph and allow target expressions to select any kind of node, including primitive values, but that would be a significant change since the restriction to objects and arrays has been in since the spec document was created more than three years ago.

Opened

to track and discuss this for v2.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then can we just go back to the original wording of that last sentence:

If you wish to update a primitive value such as a string, the target expression should select the containing object in the target document.

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost, plus the explicitly mentioned removal of a primitive array item in the suggested change.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think we've established that you can't in general remove an item from an array, primitive or not. This is only possible by replacing the entire array. To make that a "remove" operation, the overlay would need to know the original contents of the array, which typically would not be the case. And if it were the case then the action is really replacing the array -- just in a way that one item is removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The second example in Array Modification Examples shows how to remove an object from an array:

overlay: 1.0.0
info:
  title: Remove a array element
  version: 1.0.0
actions:
  - target: $.paths.*.get.parameters[[email protected] == 'dummy']
    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.

Unfortunately we restrict target to only select objects and arrays, so I am not allowed to

overlay: 1.0.0
info:
  title: Remove a string from the tags array
  version: 1.0.0
actions:
  - target: $.paths.*.get.tags[?@ == 'dummy']
    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.

Got it. You can remove an array element when it is an object using JSON Path filtering.

So do we go back to the original wording for that last sentence? By original wording I mean:

If you wish to update a primitive value such as a string, the target expression should select the containing object in the target document.

We could update this to be even more explicit:

If you wish to update a primitive value such as a string, the target expression should select the containing object in the target document and the action should contain the property with the value to be updated.

@ralfhandl ralfhandl dismissed their stale review September 11, 2024 11:27

Text needs to be changed

versions/1.0.0.md Outdated Show resolved Hide resolved
| <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 | 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`. |
| <a name="action-update"></a>update | Any | If the `target` selects an object node, the value of this field should be an object with the properties and values to merge with the node. If the `target` selects an array, the value of this field should 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.

Is the "merge" shallow or deep? That is, if target selects an object and update is an object with an object-valued property, is that property's object value replaced, or is it also (recursively) merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

On line 106 below it says:

When the Overlay document is applied, the properties in the merge object replace properties in the target object with the same name and new properties are appended to the target object.

Based on that wording, I think the "merge" is actually a shallow merge.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@ralfhandl ralfhandl merged commit 868f6eb into OAI:main Sep 13, 2024
1 check passed
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.

4 participants