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

[restful-ws] Allow configuring default writer encoding for CloudEventsProvider #533

Open
cardil opened this issue Mar 3, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@cardil
Copy link

cardil commented Mar 3, 2023

The CloudEventsProvider defaults to binary encoding. Only when there is an StructuredEncoding annotation present, this mode will be set. See the logic:

Optional<String> structuredEncodingFormat = Arrays
.stream(annotations)
.filter(a -> a.annotationType().equals(StructuredEncoding.class))
.map(a -> ((StructuredEncoding) a).value())
.findFirst();
if (structuredEncodingFormat.isPresent()) {
writeStructured(
event,
structuredEncodingFormat.get(),
new RestfulWSMessageWriter(httpHeaders, entityStream)
);
} else {
writeBinary(
event,
new RestfulWSMessageWriter(httpHeaders, entityStream)
);
}

This makes the BinaryEncoding useless, confirmed by no usages in the code.

The default to the binary mode is unfortunate. Some implementations like Quarkus RESTEasy Reactive Server Sent Events (SSE) don't provide the annotations. This is problematic, as the binary mode doesn't make sense in SSE - the headers set by CloudEventsProvider are being dropped.

See the code:

https://github.com/quarkusio/quarkus/blob/247736226a8c8a55fa88a662eda05963803905eb/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/SseUtil.java#L141-L142

See repro: cardil/openshift-knative-showcase@15e0042
Related issue: quarkusio/quarkus#31559

The fact the Quarkus doesn't pass the method annotations, is probably a bug in Quarkus. But, I feel the CloudEvents SDK should allow setting the default mode. Then, the BinaryEncoding could have a meaning, of changing that default encoding per-method.

/kind bug

@cardil cardil changed the title There should be a way to set the default encoding, making the BinaryEncoding annotation have a purpose A way to set the default encoding, adding a purpose for the BinaryEncoding annotation Mar 3, 2023
@cardil cardil changed the title A way to set the default encoding, adding a purpose for the BinaryEncoding annotation A purpose for the BinaryEncoding annotation, by a way to set the default encoding Mar 3, 2023
@pierDipi pierDipi added the enhancement New feature or request label Mar 3, 2023
@pierDipi
Copy link
Member

pierDipi commented Mar 3, 2023

With @JemDay we were discussing configurations like this in the context of XML format writer but there is no concrete design or proposal yet.

@pierDipi pierDipi changed the title A purpose for the BinaryEncoding annotation, by a way to set the default encoding [restful-ws] Allow configuring default writer encoding for CloudEventsProvider Mar 3, 2023
@cardil
Copy link
Author

cardil commented Mar 3, 2023

How about we provide settings like interface, which people could define using Java's ServiceLoader. It may be something like:

interface Settings {
  default Encoding defaultEncoding() {
    return Encoding.BINARY;
  }
}

@cardil
Copy link
Author

cardil commented Mar 3, 2023

I created a Quarkus issue about this: quarkusio/quarkus#31587

cardil added a commit to cardil/openshift-knative-showcase that referenced this issue Mar 3, 2023
cardil added a commit to cardil/openshift-knative-showcase that referenced this issue Mar 3, 2023
cardil added a commit to cardil/openshift-knative-showcase that referenced this issue Mar 7, 2023
cardil added a commit to cardil/openshift-knative-showcase that referenced this issue Mar 7, 2023
cardil added a commit to cardil/openshift-knative-showcase that referenced this issue Mar 10, 2023
cardil added a commit to cardil/openshift-knative-showcase that referenced this issue Mar 10, 2023
openshift-merge-robot pushed a commit to openshift-knative/showcase that referenced this issue Apr 13, 2023
* Events endpoint PoC

* Workaround for quarkusio/quarkus#31587 and cloudevents/sdk-java#533

* Better workaround, thx @pierDipi

for quarkusio/quarkus#31587 and cloudevents/sdk-java#533

* Development data

* Proper streaming of events SSE

* Frontend in React PoC

* Event stream using u-cloudevents

* Quarkus backend uses the React frontend

* Express app uses the React frontend

* Tests for Express's event receiver

* GH Actions Testing

* Container build fix

* Remove pre-build Containerfiles

* Self-review fixes

* Tests for Quarkus event receiver

* README format and npm install instructions
@cardil
Copy link
Author

cardil commented Jul 25, 2023

As evidenced in cardil/openshift-knative-showcase#2 the fixing of quarkusio/quarkus#31587 (PR quarkusio/quarkus#32815) didn't help completely.

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

No branches or pull requests

2 participants