Skip to content

Add ability to dynamically add filter JSON fields (and test coverage)#185

Merged
EnriqueL8 merged 8 commits intomainfrom
filter-ext
Jun 30, 2025
Merged

Add ability to dynamically add filter JSON fields (and test coverage)#185
EnriqueL8 merged 8 commits intomainfrom
filter-ext

Conversation

@peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented May 28, 2025

When processing a JSONQuery filter it is helpful to be able to dynamically build up a list of fields, for cases where there might be join tables to something very dynamic like a set of tags/labels that require an individual join for each filterable tag.

  • Added FieldResolver plug extension following the existing plugin pattern for ValueResolver
  • Allowed a Clone() of a field set, to a new map that lets to add fields
    • As per the included test you need to update this dynamically so the Finalize() will work
  • endsWith had been omitted form JSON Query - so added that

Additional things found

  • We couldn't support array typed input on the JSONHandler due to a quirk where the type was ending up as []interface{} rather than the type returned from JSONInputValue. I've proposed a new JSONInputDecoder for that, which lets you work around this very simply:
    • JSONInputDecoder: func(req *http.Request, body io.Reader) (interface{}, error) {
      var arrayInput []*testInputStruct
      err := json.NewDecoder(body).Decode(&arrayInput)
      return arrayInput, err
      },
  • There was some invalid code in the filter logic around BigInt processing that I fixed writing missing tests - it was discarding numbers that are perfectly acceptable for big integer filters. Unlikely to have been used, but I think this was a copy/paste from the other code to make a compiler check on integer limits go away.
  • Found some untested code around versions and OpenAPI constructs and validated it executed as written
  • I was wondering about the 200 rather than 204 return code that was specified in the liveness check, which returns a Content-Type: application/json without any body. But I just commented that rather than changing it.
    • Not sure if @onelapahead has a thought there... I was worried about changing it (doesn't block merging)

…age)

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst requested a review from a team as a code owner May 28, 2025 22:13
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Copy link
Contributor

@EnriqueL8 EnriqueL8 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 to me - I don't see the commented code you mentioned on the 200 /204

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
status = res.StatusCode
}
l.Warnf("WS %s connect attempt %d failed [%d]: %s", w.url, attempt, status, string(b))
l.Warnf("WS %s connect attempt %d failed [%d]: %s", w.url, attempt, status, errMsg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the use of Custom Log this was failing to actually log the underlying error at all in the case the error wasn't data returned from the remote server (but something like a connection failure instead)

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
return nil
case <-ctx.Done():
return i18n.NewError(ctx, i18n.MsgWSSendTimedOut)
case <-w.sendDone:
Copy link
Contributor Author

@peterbroadhurst peterbroadhurst Jun 29, 2025

Choose a reason for hiding this comment

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

This was an edge case deadlock:

goroutine 12 [select]:
github.com/hyperledger/firefly-common/pkg/wsclient.(*wsClient).Send(0x40004d2100, {0xb86230, 0x40004be0a0}, {0x4000412770?, 0xffffffffffffffff?, 0x0?})
        /go/pkg/mod/github.com/hyperledger/firefly-common@v1.5.6-0.20250629160958-9871d4332701/pkg/wsclient/wsclient.go:305 +0xd4
----- CODE THAT DOES A SEND IN AN afterConnect() CALLBACK ----
github.com/hyperledger/firefly-common/pkg/wsclient.(*wsClient).receiveReconnectLoop(0x40004d2100)
        /go/pkg/mod/github.com/hyperledger/firefly-common@v1.5.6-0.20250629160958-9871d4332701/pkg/wsclient/wsclient.go:542 +0x194
created by github.com/hyperledger/firefly-common/pkg/wsclient.(*wsClient).initialConnect in goroutine 10
        /go/pkg/mod/github.com/hyperledger/firefly-common@v1.5.6-0.20250629160958-9871d4332701/pkg/wsclient/wsclient.go:254 +0x68
FAIL

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Copy link
Contributor

@EnriqueL8 EnriqueL8 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 - just added explanation of one piece as it wasn't obvious

Comment on lines +182 to +193
rv := buildResolveCtx(ctx, &jq.FilterJSON, options...)
for _, field := range jq.Sort {
field, isDesc := strings.CutPrefix(field, "-")
field, err := validateFilterField(ctx, fb, field, rv)
if err != nil {
return nil, err
}
if isDesc {
field = "-" + field
}
fb = fb.Sort(field)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clunky but I get why you have done this way, Sort is shared between this filter_json and filter and you need to trim prefix to remove the desc - to validate the filter which might resolve to another name and then add again. The fb.Sort can take in multiple values as well but it's fine

@EnriqueL8 EnriqueL8 merged commit fa03f8f into main Jun 30, 2025
5 checks passed
@EnriqueL8 EnriqueL8 deleted the filter-ext branch June 30, 2025 15:22
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.

3 participants