Skip to content

Rename new trigger filters sql field to cesql #6148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow-robot merged 6 commits into from
Feb 26, 2022
Merged

Rename new trigger filters sql field to cesql #6148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
487f688
Rename new trigger filters SQL field to CESQL
devguyio Feb 15, 2022
ffb29af
run hack/update-codegen.sh
devguyio Feb 15, 2022
d8692ee
fix typo in docs
devguyio Feb 15, 2022
9119e20
fix validation cesql viaField
devguyio Feb 16, 2022
208e9fe
fix comment
devguyio Feb 16, 2022
f022bec
fix trigger validation tests
devguyio Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/eventing-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2091,14 +2091,14 @@ expression cannot be empty strings.</p>
</tr>
<tr>
<td>
<code>sql</code><br/>
<code>cesql</code><br/>
<em>
string
</em>
</td>
<td>
<em>(Optional)</em>
<p>SQL is a CloudEvents SQL expression that will be evaluated to true or false against each CloudEvent.</p>
<p>CESQL is a CloudEvents SQL expression that will be evaluated to true or false against each CloudEvent.</p>
</td>
</tr>
</tbody>
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/eventing/v1/trigger_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,10 @@ type SubscriptionsAPIFilter struct {
// +optional
Suffix map[string]string `json:"suffix,omitempty"`

// SQL is a CloudEvents SQL expression that will be evaluated to true or false against each CloudEvent.
// CESQL is a CloudEvents SQL expression that will be evaluated to true or false against each CloudEvent.
//
// +optional
SQL string `json:"sql,omitempty"`
Copy link
Member

@matzew matzew Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this proposal, and since we touch an alpha stage experimental feature, this seems right to do.

Makes it more explicit, rather than just a generic sql field on the API 👍

CESQL string `json:"cesql,omitempty"`
Copy link
Contributor

@n3wscott n3wscott Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a really confusing rename. Why not change Suffix to CESuffix and Exact to CEExact? Because those are all worse names and this filter is in the context of cloudevents filters. This is a docs problem, not an API naming problem.

Copy link
Contributor Author

@devguyio devguyio Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feedback we've had so far is related to the strong database related connotation that comes with sql. What I read from the feedback is that calling it cesql makes it instantly obvious that this is a custom expression language related to CloudEvents.

Copy link
Member

@aslom aslom Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @devguyio - renaming to CESQL will help to avoid lot of user confusion and may help with google search in future ...

Copy link
Member

@matzew matzew Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's beneficial to change the name

}

// TriggerFilterAttributes is a map of context attribute names to values for
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/eventing/v1/trigger_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func ValidateSubscriptionAPIFilter(ctx context.Context, filter *SubscriptionsAPI
).Also(
ValidateSubscriptionAPIFilter(ctx, filter.Not).ViaField("not"),
).Also(
ValidateCESQLExpression(ctx, filter.SQL).ViaField("sql"),
ValidateCESQLExpression(ctx, filter.CESQL).ViaField("cesql"),
)
return errs
}
Expand Down Expand Up @@ -275,7 +275,7 @@ func hasMultipleDialects(filter *SubscriptionsAPIFilter) bool {
dialectFound = true
}
}
if filter.SQL != "" && dialectFound {
if filter.CESQL != "" && dialectFound {
return true
}
return false
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/eventing/v1/trigger_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,14 +715,14 @@ func TestFilterSpecValidation(t *testing.T) {
name: "CE SQL with syntax error",
filters: []SubscriptionsAPIFilter{
{
SQL: "this is wrong",
CESQL: "this is wrong",
}},
want: apis.ErrInvalidValue("this is wrong", "sql", "syntax error: ").ViaFieldIndex("filters", 0),
want: apis.ErrInvalidValue("this is wrong", "cesql", "syntax error: ").ViaFieldIndex("filters", 0),
}, {
name: "Valid CE SQL expression",
filters: []SubscriptionsAPIFilter{
{
SQL: "type = 'dev.knative' AND ttl < 3",
CESQL: "type = 'dev.knative' AND ttl < 3",
}},
},
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/broker/filter/filter_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,10 +392,10 @@ func materializeSubscriptionsAPIFilter(ctx context.Context, filter eventingv1.Su
materializedFilter = subscriptionsapi.NewAnyFilter(materializeFiltersList(ctx, filter.Any)...)
case filter.Not != nil:
materializedFilter = subscriptionsapi.NewNotFilter(materializeSubscriptionsAPIFilter(ctx, *filter.Not))
case filter.SQL != "":
if materializedFilter, err = subscriptionsapi.NewCESQLFilter(filter.SQL); err != nil {
case filter.CESQL != "":
if materializedFilter, err = subscriptionsapi.NewCESQLFilter(filter.CESQL); err != nil {
// This is weird, CESQL expression should be validated when Trigger's are created.
logging.FromContext(ctx).Debugw("Found an Invalid CE SQL expression", zap.String("expression", filter.SQL))
logging.FromContext(ctx).Debugw("Found an Invalid CE SQL expression", zap.String("expression", filter.CESQL))
return nil
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/broker/filter/filter_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ func TestReceiver_WithSubscriptionsAPI(t *testing.T) {
"Dispatch succeeded - Source with type": {
triggers: []*eventingv1.Trigger{
makeTrigger(withSubscriptionAPIFilter(&eventingv1.SubscriptionsAPIFilter{
SQL: fmt.Sprintf("type = '%s' AND source = '%s'", eventType, eventSource),
CESQL: fmt.Sprintf("type = '%s' AND source = '%s'", eventType, eventSource),
})),
},
expectedDispatch: true,
Expand All @@ -574,7 +574,7 @@ func TestReceiver_WithSubscriptionsAPI(t *testing.T) {
triggers: []*eventingv1.Trigger{
makeTrigger(
withSubscriptionAPIFilter(&eventingv1.SubscriptionsAPIFilter{
SQL: fmt.Sprintf("type = '%s' AND source = '%s'", eventType, eventSource),
CESQL: fmt.Sprintf("type = '%s' AND source = '%s'", eventType, eventSource),
}),
withAttributesFilter(&eventingv1.TriggerFilter{
Attributes: map[string]string{"type": "some-other-type", "source": "some-other-source"},
Expand Down
4 changes: 2 additions & 2 deletions test/experimental/features/new_trigger_filters/filters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
// FiltersFeatureSet creates a feature set for testing the broker implementation of the new trigger filters experimental feature
// (aka Cloud Events Subscriptions API filters). It requires a created and ready Broker resource with brokerName.
//
// The feature set tests four filter dialects: exact, prefix, suffix and sql (aka CloudEvents SQL).
// The feature set tests four filter dialects: exact, prefix, suffix and cesql (aka CloudEvents SQL).
func FiltersFeatureSet(brokerName string) *feature.FeatureSet {
matchedEvent := FullEvent()
unmatchedEvent := MinEvent()
Expand All @@ -56,7 +56,7 @@ func FiltersFeatureSet(brokerName string) *feature.FeatureSet {
filters: fmt.Sprintf(snippetFor("suffix"), matchedEvent.Type()[5:], matchedEvent.Source()[5:]),
},
"CloudEvents SQL filter": {
filters: fmt.Sprintf(`- sql: "type = '%s' AND source = '%s'" `, matchedEvent.Type(), matchedEvent.Source()),
filters: fmt.Sprintf(`- cesql: "type = '%s' AND source = '%s'" `, matchedEvent.Type(), matchedEvent.Source()),
},
}

Expand Down