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

Fix Content-Type mismatch in publishEvent call #861

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MregXN
Copy link
Member

@MregXN MregXN commented May 20, 2023

Description

Please reference the issue #779

If contentType is set as ""text/plain" serialization will be skipped.

Signed-off-by: MregXN <[email protected]>
@MregXN MregXN requested review from a team as code owners May 20, 2023 16:12
@MregXN
Copy link
Member Author

MregXN commented May 26, 2023

Hi @artursouza , if I modify the serialization method of the serializer (to exclude quotation marks when serializing a string), I will also need to modify the deserialization method and its corresponding tests, otherwise I may fail to pass most of the tests.

@artursouza
Copy link
Member

I will need to think this through. It is not a simple change without breaking existing apps.

@MregXN
Copy link
Member Author

MregXN commented Jun 1, 2023

I will need to think this through. It is not a simple change without breaking existing apps.

After I debugged step by step, it is found that this bug is not directly related to the java-sdk serializer. The reason can be divided into the following two parts:

  1. When publishing data, in addition to using the serializer, the data will be encapsulated into a cloudenent envelop in dapr runtime, and the data will be further encapsulated according to the dataContentType set by the user:
// NewCloudEventsEnvelope returns a map representation of a cloudevents JSON.
func NewCloudEventsEnvelope(id, source, eventType, subject string, topic string, pubsubName string,
	dataContentType string, data []byte, traceParent string, traceState string,
) map[string]interface{} {
	....

	var ceData interface{}
	ceDataField := DataField
	var err error
	if contribContenttype.IsJSONContentType(dataContentType) {
		err = unmarshalPrecise(data, &ceData)
	} else if contribContenttype.IsBinaryContentType(dataContentType) || contribContenttype.IsCloudEventProtobuf(dataContentType, data) {
		ceData = base64.StdEncoding.EncodeToString(data)
		ceDataField = DataBase64Field
	} else {
		ceData = string(data)
	}

	if err != nil {
		ceData = string(data)
	}

	....
}

And there are similar operations when using gRPC protocal. Can find more details in dapr/pkg/runtime/runtime.go publishMessageGRPC()

So the string "This is message #0" will store differently in this envelop if user set the dataContentType as text/plain instead of application/json .

  1. But when subscribe there is no further process of the data according to the dataContentType.

As http subscriber controller for example, it uses the annotation @RequsetBody to parse the data to CloudEvent in default way. But there is no further process of the data according to the dataContentType

...
    @Topic(...)
    @PostMapping(...)
    public Mono<ResponseEntity> getCheckout(@RequestBody(required = false) CloudEvent<Order> cloudEvent) {
        return Mono.fromSupplier(() -> {
            try {
                logger.info("Subscriber received: " + cloudEvent.getData().getOrderId());
                return ResponseEntity.ok("SUCCESS");
            } catch (Exception e) {
                throw new RuntimeException(e);
            }
        });
    }
...

gPRC subscriber is the same. User always use the data directly but not porcess it according dataContentType.

So we need to add more data processing instead of modifying the java-sdk serizer when implementing subscriber.
Or directly add new methods to CloudEvent (for http subscriber) and TopicEventRequest (for gRPC subscriber) to parse data according to dataContentType.

@artursouza
Copy link
Member

Those are great insights. I think there is a way for CloudEvent to declare a custom serializer for Jackson. I think it is acceptable to have an indirect dependency on Jackson as long as people can still deserialize it without it if they decide to.

@MregXN MregXN marked this pull request as draft February 20, 2024 02:28
@cicoyle
Copy link
Contributor

cicoyle commented Jun 19, 2024

gentle ping @MregXN, any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants