Skip to content
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.

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

Fixed inconsistencies between listen and emit #943

Conversation

JBBianchi
Copy link
Member

Redo of #926

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Use Cases
  • Community
  • CTK
  • Other

Discussion or Issue link:
Closes #917 (and other little minor typo)

What this PR does:

  • Fixes inconsistencies between listen and emit
    • Added new event section in the specification documentation
    • Fixed the sample of emit in the specification documentation
    • Added new event definition in the JSON schema and referenced it in emit.event.with and eventFilter.with
  • Fixes a typo in the set feature
  • Fixed type of an http call endpoint by adding string
  • Added new (missing) endpoint section in the specification documentation
  • Escaped * character
  • Fixed double https://https://
  • Fixed links to runtime expressions
  • Fixed typo in implictly

Additional information:
-

Copy link
Collaborator

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

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

@JBBianchi I was wondering if it wont be better to have a schema with the data that is shared by emit and listen (which I feel should contain type, source, subject, dataContentType and time?) which will be used for listen filter and another one that inherits it (and adds id, data and dataschema?) and is used for emit
Then, you wont have to explain that id is required for emission only (you still have for source and type, which are shared by both), but at least a user wont be able to put id or data in a filter (I have doubts about if dataschema and time can be used for filtering or not, precisely to avoid this kind of doubt I feel we need two different schemas, that can share common properties)

@JBBianchi
Copy link
Member Author

@JBBianchi I was wondering if it wont be better to have a schema with the data that is shared by emit and listen (which I feel should contain type, source, subject, dataContentType and time?) which will be used for listen filter and another one that inherits it (and adds id, data and dataschema?) and is used for emit Then, you wont have to explain that id is required for emission only (you still have for source and type, which are shared by both), but at least a user wont be able to put id or data in a filter (I have doubts about if dataschema and time can be used for filtering or not, precisely to avoid this kind of doubt I feel we need two different schemas, that can share common properties)

Unless I'm mistaken, the current schema already shares the common properties and validates the ones that are required at their level. The explanation is necessary in the documentation because its describing the "shared" object properties.

As a side note, I'm still waiting for feedbacks ok @ricardozanini 's comment. I think it's a broader scope than just this PR though and would affect other object that could be built using runtime expressions. Should we open a dedicated issue ?

@fjtirado
Copy link
Collaborator

fjtirado commented Aug 2, 2024

@JBBianchi
But id and data should not be part of the event filter on listen isnt it?

@cdavernas
Copy link
Member

@fjtirado It can, but is not required

@fjtirado
Copy link
Collaborator

fjtirado commented Aug 2, 2024

@cdavernas Ok, that was my doubt, so we can filter by any cloud event field.

@fjtirado
Copy link
Collaborator

fjtirado commented Aug 2, 2024

Which remind me that maybe the right name for the schema, rather that eventproperties, should be cloudEvent?

@cdavernas
Copy link
Member

@fjtirado that was how JB named it first, but is IMHO opinion confusing. It's the properties/filter of the CloudEvent to produce/listen, not the event itself

Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Looks great to me, aside from 3 little comments! Thanks for your work ❤️

dsl-reference.md Show resolved Hide resolved
dsl-reference.md Show resolved Hide resolved
dsl.md Show resolved Hide resolved
…for_auth_schema

Adding missing titles for better pojo generation
@@ -1397,7 +1421,7 @@ An event filter is a mechanism used to selectively process or handle events base

| Property | Type | Required | Description |
|----------|:----:|:--------:|-------------|
| with | `object` | `yes` | A name/value mapping of the attributes filtered events must define. Supports both regular expressions and runtime expressions. |
| with | [`eventProperties`](#event-properties) | `yes` | A name/value mapping of the attributes filtered events must define. Supports both regular expressions and runtime expressions. |
Copy link
Member

Choose a reason for hiding this comment

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

I thought I was getting crazy since I couldn't see my previous comments from the other PR. Now I see you reopened it.

Regarding this property, since we are now defining that it can be regexp, runtime expressions, or eventProperties, this should reflect in the schema.

So this with should be an OneOf there amongst these types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the mix-up. When I rebased the branch, GitHub closed my previous PR. I opened a new one and realized afterward that I could have simply reopened the previous one after I readded my commits.

I might be mistaken, but I was under the impression that the statement "Supports both regular expressions and runtime expressions" applies to the properties of with, not the object itself. That said, there might be room for discussion about how and where we can use a runtime expression to construct an entire object, but that might be a broader topic than what is addressed in this PR.

@cdavernas @fjtirado @matthias-pichler-warrify, what are your thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would understand this to mean that the individual properties under with (such as id, source, ...) should each support literals, regex and runtime expressions. These are all strings so we would just produce a union of type string | string | string which doesn't seem necessary.
I would keep the with object an object.

regarding regular expressions: How would an implementor determine if a given string is a literal or a regular expression though?

For example if the filter was specified as

with:
  type: com.example  

if the string was interpreted as a literal only com.example would match. As a regex comaexample and com.example.foo would also match! Are we planning on adding a regex syntax (JSON schema support the ECMA 262 syntax with format: regex) or object? Like

with:
  type:
    re: ^com\.example

# or

with:
  type: /com\.example/

Maybe we should just stick to runtime expressions and use the test function: https://jqlang.github.io/jq/manual/#regular-expressions ?

with:
  source: ${ test(., "^com\\.example$") }

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. It's a very specific pain point considering regular expressions are only supported in listen tasks event filters.

My understanding was that the value was either a string or a runtime expression. If it's a string, then it's regarded as a regular expression (regexp). But it's not very clear, can introduce undesired matches when not properly escaped, and probably have an bad impact on performance (I would assume string equality to be more performant than regexp, in turn more performant than jq/js expressions.)

For the performance reason I just mentioned (without any proof though, it's only a feeling ^^'), I would rule out relying only on jq/js for processing regexp. Therefore, we need to either consider any string as a regexp or find a way to discriminate the regexp. I must admit I'm not convinced by neither adding a property nor using a specific format but I don't have a better option to suggest. Therefore, I would rather go for the specific format rather than the extra property.

Copy link
Collaborator

@matthias-pichler matthias-pichler Aug 5, 2024

Choose a reason for hiding this comment

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

(I would assume string equality to be more performant than regexp, in turn more performant than jq/js expressions.)

My intuition is that the performance difference between string equality and regex matching is negligible compared to jq expressions which are much slower. I however would not have a problem with taking the performance hit and only using runtime expressions

Therefore, I would rather go for the specific format rather than the extra property.

If we support regex I would also say that we should use a format such as /<regex>/. We would however not be able to rely on the format: regex from json schema since this does not have any delimiters

@cdavernas cdavernas added change: fix Something isn't working. Impacts in a minor version change. area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change. labels Aug 4, 2024
matthias-pichler and others added 27 commits August 4, 2024 18:30
Signed-off-by: Matthias Pichler <[email protected]>
Signed-off-by: Matthias Pichler <[email protected]>
…fy/fix-error-types

Remove duplicate uri scheme from error types
…fy/workflow-task-arg

Specify task and workflow arguments
…fy/string-substitution

Document limited uri-template support
…fy/query-params

Allow query parameters in call http
@JBBianchi JBBianchi closed this Aug 7, 2024
@JBBianchi JBBianchi deleted the fix-917-semantic-inconsistencies-listen-vs-emit branch August 7, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change. change: fix Something isn't working. Impacts in a minor version change.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Semantic inconsistencies between listen and emit tasks
5 participants