Filtering fix for consistency in handling empty string as value by Shashankft9 · Pull Request #5273 · knative/eventing

Shashankft9 · 2021-04-19T09:20:05Z

Fixes #4847

Proposed Changes

setting dataschema - an optional attribute in v1.0 CE spec
added check if empty string value is used in filter for optional attributes and extensions.
added specific test cases for extensions and optional attribute - subject. Let me know if its better without some of them, I'll remove.

Release Note

Docs

…and extensions

knative-prow-robot · 2021-04-19T09:20:16Z

Hi @Shashankft9. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

codecov · 2021-04-19T09:51:44Z

Codecov Report

Merging #5273 (9838781) into main (97c0d30) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5273      +/-   ##
==========================================
+ Coverage   83.34%   83.39%   +0.04%     
==========================================
  Files         243      243              
  Lines        6882     6883       +1     
==========================================
+ Hits         5736     5740       +4     
+ Misses        797      795       -2     
+ Partials      349      348       -1

Impacted Files	Coverage Δ
pkg/eventfilter/attributes/filter.go	`100.00% <100.00%> (+13.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c0d30...9838781. Read the comment docs.

Shashankft9 · 2021-04-19T10:02:45Z

/assign @zhongduo

zhongduo · 2021-04-19T16:37:10Z

@pierDipi might have more context and opinion on this.

zhongduo · 2021-04-19T16:37:20Z

/assign @pierDipi

pierDipi · 2021-04-19T16:41:49Z

I'm gonna take a look at this soon! Thanks!

/cc

n3wscott · 2021-04-19T17:27:15Z

+		"dataschema":      event.DataSchema(),
 		"schemaurl":       event.DataSchema(),
 		"datacontenttype": event.DataContentType(),
 		"datamediatype":   event.DataMediaType(),


datamediatype is not a real attribute.

n3wscott · 2021-04-19T17:28:52Z

@@ -59,8 +60,10 @@ func (attrs attributesFilter) Filter(ctx context.Context, event cloudevents.Even
 	for k, v := range attrs {


This entire method should be rewritten to me more performant. No need to explode the event for filters that do not care about most of the attributes.

(this comment was not about this PR, but the code in general. I used it to make a new issue)

n3wscott · 2021-04-19T17:29:48Z

+		// If the attribute does not exist in the event (extension context attributes) or if the event attribute
+		// has an empty string value (optional attributes) - which means it was never set in the incoming event,
+		// return false.
+		if !ok || (v == eventingv1.TriggerAnyFilter && value == "") {


this looks correct.

n3wscott · 2021-04-19T17:30:02Z

 			event:  makeEventWithExtension(extensionName, extensionValue),
 			want:   eventfilter.PassFilter,
 		},
+		"Any Extension with attribs - without Extension in Event": {


Thank you for the tests 👍

n3wscott · 2021-04-19T17:30:25Z

/ok-to-test

Looks good to me!
/lgtm
/approve

knative-prow-robot · 2021-04-19T17:30:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [n3wscott]

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

knative-metrics-robot · 2021-04-19T17:35:27Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/eventfilter/attributes/filter.go	86.7%	100.0%	13.3

Shashankft9 added 4 commits April 19, 2021 03:20

filter-fix for empty string filters, dataschema added

eda629b

merged 2 similar if cases

81da624

test added to check for consistency in filtering optional attributes …

7c60956

…and extensions

comment added for this change

75c455d

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 19, 2021

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 19, 2021

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 19, 2021

gofmt done

9838781

Shashankft9 changed the title ~~Filter fix~~ Filtering fix for consistency in handling empty string as value Apr 19, 2021

knative-prow-robot assigned zhongduo Apr 19, 2021

knative-prow-robot assigned pierDipi Apr 19, 2021

knative-prow-robot requested a review from pierDipi April 19, 2021 16:41

n3wscott reviewed Apr 19, 2021

View reviewed changes

n3wscott mentioned this pull request Apr 19, 2021

[pkg/eventfilter/attributes/filter.go] Filter method should be rewritten to me more performant. #5277

Closed

n3wscott reviewed Apr 19, 2021

View reviewed changes

knative-prow-robot assigned n3wscott Apr 19, 2021

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 19, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2021

knative-prow-robot merged commit 0452204 into knative:main Apr 19, 2021

		@@ -59,8 +60,10 @@ func (attrs attributesFilter) Filter(ctx context.Context, event cloudevents.Even
		for k, v := range attrs {

Conversation

Shashankft9 commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Apr 19, 2021

Uh oh!

codecov Bot commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Shashankft9 commented Apr 19, 2021

Uh oh!

zhongduo commented Apr 19, 2021

Uh oh!

zhongduo commented Apr 19, 2021

Uh oh!

pierDipi commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n3wscott Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

n3wscott Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

n3wscott Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

n3wscott Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

n3wscott Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Apr 19, 2021

Uh oh!

knative-prow-robot commented Apr 19, 2021

Uh oh!

knative-metrics-robot commented Apr 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Shashankft9 commented Apr 19, 2021 •

edited

Loading

codecov Bot commented Apr 19, 2021 •

edited

Loading

pierDipi commented Apr 19, 2021 •

edited

Loading