Ensure any custom CL/TE header is removed when 304 response code

[violetagg]

Fixes #2818

[violetagg]

The failed test on Windows OS is not related

[pderop]

It seems that there might be some remaining use cases where a custom content length could still remain if a 304 response is returned

I am thinking about these two following use cases:

1. use case 1

For example, let's first add this test in the dispostableServer from HttpResponseStatusCodesHandlingTests.noContentStatusCodestest:

.get("/304-7", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
    .header(HttpHeaderNames.CONTENT_LENGTH, "999")
    .send())

then add a checkResponse method call in order to invoke the above "/304-7" test:

		checkResponse("/304-7", client);

so, the /304-7 test will fail because it adds a custom invalid "Content-Length: 999" header (I know it's a corner case here, because no payload is sent while an invalid content-length is provided , but this CL is not removed, because I think send() does not invoke HttpServerOperations.isContentAlwaysEmpty(); so only the TE header will be removed by HttpServerOperations.newFullBodyMessage(), but the CL header will remain. am I making sense here ?

2. use case 2:

another use case, let's use sendObject, and let's add a new /304-8 test in the dispostableServer from HttpResponseStatusCodesHandlingTests.noContentStatusCodestest:

.get("/304-8", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
    .header(HttpHeaderNames.CONTENT_LENGTH, "6")
    .sendObject(Unpooled.wrappedBuffer("/304-8".getBytes(Charset.defaultCharset()))))
    
...
checkResponse("/304-8", client);

So, here, the test fails because HttpOperations.sendObject does not invoke isContentAlwaysEmpty(), while HttpOperations.send(Publisher<? extends ByteBuf> source) does invoke isContentAlwaysEmpty().

based on the two above use cases, it sounds like two modifications could be done in the patch:

first, the patch done in HttpServerOperations.isContentAlwaysEmpty() could maybe be moved to the HttpServerOpeartions.newFullBodyMessage(ByteBuf body) method ? something like:

	@Override
	protected HttpMessage newFullBodyMessage(ByteBuf body) {
		HttpResponse res =
				new DefaultFullHttpResponse(version(), status(), body);

		if (HttpResponseStatus.NOT_MODIFIED.code() == status().code()) {
			responseHeaders.remove(HttpHeaderNames.TRANSFER_ENCODING)
					.remove(HttpHeaderNames.CONTENT_LENGTH);
		}
                ...

and finally, shouldn't the HttpOperations.sendObject be modified in order to invoke isContentAlwaysEmpty(), like it is done in HttpOperations.send(Publisher<? extends ByteBuf> source) ?
something like:

	public NettyOutbound sendObject(Object message) {
		if (!channel().isActive()) {
			ReactorNetty.safeRelease(message);
			return then(Mono.error(AbortedException.beforeSend()));
		}
		if (!(message instanceof ByteBuf)) {
			return super.sendObject(message);
		}
		ByteBuf b = (ByteBuf) message;
		return new PostHeadersNettyOutbound(FutureMono.deferFuture(() -> {
			if (markSentHeaderAndBody(b)) {
				HttpMessage msg;
				if (HttpUtil.getContentLength(outboundHttpMessage(), -1) == 0 ||
						isContentAlwaysEmpty()) {
					if (log.isDebugEnabled()) {
						log.debug(format(channel(), "Dropped HTTP content, " +
								"since response has Content-Length: 0 {}"), b);
					}
					b.release();
					msg = newFullBodyMessage(Unpooled.EMPTY_BUFFER);
				}
				else {
					msg = newFullBodyMessage(b);
				}

				try {
					afterMarkSentHeaders();
				}
				catch (RuntimeException e) {
					b.release();
					throw e;
				}

				return channel().writeAndFlush(msg);
			}
			return channel().writeAndFlush(b);
		}), this, b);
	}

doing so seems to address the two use cases.
what do you think ?

[violetagg]

@pderop I tend to agree for use case 2, can you explain more use case 1? (IMO use case 1 is an application problem)
https://www.rfc-editor.org/rfc/rfc9110#name-304-not-modified

[pderop]

The use case 1 is indeed about an application error that is adding a custom unexpected content-length header.

[violetagg]

@pderop I addressed the use case 2, PTAL

[violetagg]

The failed tests on Windows OS are not related

[pderop]

looking good ! thanks.

[violetagg]

@pderop Thanks