Bump go-sqlite3 library to SQLite3MultipleCiphers 1.8.2 / SQLite3 3.45.0 #821
Conversation
| @@ -59,7 +59,7 @@ func BuildJsonCondition( | |||
| conditionTemplate = "%s->>? = ?" | |||
| idx := 0 | |||
| for k, v := range jsonPathValueMap { | |||
| args[idx] = k | |||
| args[idx] = fmt.Sprintf("$.%s", k) | |||
There was a problem hiding this comment.
It seems like this would require some other changes in the query module -- particularly in query.go, clause.go, query_test.go, etc
There was a problem hiding this comment.
thanks for letting me know! could you please elaborate on the required changes?
There was a problem hiding this comment.
for instance, the query parser is not supplying the '$.' prefix for keys, https://github.com/G-Research/fasttrackml/blob/main/pkg/api/aim/query/query_test.go#L283-L298
But the integration tests which depend on it are still passing. So I think it must be optional?
There was a problem hiding this comment.
Maybe we should just make this change (and adjust query_test.go accordingly)?
There was a problem hiding this comment.
perhaps add the '$.' unless already present (assuming users could provide well-formed json path inputs)?
| @@ -84,7 +84,7 @@ func TestBuildJsonCondition(t *testing.T) { | |||
| "key2.nested": "value2", | |||
| }, | |||
| expectedSQL: "contexts.json->>? = ? AND contexts.json->>? = ?", | |||
| expectedArgs: []interface{}{"key1", "value1", "key2.nested", "value2"}, | |||
| expectedArgs: []interface{}{"$.key1", "value1", "$.key2.nested", "value2"}, | |||
There was a problem hiding this comment.
For instance, this isn't the only test where we are looking at the args created by query.go -- there are a few cases in query_test.go that test the json key argument. I'm a little surprised this doesn't break more tests ... I can try out your branch, maybe the $. prefix is optional
This PR bumps up the SQLite3 library to latest version which introduces support for JSONB. The way to use JSON paths has also changed (it looks like the way we used them before was not supposed to work in the first place) so this required changing a couple of things in the code.
Once this PR is merged, we should consider using JSONB for contexts instead of the current JSON implementation.