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.

Sign up for GitHub

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

Jump to bottom

Prevent SSE writing from potentially causing accumulation of headers #31560

Merged
merged 1 commit into from
Mar 3, 2023
Merged

Prevent SSE writing from potentially causing accumulation of headers #31560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .../java/org/jboss/resteasy/reactive/client/handlers/ClientSetResponseEntityRestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import jakarta.ws.rs.client.Entity;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.MultivaluedMap;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.Response.StatusType;

Expand All @@ -17,6 +16,7 @@
import org.jboss.resteasy.reactive.client.spi.ClientRestHandler;
import org.jboss.resteasy.reactive.common.core.Serialisers;
import org.jboss.resteasy.reactive.common.jaxrs.StatusTypeImpl;
import org.jboss.resteasy.reactive.common.util.QuarkusMultivaluedHashMap;

import io.vertx.core.buffer.Buffer;

Expand Down Expand Up @@ -88,7 +88,7 @@ private ByteArrayInputStream entityStreamOfAbortedResponseOf(RestClientRequestCo
entity = Entity.entity(untypedEntity, mediaType);
}
// FIXME: pass headers?
Buffer buffer = context.writeEntity(entity, (MultivaluedMap) Serialisers.EMPTY_MULTI_MAP,
Buffer buffer = context.writeEntity(entity, new QuarkusMultivaluedHashMap<>(),
Serialisers.NO_WRITER_INTERCEPTOR);
return new ByteArrayInputStream(buffer.getBytes());
}
Expand Down
3 changes: 2 additions & 1 deletion ...nt/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/InboundSseEventImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import org.jboss.resteasy.reactive.common.core.Serialisers;
import org.jboss.resteasy.reactive.common.jaxrs.ConfigurationImpl;
import org.jboss.resteasy.reactive.common.util.QuarkusMultivaluedHashMap;

public class InboundSseEventImpl implements InboundSseEvent {

Expand Down Expand Up @@ -120,7 +121,7 @@ public <T> T readData(GenericType<T> type, MediaType mediaType) {
InputStream in = new ByteArrayInputStream(data.getBytes(StandardCharsets.UTF_8));
try {
return (T) ClientSerialisers.invokeClientReader(null, type.getRawType(), type.getType(),
mediaType, null, null, Serialisers.EMPTY_MULTI_MAP,
mediaType, null, null, new QuarkusMultivaluedHashMap<>(),
serialisers, in, Serialisers.NO_READER_INTERCEPTOR, configuration);
} catch (IOException e) {
throw new UncheckedIOException(e);
Expand Down
2 changes: 0 additions & 2 deletions ...ive/common/runtime/src/main/java/org/jboss/resteasy/reactive/common/core/Serialisers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

import jakarta.ws.rs.RuntimeType;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.MultivaluedMap;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.ext.MessageBodyReader;
import jakarta.ws.rs.ext.MessageBodyWriter;
Expand All @@ -32,7 +31,6 @@
public abstract class Serialisers {
public static final Annotation[] NO_ANNOTATION = new Annotation[0];
public static final ReaderInterceptor[] NO_READER_INTERCEPTOR = new ReaderInterceptor[0];
public static final MultivaluedMap<String, Object> EMPTY_MULTI_MAP = new QuarkusMultivaluedHashMap<>();
public static final WriterInterceptor[] NO_WRITER_INTERCEPTOR = new WriterInterceptor[0];
protected static final Map<Class<?>, Class<?>> primitivesToWrappers = new HashMap<>();
// FIXME: spec says we should use generic type, but not sure how to pass that type from Jandex to reflection
Expand Down
3 changes: 2 additions & 1 deletion ...eactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/SseUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.jboss.resteasy.reactive.common.core.Serialisers;
import org.jboss.resteasy.reactive.common.util.CommonSseUtil;
import org.jboss.resteasy.reactive.common.util.QuarkusMultivaluedHashMap;
import org.jboss.resteasy.reactive.server.handlers.PublisherResponseHandler;
import org.jboss.resteasy.reactive.server.jaxrs.OutboundSseEventImpl;
import org.jboss.resteasy.reactive.server.spi.ServerHttpResponse;
Expand Down Expand Up @@ -138,7 +139,7 @@ private static String serialiseDataToString(ResteasyReactiveRequestContext conte
if (writer.isWriteable(entityClass, entityType, Serialisers.NO_ANNOTATION, mediaType)) {
// FIXME: spec doesn't really say what headers we should use here
writer.writeTo(entity, entityClass, entityType, Serialisers.NO_ANNOTATION, mediaType,
Serialisers.EMPTY_MULTI_MAP, baos);
new QuarkusMultivaluedHashMap<>(), baos);
Comment on lines -141 to +142
Copy link

Choose a reason for hiding this comment

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

Instead of passing empty map as headers, we should pass a copy of currently set headers.

In that way, an implementation of MessageBodyWriter could detect it should serialize the object for Server Sent Events, so it makes no sense to set headers.

Also, it makes sense to warn people that those headers will be dropped. For example, to check if the headers been changed after calling write, we can log a WARN message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you can provide a test showing why this is important, we can certainly reconsider

Copy link

Choose a reason for hiding this comment

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

See #31587 and cloudevents/sdk-java#533 for more context

wrote = true;
break;
}
Expand Down
3 changes: 2 additions & 1 deletion ...e/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/StreamingUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import jakarta.ws.rs.ext.MessageBodyWriter;

import org.jboss.resteasy.reactive.common.core.Serialisers;
import org.jboss.resteasy.reactive.common.util.QuarkusMultivaluedHashMap;
import org.jboss.resteasy.reactive.server.StreamingOutputStream;
import org.jboss.resteasy.reactive.server.handlers.PublisherResponseHandler;
import org.jboss.resteasy.reactive.server.spi.ServerHttpResponse;
Expand Down Expand Up @@ -66,7 +67,7 @@ private static byte[] serialiseEntity(ResteasyReactiveRequestContext context, Ob
if (writer.isWriteable(entityClass, entityType, Serialisers.NO_ANNOTATION, mediaType)) {
// FIXME: spec doesn't really say what headers we should use here
writer.writeTo(entity, entityClass, entityType, Serialisers.NO_ANNOTATION, mediaType,
Serialisers.EMPTY_MULTI_MAP, baos);
new QuarkusMultivaluedHashMap<>(), baos);
wrote = true;
break;
}
Expand Down
3 changes: 2 additions & 1 deletion ...in/java/org/jboss/resteasy/reactive/server/core/multipart/MultipartMessageBodyWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import org.jboss.resteasy.reactive.common.core.Serialisers;
import org.jboss.resteasy.reactive.common.reflection.ReflectionBeanFactoryCreator;
import org.jboss.resteasy.reactive.common.util.QuarkusMultivaluedHashMap;
import org.jboss.resteasy.reactive.multipart.FileDownload;
import org.jboss.resteasy.reactive.server.NoopCloseAndFlushOutputStream;
import org.jboss.resteasy.reactive.server.core.CurrentRequestManager;
Expand Down Expand Up @@ -192,7 +193,7 @@ private void writeEntity(OutputStream os, Object entity, MediaType mediaType, Re
try (NoopCloseAndFlushOutputStream writerOutput = new NoopCloseAndFlushOutputStream(os)) {
// FIXME: spec doesn't really say what headers we should use here
writer.writeTo(entity, entityClass, entityType, Serialisers.NO_ANNOTATION, mediaType,
Serialisers.EMPTY_MULTI_MAP, writerOutput);
new QuarkusMultivaluedHashMap<>(), writerOutput);
wrote = true;
}

Expand Down