Skip to content

remove false comment about try-with-resources statement in StompSubProtocolHandler #35053

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

Closed
wants to merge 3 commits into from
Closed

remove false comment about try-with-resources statement in StompSubProtocolHandler #35053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b317def
use try-with-resources statement in ResourceRegionHttpMessageConverter
Jun 13, 2025
9741afe
Merge branch 'main' into fix-StompSubProtocolHandler
Jun 14, 2025
547a0dc
remove false comment about try-with-resources statement in StompSubPr…
Jun 14, 2025
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
173 changes: 173 additions & 0 deletions ...test/java/org/springframework/http/converter/ResourceRegionHttpMessageConverterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.springframework.http.converter;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.lang.reflect.Type;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand All @@ -27,6 +28,7 @@

import org.springframework.core.ParameterizedTypeReference;
import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.InputStreamResource;
import org.springframework.core.io.Resource;
import org.springframework.core.io.support.ResourceRegion;
import org.springframework.http.HttpHeaders;
Expand All @@ -36,6 +38,7 @@
import org.springframework.web.testfixture.http.MockHttpOutputMessage;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;

Expand Down Expand Up @@ -198,4 +201,174 @@ public void applicationOctetStreamDefaultContentType() throws Exception {
assertThat(outputMessage.getBodyAsString(StandardCharsets.UTF_8)).isEqualTo("Spring");
}

@Test
void shouldNotWriteForUnsupportedType() {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Object unsupportedBody = new Object();

assertThatThrownBy(() -> converter.write(unsupportedBody, null, outputMessage))
.isInstanceOfAny(ClassCastException.class, HttpMessageNotWritableException.class);
}

@Test
void shouldGetDefaultContentTypeForResourceRegion() {
Resource resource = new ClassPathResource("byterangeresource.txt", getClass());
ResourceRegion region = new ResourceRegion(resource, 0, 10);

MediaType contentType = converter.getDefaultContentType(region);
assertThat(contentType).isEqualTo(MediaType.TEXT_PLAIN);
}

@Test
void shouldGetDefaultOctetStreamContentTypeForUnknownResource() {
Resource resource = mock(Resource.class);
given(resource.getFilename()).willReturn("unknown.dat");
ResourceRegion region = new ResourceRegion(resource, 0, 10);

MediaType contentType = converter.getDefaultContentType(region);
assertThat(contentType).isEqualTo(MediaType.APPLICATION_OCTET_STREAM);
}

@Test
void shouldSupportRepeatableWritesForNonInputStreamResource() {
Resource resource = new ClassPathResource("byterangeresource.txt", getClass());
ResourceRegion region = new ResourceRegion(resource, 0, 10);

assertThat(converter.supportsRepeatableWrites(region)).isTrue();
}

@Test
void shouldNotSupportRepeatableWritesForInputStreamResource() {
Resource resource = mock(InputStreamResource.class);
ResourceRegion region = new ResourceRegion(resource, 0, 10);

assertThat(converter.supportsRepeatableWrites(region)).isFalse();
}

@Test
void shouldHandleIOExceptionWhenWritingRegion() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource = mock(Resource.class);
given(resource.contentLength()).willReturn(10L);
given(resource.getInputStream()).willThrow(new IOException("Simulated error"));
ResourceRegion region = new ResourceRegion(resource, 0, 5);

// Should not throw exception
converter.write(region, MediaType.TEXT_PLAIN, outputMessage);

// Verify Content-Range header is set correctly
assertThat(outputMessage.getHeaders().getFirst(HttpHeaders.CONTENT_RANGE))
.isEqualTo("bytes 0-4/10");

// Verify no content was written due to the IOException
assertThat(outputMessage.getBodyAsString(StandardCharsets.UTF_8)).isEmpty();
}
@Test
void shouldHandleIOExceptionWhenWritingRegionCollection() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource = mock(Resource.class);
given(resource.contentLength()).willReturn(10L);
given(resource.getInputStream()).willThrow(new IOException("Simulated error"));
ResourceRegion region = new ResourceRegion(resource, 0, 5);
List<ResourceRegion> regions = Collections.singletonList(region);

// Should not throw exception
converter.write(regions, MediaType.TEXT_PLAIN, outputMessage);

assertThat(outputMessage.getHeaders().getContentType().toString())
.isEqualTo("text/plain");
}

@Test
void shouldHandleNullResourceRegion() {
assertThatThrownBy(() -> converter.write(null, null, new MockHttpOutputMessage()))
.isInstanceOf(NullPointerException.class);
}

@Test
void shouldHandleInvalidRangeBeyondResourceLength() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource = new ClassPathResource("byterangeresource.txt", getClass());
ResourceRegion region = new ResourceRegion(resource, 35, 10); // Goes beyond resource length

converter.write(region, MediaType.TEXT_PLAIN, outputMessage);

assertThat(outputMessage.getHeaders().getFirst(HttpHeaders.CONTENT_RANGE))
.isEqualTo("bytes 35-38/39");
assertThat(outputMessage.getBodyAsString(StandardCharsets.UTF_8)).hasSize(4);
}

@Test
void shouldHandleZeroLengthResourceRegion() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource = new ClassPathResource("byterangeresource.txt", getClass());
ResourceRegion region = new ResourceRegion(resource, 5, 0);

converter.write(region, MediaType.TEXT_PLAIN, outputMessage);

assertThat(outputMessage.getHeaders().getFirst(HttpHeaders.CONTENT_RANGE))
.isEqualTo("bytes 5-4/39");
assertThat(outputMessage.getBodyAsString(StandardCharsets.UTF_8)).isEmpty();
}

@Test
void shouldHandleMultipleResourcesInCollection() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource1 = new ClassPathResource("byterangeresource.txt", getClass());
Resource resource2 = new ClassPathResource("byterangeresource.txt", getClass());
List<ResourceRegion> regions = List.of(
new ResourceRegion(resource1, 0, 5), // "Spring" is 6 bytes (0-5)
new ResourceRegion(resource2, 7, 8) // "Framework" is 8 bytes (7-14)
);

converter.write(regions, MediaType.TEXT_PLAIN, outputMessage);

String content = outputMessage.getBodyAsString(StandardCharsets.UTF_8);

// Verify multipart structure
assertThat(content).contains("Content-Type: text/plain");
assertThat(content).contains("Content-Range: bytes 7-14/39");

// Verify partial content (note the ranges only include parts of the words)
assertThat(content).contains("Sprin"); // First 5 bytes of "Spring" (0-4)
assertThat(content).contains("Framewor"); // First 7 bytes of "Framework" (7-13)
}
@Test
void shouldHandleNullContentType() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource = new ClassPathResource("byterangeresource.txt", getClass());
ResourceRegion region = new ResourceRegion(resource, 0, 5);

converter.write(region, null, outputMessage);

assertThat(outputMessage.getHeaders().getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
}

@Test
void shouldHandleUnreadableResource() throws Exception {
MockHttpOutputMessage outputMessage = new MockHttpOutputMessage();
Resource resource = mock(Resource.class);
given(resource.contentLength()).willReturn(10L);
given(resource.getInputStream()).willThrow(new IOException("Cannot read resource"));
ResourceRegion region = new ResourceRegion(resource, 0, 5);

converter.write(region, MediaType.TEXT_PLAIN, outputMessage);

assertThat(outputMessage.getHeaders().getFirst(HttpHeaders.CONTENT_RANGE))
.isEqualTo("bytes 0-4/10");
assertThat(outputMessage.getBodyAsString(StandardCharsets.UTF_8)).isEmpty();
}

@Test
void shouldHandleCanWriteWithNullType() {
assertThat(converter.canWrite(null, null, null)).isFalse();
}

@Test
void shouldHandleCanWriteWithNonParameterizedType() {
assertThat(converter.canWrite(ResourceRegion.class, null, null)).isTrue();
assertThat(converter.canWrite(String.class, null, null)).isFalse();
}


}
2 changes: 0 additions & 2 deletions ...ocket/src/main/java/org/springframework/web/socket/messaging/StompSubProtocolHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,6 @@ private void sendErrorMessage(WebSocketSession session, Throwable error) {
headerAccessor.setMessage(error.getMessage());

byte[] bytes = this.stompEncoder.encode(headerAccessor.getMessageHeaders(), EMPTY_PAYLOAD);
Copy link
Author

Choose a reason for hiding this comment

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

primitive data type is not implementing interface autocloseable, seems impossible to use try-with-resources, right?.

not possible in L:413 ether, so idk if we still need this comment, as misleading.

image

// We cannot use try-with-resources here for the WebSocketSession, since we have
// custom handling of the close() method in a finally-block.
try {
session.sendMessage(new TextMessage(bytes));
}
Expand Down