Handle custom detector response and include in extra data #3411
Conversation
|
@kashifkhan0771 could you take a look at the test failure: |
|
| if err != nil { | ||
| return fmt.Errorf("failed to read response body: %v", err) | ||
| } |
There was a problem hiding this comment.
This looks like it's returning an error for the FromData call, is that right? I think that may not be what we want to do for two reasons:
- it will break the loop so other configs aren't tested
- it seems like it would be a non-determinant error (if those exist in custom detectors), and should maybe set the verification error
There was a problem hiding this comment.
There other parts that also return an error, this is just one example.
There was a problem hiding this comment.
I think I did that out of habit. I’ve fixed it now, and we’re continuing to the next step instead of returning an error.
| // helper function to handle JSON response | ||
| func handleJSONResponse(body []byte) (string, error) { | ||
| var respBody interface{} | ||
| err := json.Unmarshal(body, &respBody) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to unmarshal JSON: %v", err) | ||
| } | ||
|
|
||
| // convert JSON map to a formatted string | ||
| jsonString, err := json.MarshalIndent(respBody, "", " ") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to marshal JSON: %v", err) | ||
| } | ||
|
|
||
| return strings.TrimSpace(string(jsonString)), nil | ||
| } |
There was a problem hiding this comment.
Why does this function unmarshal then marshal?
There was a problem hiding this comment.
Ah, I think I might have over-engineered this, even though @ahrav warned me not to! 😅 I was trying to get the response into a nicely formatted JSON string, but I realize now that it's not really necessary. We can just stick to simple string conversion instead.
|
|
||
| // read the Content-Type header and response body | ||
| respContentType := resp.Header.Get("Content-Type") | ||
| body, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
@mcastorina I could not recall where we had a discussion about PII. I remember, we decided not to include PII in extraData. Will this needed to be care here as well ?
There was a problem hiding this comment.
That's a great point. Custom detector servers are user supplied (or at least user configured), so I don't think PII would be an issue here.. @zricethezav what are your thoughts?
Description:
This PR handle custom detector response and include it in the ExtraData
Checklist:
make test-community)?make lintthis requires golangci-lint)?