Use a generic version of JQ accessor, called JQGetTypedFromAccessor to parse GitHub payload #801
Conversation
I was going back and forth on this as I was creating the patch. If func (jqe *Evaluator) Eval(ctx context.Context, pol map[string]any, obj any) error {
for idx := range jqe.assertions {
a := jqe.assertions[idx]
policyVal, err := util.JQGetValuesFromAccessor(ctx, a.Policy.Def, pol)
if err != nil {
return fmt.Errorf("cannot get values from policy accessor: %w", err)
}
dataVal, err := util.JQGetValuesFromAccessor(ctx, a.Ingested.Def, obj)
if err != nil {
return fmt.Errorf("cannot get values from data accessor: %w", err)
}
// Deep compare
if !reflect.DeepEqual(policyVal, dataVal) {
msg := fmt.Sprintf("data does not match policy: for assertion %d, got %v, want %v",
idx, dataVal, policyVal)
marshalledAssertion, err := json.MarshalIndent(a, "", " ")
if err == nil {
msg = fmt.Sprintf("%s\nassertion: %s", msg, string(marshalledAssertion))
}
return evalerrors.NewErrEvaluationFailed(msg)
}
}
return nil
}On the other hand, when parsing the JSON payload from GitHub, we typically care that the property is there and want to handle the absence as an error which is what the new generic function does: func JQGetTypedFromAccessor[T any](ctx context.Context, path string, obj any) (T, error) {
var out T
var ok bool
outAny, err := JQGetValuesFromAccessor(ctx, path, obj)
if err != nil {
return out, err
}
if outAny == nil {
return out, NewErrNoValueFound("no value found for path %s", path)
}so I thought the two have different use-cases. |
I'd be okay with having to read |
To make sure I understand you, are you saying that you'd like both functions to use generics, but let one return the default value for the type (so, nil for the case of |
No, I'm suggesting just one function that returns either a found value and no error, or the default value and an error if the value isn't found or is of the wrong type. |
That's what I started with and found it awkward (or less compact at least) in the |
Turns out that relying on the default value makes the code quite readable. WDYT? |
…o parse GitHub payload Addresses a comment brought up during an earlier review that the way we parse the github payload is unsafe - in case the JQ accessor wouldn't return anything, the value returned would have been `nil` (the default value of `any`) which would panic on type assertion. Similarly, if the assertion failed due to github changing its API. Because these attributes are all required by the layer calling `JQGetTypedFromAccessor`, I opted to return an error in case the accessor doesn't find a value which would be propagated to the caller and handled appropriately. Other parts of the mediator where `nil` can be handled more gracefully (e.g. because it is just passed to `reflect.DeepEqual`) still use the old function `JQGetTypedFromAccessor` which returns `any` Fixes: #785
|
The latest push just fixes a lint issue (one testcase didn't call |
Addresses a comment brought up during an earlier review that the way we
parse the github payload is unsafe - in case the JQ accessor wouldn't
return anything, the value returned would have been
nil(the defaultvalue of
any) which would panic on type assertion. Similarly, if theassertion failed due to github changing its API.
Because these attributes are all required by the layer calling
JQGetTypedFromAccessor, I opted to return an error in case theaccessor doesn't find a value which would be propagated to the caller
and handled appropriately.
Other parts of the mediator where
nilcan be handled more gracefully(e.g. because it is just passed to
reflect.DeepEqual) still use theold function
JQGetTypedFromAccessorwhich returnsanyFixes: #785