Skip to content

Conversation

@olegz
Copy link
Contributor

@olegz olegz commented Nov 20, 2020

This PR is the result of collaboration between Dave Syer(@dsyer ) and Oleg Zhurakousky(@olegz ) of Spring team (VMWARE).

It provides initial set of utility classes and interfaces to simplify integration of Cloud Events across many Spring projects especially those that are based on Spring Messaging.
It also provides an initial set of examples to demonstrate several web solutions (i.e., spring-cloud-function-web, spring-webmvc, spring-webflux) as well as streaming solution such as Kafka (but not limited to given an existing Spring binding support for many messaging brokers. More samples will come once the PR is accepted.

IMPORTANT:
At the moment there is an experimental support for Cloud Event in Spring Cloud Function which was recently released with version 3.1.0-M5.
However, there is also a cloudevents branch in Spring Cloud Function, which removes that support and instead relies on Java SDK - this PR.
So in a way this PR (to work properly) is contingent upon ‘cloudevents’ branch of Spring Cloud Function. That said, once this PR is merged the ‘cloudevents’ branch will also be merged to master.

Signed-off-by: Oleg Zhurakousky ozhurakousky@vmware.com

@olegz
Copy link
Contributor Author

olegz commented Nov 20, 2020

@salaboy this is what @dsyer had mentioned earlier

This PR is the result of collaboration between Dave Syer(@dsyer) and Oleg Zhurakousky(@olegz) of Spring team (VMWARE).

It provides initial set of utility classes and interfaces to simplify integration of Cloud Events across many Spring projects

Signed-off-by: Oleg Zhurakousky <ozhurakousky@vmware.com>

Removed maven wrappers and other
Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

Just did a quick pass, I'll do a proper review on monday

Comment on lines +41 to +52
String payloadWithHttpPrefix = "{\n" +
" \"ce-specversion\" : \"1.0\",\n" +
" \"ce-type\" : \"org.springframework\",\n" +
" \"ce-source\" : \"https://spring.io/\",\n" +
" \"ce-id\" : \"A234-1234-1234\",\n" +
" \"ce-datacontenttype\" : \"application/json\",\n" +
" \"ce-data\" : {\n" +
" \"version\" : \"1.0\",\n" +
" \"releaseName\" : \"Spring Framework\",\n" +
" \"releaseDate\" : \"24-03-2004\"\n" +
" }\n" +
"}";
Copy link
Member

Choose a reason for hiding this comment

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

More than hardcoding these, how about putting them in test resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally I like it that way for tests since it keeps everything in single place. That said I am ok to moving it if you feel strongly about it.

@@ -0,0 +1,12 @@
## Spring Support
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the badge and the pom dependency like the other modules? https://github.com/cloudevents/sdk-java/blob/master/kafka/README.md

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>2.4.0</version>
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to a property?

</parent>

<artifactId>cloudevents-spring</artifactId>
<name>CloudEvents - support for Spring</name>
Copy link
Member

Choose a reason for hiding this comment

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

I think CloudEvents - Spring is fine

<dependency>
<groupId>org.springframework.cloud</groupId>
<artifactId>spring-cloud-function-core</artifactId>
<version>3.1.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Ah ok so the snapshot is because of what you sad in the comments? Is it possible to merge the support in spring-cloud-function-core before merging this one, so we can point out to a proper release? I would love to avoid merging a module depending on snapshots

Copy link
Member

Choose a reason for hiding this comment

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

Also please make this version a property too

Comment on lines +120 to +127
<repository>
<id>spring-releases</id>
<name>Spring Releases</name>
<url>https://repo.spring.io/release</url>
<snapshots>
<enabled>false</enabled>
</snapshots>
</repository>
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these published in maven central too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

Don't you convert Message <-> CloudEvent? Why? I might miss some context here

@slinkydeveloper slinkydeveloper added the enhancement New feature or request label Nov 20, 2020
@slinkydeveloper slinkydeveloper added this to the 2.0.0 milestone Nov 20, 2020
@Mrc0113
Copy link

Mrc0113 commented Nov 20, 2020

+1 on this idea. Once merged I'd like to add a sample using CloudEvents with Spring Cloud Function and the Solace Spring Cloud Stream Binder as a way to exchange Cloud Events over Solace Event Brokers.

@olegz
Copy link
Contributor Author

olegz commented Nov 23, 2020

Don't you convert Message <-> CloudEvent? Why? I might miss some context here

@slinkydeveloper Sorry for the delayed response as this one required a bit more details, so here it is. . .

In SDK CloudEvent is a structure to represent the actual Cloud Event. Spring Message is another structure almost identical to CloudEvent structure capable of representing the actual Cloud Event.
In other words in theory you can use either.
However, given that many Spring-based frameworks and integrations with Spring are already relying/dependent on Spring Message it makes it a natural integration point with Cloud Events since you really don’t need to do much to receive, process and modify Cloud Events (as you can see in current set of examples). Also, as you yourself mentioned several times before, there isn’t a lot of expectation even with non-Spring SDK users to actually use CloudEvent type in their actual business applications effectively making CloudEvent more of a canonical structure to represent the cloud event within the SDK. That is exactly what Spring Message is within Spring.
In short, by relying on Spring Message as a structure to represent Cloud Event (and the supporting classes provided as part of this PR) you immediately get integration with any system, framework and application that relies on Spring Messaging.

That said, you question is still valid in the context of interoperability and integration of Spring and non-Spring based systems (which includes Spring frameworks that do not rely on Spring Messaging) at the SDK level and for that we would need to provide something along the lines of MessageConverter (standard Spring mechanism for Message type conversion) to do the to/from CloudEvent conversion, which we plan to do.

So while this is an initial PR providing integration with several framework from the Spring portfolio, it is hopefully not the last one.

@slinkydeveloper
Copy link
Member

Thanks for the explanation, I have a couple of comments:

However, given that many Spring-based frameworks and integrations with Spring are already relying/dependent on Spring Message it makes it a natural integration point with Cloud Events since you really don’t need to do much to receive, process and modify Cloud Events (as you can see in current set of examples). Also, as you yourself mentioned several times before, there isn’t a lot of expectation even with non-Spring SDK users to actually use CloudEvent type in their actual business applications effectively making CloudEvent more of a canonical structure to represent the cloud event within the SDK. That is exactly what Spring Message is within Spring.

Ok I'm trying to look at the examples to grasp how this integration looks like: https://github.com/cloudevents/sdk-java/pull/293/files#diff-3650ef1172cc81f44f7ba479f7399abf655d6ac8b4b88c4f3865652b07c969afR55-R67 <- here what if the user wants to send a custom CloudEvent, like defining an extension if something particular is in the input message? Why that function is not Function<CloudEvent, CloudEvent>? I would expect the sdk to provide the necessary bits for Spring to go from X to CloudEvent and back, so then Spring can let users write Function<CloudEvent, CloudEvent>

the SDK level and for that we would need to provide something along the lines of MessageConverter (standard Spring mechanism for Message type conversion) to do the to/from CloudEvent conversion, which we plan to do.

I wonder if we should place exactly this inside the sdk, to cover the use case above underlined (if i understood properly how spring works), more than the code provided in this PR, at least as first step. The integrations in this sdk provide the conversion X <-> CloudEvent, in order to allow people to integrate several systems using CloudEvent. That's the major goal of these integrations: then, for each integration, we might add on top the "niceties" to better integrate with the framework (eg in Kafka we provide a MessageWriter that provides ProducerRecord <-> CloudEvent, then we provide a CloudEventSerializer to play nice with the kafka client).
At least from my understanding of Spring, this PR includes the "niceties" without providing/nor relying on the basic Message <-> CloudEvent conversion, which is the goal of the integration.
Also did you tried to use MessageReader/MessageWriter abstractions to convert back and forth to CloudEvent from Spring's Message? We use these in other integrations (the last merged one is this: https://github.com/cloudevents/sdk-java/tree/master/amqp) and it would be nice to keep everything consistent. I ask because I see a tons of code we could avoid just using them, in particular, seems it's reimplementing the logic behind MessageReader and MessageWriter https://github.com/cloudevents/sdk-java/pull/293/files#diff-f195f970ff73a62bf30fc327ada222ed2a1c5c50f8d0175b0459295162834062. If they're not suitable, can you provide some feedback on how we could improve them?

Last but not least, I might miss something, but in this PR I don't see support nor for structured mode nor for extensions. Our integrations should be 100% compatible and compliant with the CloudEvent spec, so we cannot neglect these 2 features

@dsyer
Copy link
Contributor

dsyer commented Nov 23, 2020

I don't see support nor for structured mode nor for extensions

Structured mode is definitely covered (e.g. see the samples - the HTTP one has both binary and structured very explicitly, and the Cloud Function samples support both as well). The presence of the constant CloudEventAttributeUtils.DATA is probably a sign that it's there. Extensions are not covered yet. I think maybe because we don't understand the SDK for them - e.g. the spec says they have the same type system as attributes, but the SDK only supports String, Number and Boolean.

Also did you tried to use MessageReader/MessageWriter abstractions to convert back and forth to CloudEvent from Spring's Message?

Yes, we looked at that, but it's the wrong level of abstraction really. Spring already handles all those concerns for HTTP, Kafka, etc. So we only really need the CE domain for the attributes (and probably extensions as you point out).

I have an alternative implementation that ships most of the CE domain concerns back to the core SDK:
https://github.com/dsyer/sdk-java/tree/core. I think I might prefer that approach, but it's kind a a poor fit still because the only object in the CE API that represents both attributes and extensions is CloudEvent. It would be a better fit if there was a CloudEventHeaders that extended CloudEventAttributes and CloudEventExtensions (but that might mean quite a big change to core to extract most of CloudEventBuilder into a CloudEventHeadersBuilder).

The integrations in this sdk provide the conversion X <-> CloudEvent

That much is clear. But it isn't a very useful model for the majority of Spring applications, mainly (I think) because CloudEventData is the wrong abstraction for them. It might be something we could offer (for minority cases), and then you could have your Function<CloudEvent,CloudEvent> quite easily. An alternative formulation would be to use Message<CloudEventData> (also probably quite easy for a user to implement already). I'm not sure either is very helpful in practice, but there's no reason not to provide the machinery.

Note that if CloudEventData was just Object (or a type parameter) we could support it easily and very directly (since it would map directly to Message and HttpEntity that we already use extensively).

@slinkydeveloper
Copy link
Member

slinkydeveloper commented Nov 23, 2020

Structured mode is definitely covered (e.g. see the samples - the HTTP one has both binary and structured very explicitly, and the Cloud Function samples support both as well)

I'm sorry I didn't saw it! Do you mean this one? https://github.com/cloudevents/sdk-java/pull/293/files#diff-cdd5264380e57fcd33464c1b7992c9d64b37c6b8ca62299fdc154a00cfb9018bR54-R71
Can't the user transparently handle both structured and binary with the same handler? Also Why she/he needs to write 2 handlers for that? As a counter example, here both binary and structured are handled transparently https://github.com/cloudevents/sdk-java/pull/288/files#diff-d32fb9f764f16209b2cb69f1239b4309975c5601affe825cc4ff9428eca42d1bR44
Also, to convert from json to CloudEvent, do you use EventFormatProvider (which provides support for potentially other Event formats too like protobuf and avro)?

It would be a better fit if there was a CloudEventHeaders that extended CloudEventAttributes and CloudEventExtensions (but that might mean quite a big change to core to extract most of CloudEventBuilder into a CloudEventHeadersBuilder)

I guess we could easily provide that creating a CloudEventContext which extends CloudEventAttributes and CloudEventExtension

Extensions are not covered yet. I think maybe because we don't understand the SDK for them - e.g. the spec says they have the same type system as attributes, but the SDK only supports String, Number and Boolean.

Yes this is kinda a tricky corner case of the spec: extensions supports the same type system of the attributes, but they don't carry the type information with them when they're converted back and forth to protocol binding/event formats (e.g. think about http when the extensions are sent as headers). So, as a general form of abstraction, string/number/boolean is what we support today (which maps more or less to json extension types). But that part of the sdk has definitely room for improvements.

I have an alternative implementation that ships most of the CE domain concerns back to the core SDK:
https://github.com/dsyer/sdk-java/tree/core. I think I might prefer that approach, but it's kind a a poor fit still because the only object in the CE API that represents both attributes and extensions is CloudEvent. It would be a better fit if there was a CloudEventHeaders that extended CloudEventAttributes and CloudEventExtensions (but that might mean quite a big change to core to extract most of CloudEventBuilder into a CloudEventHeadersBuilder).

So if I understood your point correctly, you're saying that the sdk should not deal with payloads, which are usually handled by other abstractions (like Spring's Message), but it should handle only the cloudevent context (attributes and extensions). Am I right? Then how do you deal with unpacking the structured messages transparently then? How do you pass both the context and the payload to another system you're integrating? (say, a library which accepts CloudEvents as input)

Note that if CloudEventData was just Object (or a type parameter) we could support it easily and very directly (since it would map directly to Message and HttpEntity that we already use extensively).

You can in fact: you could or implement your own CloudEventData which has a method like get() that returns object and that's it. The point of CloudEventData is to carry any data you want, but just making sure it's serializable back to bytes.

@olegz
Copy link
Contributor Author

olegz commented Nov 23, 2020

I'm sorry I didn't saw it! Do you mean this one? https://github.com/cloudevents/sdk-java/pull/293/files#diff-cdd5264380e57fcd33464c1b7992c9d64b37c6b8ca62299fdc154a00cfb9018bR54-R71
Can't the user transparently handle both structured and binary with the same handler? Also Why she/he needs to write 2 handlers for that? As a counter example, here both binary and structured are handled transparently https://github.com/cloudevents/sdk-java/pull/288/files#diff-d32fb9f764f16209b2cb69f1239b4309975c5601affe825cc4ff9428eca42d1bR44
Also, to convert from json to CloudEvent, do you use EventFormatProvider (which provides support for potentially other Event formats too like protobuf and avro)?

That is one of the example using spring-webmvc. No, you don't need to write a different handler between the structured and binary. The examples simply show variety of signatures you can use in thte context of a particular execution and have no bearings on the structure of the incoming event as you can see in this example.

Also, to convert from json to CloudEvent, do you use EventFormatProvider

In Spring, MessageConverter is the standard mechanism to provide type conversion of this sort and we have a large library of existing MessageConverters (including Avro, variety of Json and more). Further more it is a natural extension point in practically all frameworks that rely on Spring Messaging, which means you can simply implement MessageConverter yourself for some imaginary format, add it as a bean (@Bean) and it will be picked up by the framework automatically as you can see in one of the examples that are on the master of s-c-function. Here you see we have imaginary format with content-type foo/bar.

Also, as you said yourself several month ago:
...the business logic of an application should not depend on CloudEvent, which is merely a standard exchange format, but on its attributes and payload....
While I am not in the position to argue either side of this argument, I do agree that most of the consumers of Cloud Event need to deal with its payload (data). However some time they may need to have access to meta-data (attributes and extensions) and Message structure supports all of these usage patterns. In fact we have identical concerns in Messaging. You may only care about payload - Function<Foo, Bar> or if you also care about meta-data Function<Message<Foo>, Bar>. The same applies for REST controllers as well as other abstractions. As a user you have flexibility of signatures and
underlying frameworks know how the incoming data should be mapped to user types. You can clearly see it in the provided examples. This one is dependent on Spring Cloud Stream receiving events over Kafka (in this example), yet as a user you only care about SpringReleaseEvent (an imaginary POJO) and it is the framework responsibility to serve it to you as you expect while handling all the internals such as type conversion, connectivity to messaging systems, transaction, acks etc,.

How do you pass both the context and the payload to another system you're integrating?

Spring Message defines payload (Object) and headers (Map<String, Object>). That is what I meant earlier when I said that it is adequately positioned to represent Cloud Event where data maps to payload and attributes and extension map to headers.

@olegz
Copy link
Contributor Author

olegz commented Nov 25, 2020

I am closing it it in favor of #302 from @dsyer, since it is more alined with the actual SDK implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants