-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Cloud Events support to Spring Integration #10448
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
base: main
Are you sure you want to change the base?
Conversation
03d1f51
to
02a1329
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some at a glance review.
Thank you!
...src/main/java/org/springframework/integration/cloudevents/v1/CloudEventMessageConverter.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/integration/cloudevents/v1/CloudEventMessageConverter.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/springframework/integration/cloudevents/v1/CloudEventMessageConverter.java
Outdated
Show resolved
Hide resolved
...g/springframework/integration/cloudevents/v1/transformer/CloudEventMessageConverterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for a lengthy review and some doubts I've expressed.
I addition, what are your thoughts about content of this package in Spring Cloud Function: https://github.com/spring-cloud/spring-cloud-function/tree/main/spring-cloud-function-context/src/main/java/org/springframework/cloud/function/cloudevent ?
I mean transformer does the trick indeed, but only from an integration flow context.
How about the way to be able to construct CloudEvent
programmatically?
Or just existing SDK API is enough to deal with?
return CloudEventUtils.toReader((CloudEvent) payload).read(new MessageBuilderMessageWriter(headers, this.cePrefix)); | ||
} | ||
|
||
private MessageReader createMessageReader(Message<?> message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all of methods starting with this has to be static
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has not been addressed yet.
...ation/cloudevents/transformer/strategies/cloudeventconverter/CloudEventMessageConverter.java
Show resolved
Hide resolved
...ation/cloudevents/transformer/strategies/cloudeventconverter/MessageBinaryMessageReader.java
Show resolved
Hide resolved
...pringframework/integration/cloudevents/v1/transformer/ToCloudEventTransformerExtensions.java
Outdated
Show resolved
Hide resolved
...a/org/springframework/integration/cloudevents/v1/transformer/utils/HeaderPatternMatcher.java
Outdated
Show resolved
Hide resolved
...java/org/springframework/integration/cloudevents/v1/transformer/ToCloudEventTransformer.java
Outdated
Show resolved
Hide resolved
...java/org/springframework/integration/cloudevents/v1/transformer/ToCloudEventTransformer.java
Outdated
Show resolved
Hide resolved
* | ||
* @since 7.0 | ||
*/ | ||
public class CloudEventProperties { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure in this class.
Why all these properties cannot be on the transformer itself?
More over: how about expressions against the request message to determine all these options at runtime?
I think I had expressions in my original PoC: #3246.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any comments here, please?
* | ||
* @since 7.0 | ||
*/ | ||
public final class CloudEventsHeaders { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very convinced that we need this class at all.
Especially with the case when we can change the prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any comments, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CloudEventHeaders
can be changed from a class
to a record
, since it only contains constants. The CE_PREFIX
is used across all packages in the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I have to disagree.
Whatever place you use it is just artificial, having the fact that prefix could be customized.
I prefer to not have these constants at all.
We can just have ce-
hardcoded in the transformer (or converter).
As long as those specversion
and datacontenttype
.
Just because they simply become totally different values when we change the prefix (or don't use it at all).
src/reference/antora/modules/ROOT/pages/cloudevents/cloudevents-transform.adoc
Outdated
Show resolved
Hide resolved
cabc5e5
to
d14c5f8
Compare
return CloudEventUtils.toReader((CloudEvent) payload).read(new MessageBuilderMessageWriter(headers, this.cePrefix)); | ||
} | ||
|
||
private MessageReader createMessageReader(Message<?> message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has not been addressed yet.
* | ||
* @since 7.0 | ||
*/ | ||
public final class CloudEventsHeaders { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any comments, please?
|
||
import io.cloudevents.SpecVersion; | ||
import io.cloudevents.core.data.BytesCloudEventData; | ||
import io.cloudevents.core.impl.StringUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, consider to use org.springframework.util.StringUtils
instead.
I guess, the one in CloudEvents is just a copy of ours 😄
|
||
@Override | ||
protected boolean isContentTypeHeader(String key) { | ||
return org.springframework.messaging.MessageHeaders.CONTENT_TYPE.equalsIgnoreCase(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need FQCN?
|
||
@Override | ||
public Message<byte[]> end() { | ||
return MessageBuilder.withPayload(new byte[0]).copyHeaders(this.headers).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we could just call end(null)
from here to avoid duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. In this implementation the copyHeaders
is called while in the end(null)
does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I have to disagree.
Please, take a look into those two lines again:
MessageBuilder.withPayload(new byte[0]).copyHeaders(this.headers).build();
...
MessageBuilder.withPayload(value == null ? new byte[0] : value.toBytes()).copyHeaders(this.headers).build();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct I missed the parenthesis before the copyHeaders on my read . Will adjust.
Thank you for catching this.
* | ||
* @since 7.0 | ||
*/ | ||
public class MessageBuilderMessageWriter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DITTO.
Why public
?
private final Map<String, Object> filteredHeaders; | ||
|
||
/** | ||
* Constructs CloudEvent extensions by filtering message headers against patterns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method Javadoc must be imperative, like a command, not infinitive.
} | ||
|
||
public Map<String, Object> getFilteredHeaders() { | ||
return this.filteredHeaders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that filtering has to be a part of this implementation.
I think the transformer has to do the trick for categorizing headers.
And from there, it looks like this whole class could go as inner one into the transformer.
* @param messageHeaders the headers associated with the {@link Message} | ||
* @return the converted {@link Message} | ||
*/ | ||
Message<?> convert(CloudEvent cloudEvent, MessageHeaders messageHeaders); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure in this contract.
Looks like a duplication of the MessageConverter
.
I expected here something similar what you had before in the transformer for XML, AVRO etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. So, I found an original code in your first commit (sorry again for the noise!):
AvroCompactFormat avroFormat = new AvroCompactFormat();
byte[] avroBytes = avroFormat.serialize(cloudEvent);
return MessageBuilder.withPayload(avroBytes)
.copyHeaders(filterHeaders(originalHeaders))
.setHeader("content-type", "application/avro")
.build();
Therefore I imaging the strategy like this:
public interface FormatStrategy {
byte[] toByteArray(CloudEvent cloudEvent);
String getContentType();
}
And from here it sounds like a part of that our CloudEventMessageConverter
.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss. I'm not sure I understand.
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
class CloudEventPropertiesTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before: I'm not sure in the CloudEventProperties
class at all.
And our preferences is *Tests
for test class names.
Please, revise all of your new test classes.
For now hold off on a review. I'm writing up some questions for some guidance on some of the issues you raised in the last 2 reviews. |
* @param messageHeaders the headers associated with the {@link Message} | ||
* @return the converted {@link Message} | ||
*/ | ||
Message<?> convert(CloudEvent cloudEvent, MessageHeaders messageHeaders); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. So, I found an original code in your first commit (sorry again for the noise!):
AvroCompactFormat avroFormat = new AvroCompactFormat();
byte[] avroBytes = avroFormat.serialize(cloudEvent);
return MessageBuilder.withPayload(avroBytes)
.copyHeaders(filterHeaders(originalHeaders))
.setHeader("content-type", "application/avro")
.build();
Therefore I imaging the strategy like this:
public interface FormatStrategy {
byte[] toByteArray(CloudEvent cloudEvent);
String getContentType();
}
And from here it sounds like a part of that our CloudEventMessageConverter
.
WDYT?
Introduces Cloud Events v1.0 specification support including message converters, transformers, and utilities. Key components added: - CloudEventMessageConverter for message format conversion - ToCloudEventTransformer for transforming messages to Cloud Events - MessageBinaryMessageReader/Writer for binary format handling - CloudEventProperties for configuration management - Header pattern matching utilities for flexible event mapping - Add reference docs and what's-new paragraph
Remove v1 subpackage and flatten the CloudEvents package hierarchy. Introduce strategy pattern for format conversion to replace enum-based approach, improving extensibility and reduce dependencies. Key changes: - Move all classes from cloudevents.v1 to cloudevents base package - Remove optional format dependencies (JSON, XML, Avro) from build - Replace `ConversionType` enum with `FormatStrategy` interface - Add `CloudEventMessageFormatStrategy` as default implementation - Inline `HeaderPatternMatcher` logic into `ToCloudEventTransformerExtensions` - Add `@NullMarked` package annotations and `@Nullable` throughout - Document `targetClass` parameter behavior in `CloudEventMessageConverter` - Split transformer tests for better organization and coverage - Update component type identifier to "ce:to-cloudevents-transformer" - Remove unnecessary docs from package-info
|
||
@Override | ||
public Message<byte[]> end() { | ||
return MessageBuilder.withPayload(new byte[0]).copyHeaders(this.headers).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I have to disagree.
Please, take a look into those two lines again:
MessageBuilder.withPayload(new byte[0]).copyHeaders(this.headers).build();
...
MessageBuilder.withPayload(value == null ? new byte[0] : value.toBytes()).copyHeaders(this.headers).build();
* | ||
* @since 7.0 | ||
*/ | ||
public final class CloudEventsHeaders { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I have to disagree.
Whatever place you use it is just artificial, having the fact that prefix could be customized.
I prefer to not have these constants at all.
We can just have ce-
hardcoded in the transformer (or converter).
As long as those specversion
and datacontenttype
.
Just because they simply become totally different values when we change the prefix (or don't use it at all).
- Simplify the CloudEvent transformer by consolidating configuration directly into ToCloudEventTransformer class rather than using separate configuration objects - Remove CloudEventProperties and ToCloudEventTransformerExtensions classes to reduce abstraction layers and improve maintainability - Make MessageBinaryMessageReader package-private and convert CloudEventMessageConverter methods to static where possible - Move extension filtering logic into a private inner class within the transformer - Remove CloudEventsHeaders class and CE_PREFIX constant as the prefix is no longer used as a configurable value
d14c5f8
to
d8989f9
Compare
Introduces Cloud Events v1.0 specification support including message converters, transformers, and utilities.
Key components added: