fix: payload serialization in sensor. Fixes #2272 #2273
Conversation
GoshaDo
force-pushed
the
fix-sensor-parameter-serlization-block
branch
from
12e051f
to
ae83c04
Compare
sensors/triggers/params.go
Outdated
| resultValue, err = getValueByKey(eventPayload, key) | ||
| tmp, typ, err := getValueByKey(eventPayload, key) | ||
| // For block injection support | ||
| fmt.Printf("failed to get value by key: %+v\n", err) |
There was a problem hiding this comment.
What is this?
How come we have so many fmt.Printf in the code.....
There was a problem hiding this comment.
already been there.
https://github.com/argoproj/argo-events/blob/25f5a50df5963b0b5c6c7e0508474ade24b715ba/sensors/triggers/params.go#L240
changing it's place to after the error check
GoshaDo
force-pushed
the
fix-sensor-parameter-serlization-block
branch
from
4e144c5
to
48a81a3
Compare
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
…goproj#2286) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
…rgoproj#2285) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
…rgoproj#2288) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
…argoproj#2289) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
…j#2283) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
…rgoproj#2284) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: Joep Keijsers <joep.keijsers@geodan.nl> Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: Eddie Knight <iv.eddieknight@gmail.com> Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: Elifarley <elifarley@gmail.com> Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: Eddie Knight <knight@linux.com> Signed-off-by: goshado <goshatoo@gmail.com>
Signed-off-by: goshado <goshatoo@gmail.com>
GoshaDo
force-pushed
the
fix-sensor-parameter-serlization-block
branch
from
48a81a3
to
3a8c609
Compare
sensors/triggers/params.go
Outdated
| @@ -183,7 +205,7 @@ func ResolveParamValue(src *v1alpha1.TriggerParameterSource, events map[string]* | |||
| } | |||
|
|
|||
| if err == nil { | |||
| return &resultValue, nil | |||
| return &resultValue, errorType, nil | |||
There was a problem hiding this comment.
Is this right? If there's no error, return errorType?
The original logic looks very tricky. Can we make it simple like this:
if src.Value != nil {
return *src.Value
} else {
eventPayload, err = json.Marshal.....
if err != nil {
return err
}
return string(eventPayload)
}
There was a problem hiding this comment.
have changed the error check just after json.Marshal
about the return, we should return a pointer to the casted eventPayload variable. I think best to write it in memory.
There was a problem hiding this comment.
and the function returns (*string, string, error)
we can't return error only...
There was a problem hiding this comment.
It's crashing the unit-test... I have revert the changes
sensors/triggers/params.go
Outdated
| } | ||
| fmt.Printf("failed to get value by key: %+v\n", err) | ||
| } | ||
| // In case neither key nor template resolving was successful, fall back to the default value if exists | ||
| if src.Value != nil { | ||
| resultValue = *src.Value | ||
| return &resultValue, nil | ||
| return &resultValue, typ, nil |
There was a problem hiding this comment.
Does the typ returned here have a value?
There was a problem hiding this comment.
Right... I will return errorType, because we fail the function
There was a problem hiding this comment.
It falls back to the default value, does not fail the function.
There was a problem hiding this comment.
What do you suggest to return here?
sensors/triggers/params.go
Outdated
| default: | ||
| // The parameter doesn't have a default value and is referencing a dependency that is | ||
| // missing in the received events. This is not an error and may happen with || conditions. | ||
| return nil, nil | ||
| return nil, errorType, nil |
There was a problem hiding this comment.
The original return seems to be incorrect, it should return an error.
There was a problem hiding this comment.
checkout the comment in lines 230-231
There was a problem hiding this comment.
If that's the case, can you return stringType as that's the original expectation?
sensors/triggers/params.go
Outdated
| default: | ||
| // The parameter doesn't have a default value and is referencing a dependency that is | ||
| // missing in the received events. This is not an error and may happen with || conditions. | ||
| return nil, nil | ||
| return nil, errorType, nil |
There was a problem hiding this comment.
If that's the case, can you return stringType as that's the original expectation?
sensors/triggers/params.go
Outdated
| // Get the value corresponding to specified key or template within event payload | ||
| if eventPayload != nil { | ||
| if tmplt != "" { | ||
| resultValue, err = getValueWithTemplate(eventPayload, tmplt) | ||
| if err == nil { | ||
| return &resultValue, nil | ||
| return &resultValue, errorType, nil |
There was a problem hiding this comment.
Why return errorType when err == nil?
There was a problem hiding this comment.
because the we didn't resolver the param. what are you suggestions ?
sensors/triggers/params.go
Outdated
| } | ||
| fmt.Printf("failed to get value by key: %+v\n", err) | ||
| } | ||
| // In case neither key nor template resolving was successful, fall back to the default value if exists | ||
| if src.Value != nil { | ||
| resultValue = *src.Value | ||
| return &resultValue, nil | ||
| return &resultValue, errorType, nil |
sensors/triggers/params.go
Outdated
| @@ -179,12 +200,11 @@ func ResolveParamValue(src *v1alpha1.TriggerParameterSource, events map[string]* | |||
| } else { | |||
| // Default value doesn't exist so return the whole event payload | |||
| eventPayload, err = json.Marshal(&event) | |||
| if err == nil { | |||
| return &resultValue, errorType, nil | |||
| // returns an error if the Path is invalid/not found and the default value is nil OR if the eventDependency event doesn't exist and default value is nil | ||
| func ResolveParamValue(src *v1alpha1.TriggerParameterSource, events map[string]*v1alpha1.Event) (*string, error) { | ||
| func ResolveParamValue(src *v1alpha1.TriggerParameterSource, events map[string]*v1alpha1.Event) (*string, string, error) { |
There was a problem hiding this comment.
Even you added an errorType in the return, but you never checked it. To make it simple, the guideline should be:
- If there's no error, return
(xxx, xx, nil) - Or return
(nil, "", err)
There was a problem hiding this comment.
that what originally been there... reverting those changes..
sensors/triggers/params.go
Outdated
| // Otherwise, return the error | ||
| return nil, err | ||
| return nil, errorType, err |
There was a problem hiding this comment.
There's no need to have an errorType, since you never checked it.
Simply return nil, "", err,
sensors/triggers/params.go
Outdated
| // Get the value corresponding to specified key or template within event payload | ||
| if eventPayload != nil { | ||
| if tmplt != "" { | ||
| resultValue, err = getValueWithTemplate(eventPayload, tmplt) | ||
| if err == nil { | ||
| return &resultValue, nil | ||
| return &resultValue, "", nil |
There was a problem hiding this comment.
What is the reason it returns "" as the type instead of stringType when err == nil?
sensors/triggers/params.go
Outdated
| } | ||
| fmt.Printf("failed to get value by key: %+v\n", err) | ||
| } | ||
| // In case neither key nor template resolving was successful, fall back to the default value if exists | ||
| if src.Value != nil { | ||
| resultValue = *src.Value | ||
| return &resultValue, nil | ||
| return &resultValue, "", nil |
There was a problem hiding this comment.
Why it returns "" as the type instead of stringType?
sensors/triggers/params.go
Outdated
| } | ||
| return "", fmt.Errorf("key %s does not exist to in the event payload", key) | ||
| return "", errorType, fmt.Errorf("key %s does not exist to in the event payload", key) |
There was a problem hiding this comment.
no need to have an errorType, same reason as above.
sensors/triggers/params.go
Outdated
| } else if res.Type.String() == jsonType { | ||
| return res.Raw, res.Type.String(), nil | ||
| } | ||
| return res.Raw, res.Type.String(), nil |
There was a problem hiding this comment.
This is the default return, make it consistent as before:
return res.String(), stringType, nil
sensors/triggers/params.go
Outdated
| @@ -183,7 +203,7 @@ func ResolveParamValue(src *v1alpha1.TriggerParameterSource, events map[string]* | |||
| } | |||
|
|
|||
| if err == nil { | |||
| return &resultValue, nil | |||
| return &resultValue, "", nil | |||
There was a problem hiding this comment.
return &resultValue, stringType, nil
whynowy
changed the title
Fixes #2272
Signed-off-by: GoshaDo goshatoo@gmail.com