Preserve escaped characters when parsing json path 🦟 #279

bobcatfish · 2019-12-16T15:08:57Z

Changes

I was trying to use Triggers to deliver a payload that included some
newlines and carriage returns into the value of a Pipeline param, but
along the way they were being turned from actual \ + n to the newline
character, etc., which when embedded into the yaml of the CRD I wanted
to create was invalid.

Now we will treat json strings as json when using the values via json
path instead of decoding the the escaped characters.

Dropping the quotation marks seems like a pretty big hack but it's the
best option we could think of and seems to consistently give us the
results we'd expect as users.

Co-authored-by: Dibyo Mukherjee [email protected]

Fixes #258

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)
[n/a] Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Can now handle using json path to extract payloads that contain escape sequences such as \r and \n

tekton-robot · 2019-12-16T15:10:53Z

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/resources/create.go	82.9%	81.4%	-1.5

tekton-robot · 2019-12-16T15:11:39Z

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/resources/create.go	82.9%	81.4%	-1.5

dibyom

A couple of minor things in the tests but otherwise looks great!

dibyom · 2019-12-16T16:11:39Z

pkg/resources/create_test.go

-			},
-			Spec: pipelinev1.PipelineResourceSpec{},
-		},
+		json: json.RawMessage("{\"kind\":\"PipelineResource\",\"apiVersion\":\"tekton.dev/v1alpha1\",\"metadata\":{\"name\":\"my-pipelineresource\",\"creationTimestamp\":null,\"labels\":{\"woriginal-label-1\":\"label-1\"}},\"spec\":{\"type\":\"\",\"params\":[{\"name\":\"foo\",\"value\":\"bar\\r\\nbaz\"}]},\"status\":{}}"),


Could we use raw strings `` here...then we could lose the escaping around the quotes "?

good call! :D

dibyom · 2019-12-16T16:25:36Z

pkg/template/event_test.go

+			if err != nil {
+				t.Fatalf("ResolveParams() returned unexpected error: %s", err)
+			}
+			trans := cmp.Transformer("Sort", func(in []pipelinev1.Param) []pipelinev1.Param {


Could we use cmpopts.SortSlices here as we do in resource_test.go?

triggers/pkg/template/resource_test.go

Line 117 in 5d31c96

if diff := cmp.Diff(tt.want, got, cmpopts.SortSlices(compareParam)); diff != "" {

ah kk that's simpler, i didnt know about that!! only downside i can think of is that'll sort any slice, not just pipeline params? maybe thats actually a good thing - usually we don't care about order

only downside i can think of is that'll sort any slice, not just pipeline params? maybe thats actually a good thing - usually we don't care about order

nvm, just saw the value of compareParam! no downsides at all :D

dibyom · 2019-12-16T16:35:01Z

pkg/template/jsonpath.go

@@ -89,7 +89,16 @@ func textValue(v reflect.Value) ([]byte, error) {

 	switch t.Kind() {
 	// evalToText() returns <map> ....; return JSON string instead.
-	case reflect.Map, reflect.Slice:
+	case reflect.String:


I wonder if we can always use json.Marshal and get rid of the evalToText altogether....we'd still need a special case for strings to remove the quotes but for every other type I think using json.Marshal would give us what we want!
(Just a thought.....if we decide to do this, we should do it in a followup)

ah interesting idea! ill see what happens to our test cases at least :D

dibyom · 2019-12-16T16:35:21Z

pkg/template/jsonpath_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 )

-var objects = `{"a":"v","c":{"d":"e"},"empty": "","null": null, "number": 42}`
+var objects = `{"a":"v\r\n烈","c":{"d":"e"},"empty": "","null": null, "number": 42}`


dibyom · 2019-12-16T16:48:09Z

test/eventlistener_test.go

 )

+func loadExamplePREventBytes() ([]byte, error) {


This is great! I think we should have more tests that use actual event payload...and not just in the e2e test but also in the sink tests!

dibyom · 2019-12-16T16:53:39Z

I1216 15:25:23.555] {"level":"error","logger":"eventlistener","caller":"sink/sink.go:126","msg":"Could not create resources for "": couldn't create resource with group version kind "tekton.dev/v1alpha1, Resource=pipelineresources": PipelineResource.tekton.dev "pr2" is invalid: metadata.labels: Invalid value: "3.52140673e+08": name part must consist of alphanumeric characters, '-', '' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9.]*)?[A-Za-z0-9]')","knative.dev/controller":"eventlistener","/triggers-eventid":"xj489","/trigger":"","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:126"}

🤔

dibyom · 2019-12-16T17:07:08Z

test/eventlistener_test.go

@@ -111,10 +124,11 @@ func TestEventListenerCreate(t *testing.T) {
 		bldr.TriggerTemplate("my-triggertemplate", "",
 			bldr.TriggerTemplateSpec(
 				bldr.TriggerTemplateParam("oneparam", "", ""),
-				bldr.TriggerTemplateParam("twoparamname", "", ""),
+				bldr.TriggerTemplateParam("twopgo test -v -tags=e2e -count=1 ./test -run ^TestEventListeneraramname", "", ""),


Is this intentional? As in do we expect the name (and not the value) of a template param to contain spaces and special chars?

something is going on lately where i randomly paste into my IDE all the time. i need to get to the bottom of this XD

dibyom · 2019-12-16T17:10:12Z

pkg/resources/create.go

@@ -100,8 +100,12 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
 		Resource: apiResource.Name,
 	}

-	_, err = dc.Resource(gvr).Namespace(namespace).Create(data, metav1.CreateOptions{})
-	return err
+	logger.Infof("Creating resource with payload: %v", data)


I wonder if should be logging the whole payload here...just looking at the integration test logs, the payload can he huge (especially if we are passing through large chunks of the incoming body)....maybe we log the gvk and the eventID by default...and if the resource creation fails, then we log the whole payload? (Another option would be to log at the Debug level instead of Info)

ah yeah good call - ill make it shorter for now. as a follow up, i think it could be useful to make these payloads available on disk somewhere for debugging (i hacked that into the github interceptor to get this payload)

dibyom · 2019-12-16T17:14:34Z

test/eventlistener_test.go

@@ -245,8 +266,10 @@ func TestEventListenerCreate(t *testing.T) {
 		Spec: v1alpha1.PipelineResourceSpec{
 			Type: "git",
 			Params: []v1alpha1.ResourceParam{
-				{Name: "body", Value: strings.Replace(`{"one": "zonevalue", "two": {"name": "zfoo", "value": "bar"}}`, " ", "", -1)},


I think this zonevalue came from the previous body, so we'd have to replace all instances of it

bobcatfish · 2019-12-17T15:56:58Z

I1216 15:21:26.950] level=warning msg="[runner] Can't run linter goanalysis_metalinter: ctrlflow: failed prerequisites: [email protected]/tektoncd/triggers/test/builder [github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1.test]"
I1216 15:21:26.951] level=error msg="Timeout exceeded: try increase it by passing --timeout option"

wut

bobcatfish · 2019-12-17T19:15:57Z

🤔

whoops i think i was using a value for a label that I shouldnt XD

tekton-robot · 2019-12-19T21:24:44Z

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/resources/create.go	86.8%	85.0%	-1.8
test/params.go	Do not exist	0.0%

dibyom · 2019-12-19T21:35:50Z

test/params.go

+	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
+)
+
+// CompareParams can be used with comparison options such as `cmpopts.SortSlices`


tekton-robot · 2019-12-19T21:37:44Z

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/resources/create.go	86.8%	85.0%	-1.8
test/params.go	Do not exist	0.0%

dibyom · 2019-12-19T21:37:37Z

test/eventlistener_test.go

+// when the values aren't equal if they can be unmarshalled into the license object and if they are
+// then equal. This is because the order of values in a dictionary is not deterministic and dictionary
+// values passed through an event listener may change order.
+func compareParamsWithLicenseJSON(x, y v1alpha1.ResourceParam) bool {


Is this different from unmarshalling and then using cmp.Diff?

Okay now im unmarshalling + then using cmp.Diff! Most of the function remains the same but now I don't have to manually check all the fields.

I feel like I'm slowly starting to understand how all the unmarshalling and marshalling works - cant wait to stop working on it and forget in 6 months X'D

tekton-robot · 2019-12-19T21:53:27Z

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/resources/create.go	86.8%	85.0%	-1.8
test/params.go	Do not exist	0.0%

tekton-robot · 2019-12-19T21:54:31Z

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/resources/create.go	86.8%	85.0%	-1.8
test/params.go	Do not exist	0.0%

tekton-robot · 2019-12-19T22:03:15Z

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/resources/create.go	86.8%	85.0%	-1.8
test/params.go	Do not exist	0.0%

I was trying to use Triggers to deliver a payload that included some newlines and carriage returns into the value of a Pipeline param, but along the way they were being turned from actual \ + n to the newline character, etc., which when embedded into the yaml of the CRD I wanted to create was invalid. Now we will treat json strings as json when using the values via json path instead of decoding the the escaped characters. Dropping the quotation marks seems like a pretty big hack but it's the best option we could think of and seems to consistently give us the results we'd expect as users. In the end to end test, when json dictionaries are passed through an EventListener, the order may change, so we are deserializing the example data so that we can compare it in an order agnostic manner. Fixes tektoncd#258 Co-authored-by: Dibyo Mukherjee <[email protected]>

bobcatfish · 2019-12-19T22:10:21Z

PLEASE LINTER PLEASE

bobcatfish · 2019-12-19T22:10:51Z

tekton-robot · 2019-12-19T22:11:15Z

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/resources/create.go	86.8%	85.0%	-1.8
test/params.go	Do not exist	0.0%

dibyom

/lgtm

tekton-robot · 2019-12-20T06:53:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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~~ [dibyom]

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

tekton-robot requested review from dibyom and ncskier December 16, 2019 15:09

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 16, 2019

bobcatfish force-pushed the escape branch from e21fc6e to d7e2f7c Compare December 16, 2019 15:09

dibyom approved these changes Dec 16, 2019

View reviewed changes

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

dibyom reviewed Dec 16, 2019

View reviewed changes

bobcatfish mentioned this pull request Dec 19, 2019

Change sink response to return JSON #261

Merged

3 tasks

bobcatfish force-pushed the escape branch 2 times, most recently from db23337 to 19cee57 Compare December 19, 2019 21:22

dibyom reviewed Dec 19, 2019

View reviewed changes

bobcatfish force-pushed the escape branch from 19cee57 to fe20b27 Compare December 19, 2019 21:36

dibyom reviewed Dec 19, 2019

View reviewed changes

bobcatfish force-pushed the escape branch from fa07c73 to ca70f56 Compare December 19, 2019 21:52

bobcatfish force-pushed the escape branch from ca70f56 to ec0b059 Compare December 19, 2019 22:01

bobcatfish force-pushed the escape branch from ec0b059 to abb1988 Compare December 19, 2019 22:09

dibyom approved these changes Dec 20, 2019

View reviewed changes

tekton-robot assigned dibyom Dec 20, 2019

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

tekton-robot merged commit b5944e5 into tektoncd:master Dec 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve escaped characters when parsing json path 🦟 #279

Preserve escaped characters when parsing json path 🦟 #279

bobcatfish commented Dec 16, 2019 •

edited

Loading

tekton-robot commented Dec 16, 2019

tekton-robot commented Dec 16, 2019

dibyom left a comment

dibyom Dec 16, 2019

bobcatfish Dec 17, 2019

dibyom Dec 16, 2019

bobcatfish Dec 17, 2019

bobcatfish Dec 17, 2019

dibyom Dec 16, 2019

bobcatfish Dec 17, 2019

dibyom Dec 16, 2019

dibyom Dec 16, 2019

dibyom commented Dec 16, 2019

dibyom Dec 16, 2019

bobcatfish Dec 16, 2019

bobcatfish Dec 16, 2019

dibyom Dec 16, 2019

bobcatfish Dec 17, 2019

dibyom Dec 16, 2019

bobcatfish commented Dec 17, 2019

bobcatfish commented Dec 17, 2019

tekton-robot commented Dec 19, 2019

dibyom Dec 19, 2019

tekton-robot commented Dec 19, 2019

dibyom Dec 19, 2019

bobcatfish Dec 19, 2019

tekton-robot commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

bobcatfish commented Dec 19, 2019

bobcatfish commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

dibyom left a comment

tekton-robot commented Dec 20, 2019

Preserve escaped characters when parsing json path 🦟 #279

Preserve escaped characters when parsing json path 🦟 #279

Conversation

bobcatfish commented Dec 16, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 16, 2019

tekton-robot commented Dec 16, 2019

dibyom 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

dibyom commented Dec 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Dec 17, 2019

bobcatfish commented Dec 17, 2019

tekton-robot commented Dec 19, 2019

Choose a reason for hiding this comment

tekton-robot commented Dec 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

bobcatfish commented Dec 19, 2019

bobcatfish commented Dec 19, 2019

tekton-robot commented Dec 19, 2019

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 20, 2019

bobcatfish commented Dec 16, 2019 •

edited

Loading