Add defaultEventSource to WorkflowApplication#1556
Conversation
|
@matheusandre1 you must sign your commits with |
Sorry, I always forget about that. |
fjtirado
left a comment
fjtirado
left a comment
There was a problem hiding this comment.
@matheusandre1
Lets make default event source a WorkflowValueResolver, so it returns an URI from the WorkflowContext, TaskContext and Model
bd2d409 to
37dd274
Compare
| } | ||
|
|
||
| public Builder withDefaultEventSource(String defaultEventSource) { | ||
| return withDefaultEventSource(URI.create(defaultEventSource)); |
There was a problem hiding this comment.
If the string is not a valir URI, application initialization will fail. Im not sure we should have this method to be hones
There was a problem hiding this comment.
Yeah, it must fail fast.
There was a problem hiding this comment.
If the string is not a valir URI, application initialization will fail. Im not sure we should have this method to be hones
I'm a little confused here, should I remove it or keep it?
There was a problem hiding this comment.
@fjtirado we must have somethign for the downstream client to implement/apply as a supplier. For instance, in Quarkus Flow we want to set the default source to /{namespace}/{name}/{version}.
There was a problem hiding this comment.
yes, thats the WorkflowValueResolver<URI) that accepts the three parameters. This is a particular case where the source is a constant string which Im not sure is useful and is erro prompt if the string cannot be converted to an URI (note that I did not question the constant URI one)
There was a problem hiding this comment.
Ok makes sense removing it then.
There was a problem hiding this comment.
Pull request overview
This PR improves CloudEvent emission defaults by introducing an application-level default source resolver and by ensuring emitted events always have a time value, aligning emitted events more closely with the Serverless Workflow spec requirements discussed in #1554.
Changes:
- Added
defaultEventSourcetoWorkflowApplication(configurable via builder, with a sensible default resolver). - Updated
EmitExecutorto defaultsourcevia the application’s resolver and to always settime(defaulting toOffsetDateTime.now()). - Added test coverage (including a YAML-based workflow sample) verifying defaulting and precedence behavior for
source,time, andid.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| impl/test/src/test/resources/workflows-samples/emit-default-source.yaml | Adds a YAML workflow sample used to verify default CloudEvent source behavior when loading workflows from YAML. |
| impl/test/src/test/java/io/serverlessworkflow/impl/test/EmitExecutorTest.java | Adds tests covering source defaulting/override precedence, and time/id defaults for emitted events. |
| impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java | Introduces defaultEventSource on the application and builder APIs to configure it, with a default resolver. |
| impl/core/src/main/java/io/serverlessworkflow/impl/executors/EmitExecutor.java | Switches emitted event source defaulting to use WorkflowApplication.defaultEventSource() and defaults time to now. |
| impl/core/src/main/java/io/serverlessworkflow/impl/events/EmitSourceResolver.java | Adds the default resolver implementation that builds source from application id + workflow definition id. |
| impl/core/src/main/java/io/serverlessworkflow/impl/events/CloudEventUtils.java | Updates the static fallback source() URI value to a more meaningful identifier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@matheusandre1 when generating the |
Signed-off-by: Matheus André <[email protected]>
| ceBuilder.withSource( | ||
| props | ||
| .sourceFilter() | ||
| .map(filter -> filter.apply(workflow, taskContext, taskContext.input())) | ||
| .map(URI::create) |
| public Builder withDefaultEventSource(URI defaultEventSource) { | ||
| return withDefaultEventSource((workflow, task, model) -> defaultEventSource); | ||
| } | ||
|
|
||
| public Builder withDefaultEventSource(WorkflowValueResolver<URI> defaultEventSource) { | ||
| this.defaultEventSource = defaultEventSource; | ||
| return this; | ||
| } |
| void whenTimeIsNotSpecified_defaultsToNow() throws IOException { | ||
| Workflow wf = emitWorkflow("emit-time-default", "org.example.event"); | ||
| List<CloudEvent> events = runWorkflow(wf, "org.example.event"); | ||
|
|
||
| assertThat(events).hasSize(1); | ||
| CloudEvent ev = events.get(0); | ||
| OffsetDateTime now = OffsetDateTime.now(); | ||
| assertThat(ev.getTime()).isNotNull(); | ||
| assertThat(ev.getTime()).isBeforeOrEqualTo(now); | ||
| assertThat(ev.getTime()).isAfter(now.minusSeconds(30)); | ||
| } |
Many thanks for submitting your Pull Request ❤️!
What this PR does / why we need it: Closes: #1554
Special notes for reviewers:
Additional information (if needed):