ohler55 / ojg

Optimized JSON for Go
MIT License
839 stars 50 forks source link

jp feature request: Set() that only replaces existing values #99

Closed juliannguyen4 closed 1 year ago

juliannguyen4 commented 1 year ago

Currently Set() will insert any new values in lists and maps if they don't exist. Is there a way to call Set() so it only replaces existing matching items?

For example, this code:

expr, err := jp.ParseString("$..int")
expr.Set(document, 10)

with this document:

{
    "map": {
        "int": 4
    }
}

should only replace the value of "int" in the nested map:

{
    "map": {
        "int": 10
    }
}

Right now, Set() would insert a key-value pair of "int" and 10 in every matching location:

{
    "map": {
        "int": 10
    }
    "int": 10
}
ohler55 commented 1 year ago

The first document is not a valid JSON document. There is not function to check for existence and then set but you can combine a get with a set. Use the filter for exists and then set.

ohler55 commented 1 year ago

Does that answer your question? If so can this issue be closed?

juliannguyen4 commented 1 year ago

Hi @ohler55, I tried the solution you proposed, but I had trouble implementing it because there's a lot of overhead to create a filter expression for each existing path in the JSON document. (e.g you have to check if the operation after a recursive operator is a map or list access, and then having to construct a new JSON path to get the item) But I think it would benefit a lot of people to have a Set() for only existing values, since it is a common operation to update existing data fields in a document.

ohler55 commented 1 year ago

I'm not sure I follow. Can you give an example?

ohler55 commented 1 year ago

I see the example above was updated but without the path used it's hard to tell why you would get the result you did. Was it $..int?

ohler55 commented 1 year ago

I'm still not sure what you were looking for but if the intent was to replace any occurrence of "int": 4 anywhere in the json then $..[?(@.int == 4)].int would be the JSONPath to use. If that wasn't the intent then please provide the path you were trying to use and I'll reopen the issue.

Closing due to no response.

juliannguyen4 commented 1 year ago

Hi @ohler55, sorry I was on vacation for the holidays and I wasn't able to get back to you until now.

Yes, the jsonpath was $..int, and the intent was to replace every existing occurrence of int in the document. I should've made that more clear.

ohler55 commented 1 year ago

Try $..[?(@.int != null)].int and see if that does what you want.

juliannguyen4 commented 1 year ago

That seems to work. Thanks for making that suggestion!

juliannguyen4 commented 1 year ago

Try $..[?(@.int != null)].int and see if that does what you want.

This method works for replacing every existing item with the same value, but it doesn't work for more advanced operations.

Right now, I'm writing a method to append to every list that matches a JSON path. For example, with JSON path$..list, I want to append the number 4 to every matching list:

{
    "map1": {
        "list": [1, 2, 3]
    },
    "map2": {
        "list": [0, 1, 2]
    }
}

The expected results should be:

{
    "map1": {
        "list": [1, 2, 3, 4] # 4 appended to this list
    },
    "map2": {
        "list": [0, 1, 2, 4] # 4 appended to this list
    }
}

Do you have a way to do this?

ohler55 commented 1 year ago

There is not a way to do that in the current release. It would require a change in the API with a new function since the modified slice would change and that would have to be passed back to the caller. That is similar to the recent addition of the Remove() functions for the jp package. It would also require a new variation to JSONPath to differentiate between a set and an append.

thadguidry commented 1 year ago

It seems as if the Union operator [,] could help somehow?

[,] Union operator in XPath results in a combination of node sets. JSONPath allows alternate names or array indices as a set.

list: [0, 1, 2, 3][*,"4"] (but that's a String type and not Int type).

I personally love the << leftShift operator (push method) in other languages like Java, Ruby, etc. to append to a list.

foo = Array(foo) << :element

But the JsonPath function is really append(X) or alternatively if you need stricter typing extending the list you could use concat(X).

In Python, there are 2 concepts, appending with a single value using append() and extending with += operator theirlist += mylist[:3]

@ohler55 have you given broader thought that OJG eventually expands more with additional operators like they did in https://www.npmjs.com/package/jsonpath-plus. I ask because of your last sentence that seems as if you are OK with deviating from JSONPath (Stefann Goessner implementation)

ohler55 commented 1 year ago

The Goessner description of JSONPath is a bit loose so adding new capabilities doesn't really deviate from the spec so much as expand on it.

I've been giving some thought to being able to append. Adding an Append() function might be the most straight forward but what about inserts or other list modifications. I'm leaning toward a more flexible function. Something where a function could be applied to what ever the JSONPath identifies. Not sure about the API for it yet though.

Anyway, JSONPath locates a JSON element. I'm hesitant to expand its purpose into a processing language. I think keeping the processing features outside the path is probably best and less confusing.

thadguidry commented 1 year ago

Totally agree. Finding elements is different from changing elements.

ohler55 commented 1 year ago

I was thinking of this:

func (x Expr) Modify(data any, modifier func(element any) (newElement any, changed bool)) any {
        // the code
        return data
}

The data elements are found and passed to the modifier and the result then set in the data. Can be used for appends, inserts, sorting, or what ever is desired.

juliannguyen4 commented 1 year ago

Hi @ohler55, that would be great but why does the modifier function need to return changed? Wouldn't this function apply to all the matches in the document?

ohler55 commented 1 year ago

Yes to both questions. It would return the changed data and it would apply to all the matches. It's up to the provided function to decide what to do.

juliannguyen4 commented 1 year ago

In the modifier function, how would it access data from Modify()? For example, if I wanted to append data to element and return the result as newElement. Never mind I realized data is the JSON document being modified.

I think the proposal looks good and I'm looking forward to seeing it implemented.

ohler55 commented 1 year ago

It might take a few weeks depending on my work schedule.

ohler55 commented 1 year ago

Or it could take a day. Expect a branch (jp-modify) for review in a couple of hours. Expr.Remove() has been reimplemented to use the Expo.Modify() function. Just need more tests and it will be ready.

ohler55 commented 1 year ago

Released v1.17.0 with Expr.Modify()

juliannguyen4 commented 1 year ago

Hi @ohler55, thank you so much for releasing this!

Right now, I think I have found an issue in Modify(). A JSON path containing recursive descent doesn't seem to work:

package main
import (
    "fmt"
    "github.com/ohler55/ojg/jp"
)

func main() {
    expr := jp.MustParseString("$..int")
    var data any
    data = map[string]any{
        "a": []any{
            map[string]any{
                "int": 1,
            },
        },
    }

    modifier := func(_ any) (any, bool) {
        return 4, true
    }
    data, err := expr.Modify(data, modifier)
    if err != nil {
        panic("error")
    }

    fmt.Println(data)
}

Output:

map[a:[map[int:1]]]

Expected output:

map[a:[map[int:4]]]
ohler55 commented 1 year ago

Descent are a difficult one. An error is returned if the last fragment is a descent. Being the second to last should be okay though. Let me look at this tonight.

ohler55 commented 1 year ago

Fixed in the "fix-modify-descent" branch. If you have the time give it a try.

thadguidry commented 1 year ago

Fixed in PR #105

juliannguyen4 commented 1 year ago

Hi @ohler55, that branch fixes the problem so I really appreciate that. Could you create a release for it please?

ohler55 commented 1 year ago

done