JSONPath Support for TriggerBindings #241

dibyom · 2019-11-27T19:31:52Z

Changes

There are two commits in this PR. The first commit adds functions for JSONPath and the second one uses those functions to extract values for EventBinding.

Commits:
This commit adds functions to support JSONPath
in TriggerBindings using the k8s.io/client-go/util/jsonpath
library.

There are a few deviations from the default behavior of the library:

The JSONPath expressions have to be wrapped in the Tekton variable
interpolation syntax i.e $() : $(body).
We support the RelaxedJSONPathExpresion syntax used in
kubectl get -o custom-columns. This means that the curly braces {}
and the leading dot . can be omitted i.e we support both $(body.key)
as well as `$({.body.key})
We return valid JSON values when the value is a JSON array or
map. By default, the library returns a string containing an internal
go representation. So, if the JSONPath expression selects {"a": "b"},
the library will return map[a: b] while we return the valid JSON
string i.e {"a":"b"}

This commit switches TriggerBinding params to use
JSONPath instead of GJSON.

Part of #178

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

TriggerBindings now support JSONPath expressions instead of GJSON. Check docs/triggerbindings.md for the syntax and examples

tekton-robot · 2019-11-27T19:34:59Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	91.3%	-7.3
pkg/template/jsonpath.go	Do not exist	77.6%

tekton-robot · 2019-12-02T20:02:50Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	91.3%	-7.3
pkg/template/jsonpath.go	Do not exist	86.5%

tekton-robot · 2019-12-03T00:16:57Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	89.3%	-9.3
pkg/template/jsonpath.go	Do not exist	87.3%

tekton-robot · 2019-12-03T00:18:25Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	89.3%	-9.3
pkg/template/jsonpath.go	Do not exist	87.3%

tekton-robot · 2019-12-03T00:24:51Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	89.3%	-9.3
pkg/template/jsonpath.go	Do not exist	87.3%

pkg/template/event.go

pkg/template/jsonpath.go

wlynch · 2019-12-03T01:39:23Z

pkg/template/jsonpath_test.go

+	tests := []struct {
+		name  string
+		expr string
+		in     string


Instead of encoding the object body to a string then unmarshalling it out, would it be easier to make this interface{} and set the fields as maps?

yeah, don't have a strong opinion on this -> might be a bit more verbose for the array and objects that contain other embedded objects?

wlynch · 2019-12-03T01:44:43Z

pkg/template/jsonpath_test.go

+		name: "null values",
+		in: objectBody,
+		expr: "$(body.null)",
+		want: "null",


Is this WAI? Even though the payload says null we should probably make a distinction between null and "null". I say either default to "" or have ParseJSONPath return *string so we can represent this nil value (my guess is "" is probably sufficient)

Yeah, so I went back and forth on this a few times before settling on "null". I agree that if we were talking about return go values, the correct way should be *string. However, for our use case here (i.e. concatenating the json values in string in the trigger templates), that would result in the string <nil>. If we use "" to represent the printable value of null, we have no way to differentiate between null and the empty string "".

The "null" and null collision is unfortunate though I think this is the least bad way at the moment. Also, this issue is only valid if the JSONPath selects a field whose value is null. If the selection is an object or an array which contains both the string null as well as a null value, we get the following: {"stringNull":"null","nullValue":null}

tekton-robot · 2019-12-03T03:39:09Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	89.3%	-9.3
pkg/template/jsonpath.go	Do not exist	87.3%

tekton-robot · 2019-12-04T00:17:08Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	89.3%	-9.3
pkg/template/jsonpath.go	Do not exist	88.5%

tekton-robot · 2019-12-04T00:22:40Z

The following is the coverage report on pkg/.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/template/event.go	98.6%	89.3%	-9.3
pkg/template/jsonpath.go	Do not exist	88.5%

bobcatfish

My biggest overall piece of feedback is that there are some new functions added which could benefit from unit tests. I'm sure we have some coverage via Test_NewResources, and i can see that you want to break that up later on, but in the meantime i think it's still worth adding unit tests for the new functions right away (and its easier to tell what the expected behaviour is in each function, as well as what's covered and what isnt) <-- i think mostly you've done a great job, e.g. unit tests for ParseJSONPath but there are couple of functions which i think could also use tests which you probably skipped b/c they are unexported, and i think many of the functions (e.g. relaxedJSONPathExpression) are significant enough to actually be exported and have their own tests

Other than that I think I've just got nitpicks! Overall looks great :D
/approve

And definitely we want to make sure we squash some of these commits before we merge so:
/hold

bobcatfish · 2019-12-09T13:50:54Z

docs/triggerbindings.md

-`\$\(header(\.[[:alnum:]_\-]+)?\)`
+TriggerBindings can access values from the HTTP JSON body and the headers using
+JSONPath expressions. The expressions have to be wrapped in `$()` but can omit
+the curly braces `{}` and the leading `.`. 


taking a quick look online, it seems to me like most examples for jsonpath don't include {} anyway? e.g. https://support.smartbear.com/alertsite/docs/monitors/api/endpoint/jsonpath.html and this json path expression tester: https://jsonpath.curiousconcept.com/

so i'm wondering if a) we need to say this at all or if b) this b/c of the specific version of jsonpath we're using, in which case maybe this sentence could explain a bit more with a link, e.g. something like "We are using jsonpath version blah blah blah, and it expects expressions to be wrapped in {}, however we do not require {} in addition to $() though it can be used.."

https://kubernetes.io/docs/reference/kubectl/jsonpath/ mentions the enclosing {} though it also says its a JSONPath "template"....maybe we keep things simple and never mention the {}?

Removed the curly braces from the examples!

pkg/template/event.go

pkg/template/event_test.go

bobcatfish · 2019-12-09T13:56:02Z

pkg/template/event.go

 	}
 	return resources
 }
+
+// event represents a HTTP event that Triggers processes
+type event struct {


maybe "request" would be a more specific name than "event"?

might be confusing with http.Request. Also, I like event since we can later add the one-offs such as the $(uid) to this struct to unify how we do the "templating"

might be confusing with http.Request

it basically is an http.Request tho since it has a header + a body 🤔

I see what you mean about the uid tho! you're probably right about event being better in the long run, sgtm :D

bobcatfish · 2019-12-09T13:57:44Z

pkg/template/event.go

+
+	return &event{
+		Header: joinedHeaders,
+		Body:   data,


if len(body) == 0, what does data end up being?

(maybe we handle this totally fine - but to be sure it might be good to have some unit tests directly for newEvent?)

Yeah I added that check after verifying the empty body case: https://github.com/tektoncd/triggers/pull/241/files/4fe79f8e2d17b38e31d2d871c983a545cb06db01#diff-8d7c043b29f6a04a92258e1aac64fc2bR44

i cant see that link anymore - good ol rebasing 😅

https://github.com/tektoncd/triggers/pull/241/files?file-filters%5B%5D=.go#diff-8d7c043b29f6a04a92258e1aac64fc2bR42

bobcatfish · 2019-12-09T14:07:37Z

pkg/template/jsonpath.go

@@ -0,0 +1,158 @@
+package template


hm do you think there's another name we could use besides "template"? in pipelines we are trying to avoid using the term "templating" for the variable substitution we are doing b/c "template" implies additional functionality we want to avoid adding

i just realized the type is literally called "trigger template" and this package is probably named after that so just ignore me 🤦‍♀️

Given the discussion around JSONPath support in pipeline params, it might be worth having a common jsonpath pkg that both pipeline and triggers could use 🤔

Though that's a yak that we should shave another day 😄

pkg/template/jsonpath_test.go

pkg/template/resource.go

pkg/template/jsonpath.go

tekton-robot · 2019-12-09T14:14:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bobcatfish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dibyom · 2019-12-09T18:54:20Z

i think mostly you've done a great job, e.g. unit tests for ParseJSONPath but there are couple of functions which i think could also use tests which you probably skipped b/c they are unexported, and i think many of the functions (e.g. relaxedJSONPathExpression) are significant enough to actually be exported and have their own tests

The current TektonJSONPathExpression tests does test all the behaviors of RelaxedJSONPath expression. I do see the point about the behavior being easier to grok if some of the unexported functions have their own tests though I'm not sure if we should be exporting functions solely because they are complex or for testing.

Edit: Looking through the coverage report, there are a few error cases that we should add tests for:

      if i != len(results)-1 {
              text = append(text, ' ')
      }


        if !ok {
                return nil, fmt.Errorf("can't print type %s", v.Type())
        }

        if len(submatches) != 3 {
                return "", fmt.Errorf("unexpected submatch list: %v", submatches)
        }

dibyom · 2019-12-10T16:44:27Z

/test pull-tekton-triggers-build-tests

In tektoncd#178 we decided to support JSONPath syntax for expressions in TriggerBindings. This commit adds functions to support JSONPath using the `k8s.io/client-go/util/jsonpath` library. There are a few deviations from the default behavior of the library: 1. The JSONPath expressions have to be wrapped in the Tekton variable interpolation syntax i.e `$()` : `$(body)`. 2. We support the `RelaxedJSONPathExpresion` syntax used in `kubectl get -o custom-columns`. This means that the curly braces `{}` and the leading dot `.` can be omitted i.e we support both `$(body.key)` as well as `$({.body.key}) 3. We return valid JSON values when the value is a JSON array or map. By default, the library returns a string containing an internal go representation. So, if the JSONPath expression selects `{"a": "b"}`, the library will return `map[a: b]` while we return the valid JSON string i.e `{"a":"b"}` In order to support 3, we have to copy a couple of unexported functions from the library (printResults, and evalToText). Signed-off-by: Dibyo Mukherjee <[email protected]>

dibyom · 2019-12-10T19:25:14Z

Ok, I think I addressed all the comments!

Also, added a few more tests (header values, more relaxed jsonpath expressions etc.)

There area a couple of if branches that are hard to test e.g. we only reach there if the unmarshalled value is a Function or a Channel --I don't think any HTTP JSON payload can be unmarshalled to that!

Quite a few followups from this:

Escaping strings (Values injected into the template should be escaped #258, Escaping Quotes in TriggerBinding values #257)
Values for headers are sometimes inconsistent (Inconsistent types for Header values extracted from bindings #260)
"Typed" Binding values? (Proposal: Typed Values for TriggerBindings #253)
More focussed tests (Split out Test_NewResources into smaller tests #252)

/hold cancel

bobcatfish · 2019-12-10T20:50:08Z

Thanks for all the follow up issues @dibyom !! ❤️

pkg/template/jsonpath_test.go

This commit switches the expression syntax in TriggerBindings from GJson to JSONPath. Fixes tektoncd#178 Signed-off-by: Dibyo Mukherjee <[email protected]>

bobcatfish · 2019-12-10T21:34:25Z

/lgtm

bobcatfish · 2019-12-10T21:34:31Z

/meow space

tekton-robot · 2019-12-10T21:34:33Z

@bobcatfish:

In response to this:

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2019

tekton-robot requested review from dlorenc and ncskier November 27, 2019 19:31

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 27, 2019

dibyom force-pushed the jsonpath branch from 2d6d96b to a85fd79 Compare December 2, 2019 20:00

dibyom force-pushed the jsonpath branch from a85fd79 to 8150ab3 Compare December 3, 2019 00:13

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2019

dibyom force-pushed the jsonpath branch from 8150ab3 to 16969a1 Compare December 3, 2019 00:14

dibyom marked this pull request as ready for review December 3, 2019 00:14

dibyom changed the title ~~WIP: Jsonpath support~~ JSONPath Support for TriggerBindings Dec 3, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2019

dibyom requested a review from wlynch December 3, 2019 00:20

dibyom force-pushed the jsonpath branch from 16969a1 to 484de0f Compare December 3, 2019 00:23

wlynch reviewed Dec 3, 2019

View reviewed changes

dibyom force-pushed the jsonpath branch from 484de0f to 2a90518 Compare December 3, 2019 03:18

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 3, 2019

dibyom force-pushed the jsonpath branch from 2a90518 to 077ea13 Compare December 3, 2019 03:37

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 4, 2019

dibyom force-pushed the jsonpath branch from 58c20e5 to f1c9c3d Compare December 4, 2019 00:20

bobcatfish reviewed Dec 9, 2019

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2019

dibyom mentioned this pull request Dec 9, 2019

Escaping Quotes in TriggerBinding values #257

Closed

dibyom force-pushed the jsonpath branch from 4fe79f8 to 017ba82 Compare December 9, 2019 18:20

dibyom force-pushed the jsonpath branch 3 times, most recently from cc3b2f6 to 4b3dd7f Compare December 10, 2019 16:25

dibyom mentioned this pull request Dec 10, 2019

Inconsistent types for Header values extracted from bindings #260

Closed

dibyom force-pushed the jsonpath branch from 4b3dd7f to 3b70b51 Compare December 10, 2019 18:58

dibyom force-pushed the jsonpath branch 2 times, most recently from ec6a539 to 239553c Compare December 10, 2019 19:15

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2019

bobcatfish reviewed Dec 10, 2019

View reviewed changes

pkg/template/jsonpath_test.go Outdated Show resolved Hide resolved

dibyom force-pushed the jsonpath branch from 239553c to 7e51560 Compare December 10, 2019 21:21

Support JSONPath in TriggerBindings

2bc9a11

This commit switches the expression syntax in TriggerBindings from GJson to JSONPath. Fixes tektoncd#178 Signed-off-by: Dibyo Mukherjee <[email protected]>

dibyom force-pushed the jsonpath branch from 7e51560 to 2bc9a11 Compare December 10, 2019 21:32

tekton-robot assigned bobcatfish Dec 10, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019

tekton-robot merged commit 89813de into tektoncd:master Dec 10, 2019

dibyom deleted the jsonpath branch December 10, 2019 21:50

JSONPath Support for TriggerBindings #241

JSONPath Support for TriggerBindings #241

Conversation

dibyom commented Nov 27, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 27, 2019

tekton-robot commented Dec 2, 2019

tekton-robot commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

tekton-robot commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Dec 3, 2019

tekton-robot commented Dec 4, 2019

tekton-robot commented Dec 4, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Dec 9, 2019

dibyom commented Dec 9, 2019 • edited Loading

dibyom commented Dec 10, 2019

dibyom commented Dec 10, 2019 • edited Loading

bobcatfish commented Dec 10, 2019

bobcatfish commented Dec 10, 2019

bobcatfish commented Dec 10, 2019

tekton-robot commented Dec 10, 2019

dibyom commented Nov 27, 2019 •

edited

Loading

dibyom Dec 9, 2019 •

edited

Loading

dibyom commented Dec 9, 2019 •

edited

Loading

dibyom commented Dec 10, 2019 •

edited

Loading