From 7a3a28bec02c8339255f02c02ef3c7211c45fa2e Mon Sep 17 00:00:00 2001 From: Paul Millar Date: Fri, 26 Jan 2024 23:05:49 +0100 Subject: [PATCH] Include Content-Length response header Motivation: The 'Content-Length' response header is expected by many clients. While it is not possible to specify the content length for on-the-fly compression, it is possible when sending an individual file. Modification: Update DataSelection to optionally return the content length. Update the 'getData' Response to include the `Content-Length` header when the request targets a single file that is not placed in a zip archive. If the content is dynamically generated (e.g., a zip file) then no `Content-Length` header is provided. Result: IDS now includes the `Content-Length` response header when this is possible Signed-off-by: Paul Millar --- pom.xml | 6 ++ .../org/icatproject/ids/DataSelection.java | 9 +++ .../java/org/icatproject/ids/IdsBean.java | 12 +++- .../icatproject/ids/integration/BaseTest.java | 12 ++++ .../integration/one/GetDataExplicitTest.java | 55 ++++++++++++++++--- .../util/client/TestingClient.java | 14 ++++- 6 files changed, 94 insertions(+), 14 deletions(-) diff --git a/pom.xml b/pom.xml index 0fa6f72c..413e33ca 100644 --- a/pom.xml +++ b/pom.xml @@ -129,6 +129,12 @@ test + + org.hamcrest + hamcrest + 2.2 + test + diff --git a/src/main/java/org/icatproject/ids/DataSelection.java b/src/main/java/org/icatproject/ids/DataSelection.java index c0ed8abc..0a901edd 100644 --- a/src/main/java/org/icatproject/ids/DataSelection.java +++ b/src/main/java/org/icatproject/ids/DataSelection.java @@ -7,6 +7,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.OptionalLong; import java.util.Set; import jakarta.json.Json; @@ -53,6 +54,7 @@ public class DataSelection { private Set emptyDatasets; private boolean dsWanted; private boolean dfWanted; + private long length; public enum Returns { @@ -135,6 +137,7 @@ private void resolveDatasetIds() dsInfos.put(dsid, new DsInfoImpl(ds)); if (dfWanted) { Datafile df = (Datafile) icat.get(userSessionId, "Datafile", dfid); + length += df.getFileSize(); String location = IdsBean.getLocation(dfid, df.getLocation()); dfInfos.add( new DfInfoImpl(dfid, df.getName(), location, df.getCreateId(), df.getModId(), dsid)); @@ -315,4 +318,10 @@ public Set getEmptyDatasets() { return emptyDatasets; } + public OptionalLong getFileLength() { + if (!dfWanted || mustZip()) { + return OptionalLong.empty(); + } + return OptionalLong.of(length); + } } diff --git a/src/main/java/org/icatproject/ids/IdsBean.java b/src/main/java/org/icatproject/ids/IdsBean.java index e5c371a4..59b49fa7 100644 --- a/src/main/java/org/icatproject/ids/IdsBean.java +++ b/src/main/java/org/icatproject/ids/IdsBean.java @@ -20,6 +20,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.OptionalLong; import java.util.Set; import java.util.SortedMap; import java.util.SortedSet; @@ -50,6 +51,7 @@ import jakarta.json.JsonReader; import jakarta.json.JsonValue; import jakarta.json.stream.JsonGenerator; +import static jakarta.ws.rs.core.HttpHeaders.CONTENT_LENGTH; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.StreamingOutput; @@ -859,6 +861,7 @@ public Response getData(String sessionId, String investigationIds, String datase // Do it Map dsInfos = dataSelection.getDsInfo(); Set dfInfos = dataSelection.getDfInfo(); + var length = zip ? OptionalLong.empty() : dataSelection.getFileLength(); Lock lock = null; try { @@ -909,11 +912,14 @@ public Response getData(String sessionId, String investigationIds, String datase } } - return Response.status(offset == 0 ? HttpURLConnection.HTTP_OK : HttpURLConnection.HTTP_PARTIAL) + var response = Response.status(offset == 0 ? HttpURLConnection.HTTP_OK : HttpURLConnection.HTTP_PARTIAL) .entity(new SO(dataSelection.getDsInfo(), dataSelection.getDfInfo(), offset, finalZip, compress, lock, transferId, ip, start)) - .header("Content-Disposition", "attachment; filename=\"" + name + "\"").header("Accept-Ranges", "bytes") - .build(); + .header("Content-Disposition", "attachment; filename=\"" + name + "\"").header("Accept-Ranges", "bytes"); + length.stream() + .map(l -> Math.max(0L, l - offset)) + .forEach(l -> response.header(CONTENT_LENGTH, l)); + return response.build(); } catch (AlreadyLockedException e) { logger.debug("Could not acquire lock, getData failed"); throw new DataNotOnlineException("Data is busy"); diff --git a/src/test/java/org/icatproject/ids/integration/BaseTest.java b/src/test/java/org/icatproject/ids/integration/BaseTest.java index 2d4bca96..fbfdb64a 100644 --- a/src/test/java/org/icatproject/ids/integration/BaseTest.java +++ b/src/test/java/org/icatproject/ids/integration/BaseTest.java @@ -6,6 +6,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; @@ -20,6 +21,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.NoSuchElementException; import java.util.Set; import java.util.UUID; import java.util.zip.CRC32; @@ -369,6 +371,16 @@ protected void checkStream(InputStream stream, long id) throws IOException { assertTrue(found); } + protected long fileLength(long id) { + for (Entry e : ids.entrySet()) { + if (id == e.getValue()) { + var fileContents = contents.get(e.getKey()); + return fileContents.getBytes(StandardCharsets.UTF_8).length; + } + } + throw new NoSuchElementException("No file with id " + id); + } + protected void writeToFile(Datafile df, String content, String key) throws IOException, IcatException_Exception, NoSuchAlgorithmException { Path path = setup.getStorageDir().resolve(df.getLocation()); diff --git a/src/test/java/org/icatproject/ids/integration/one/GetDataExplicitTest.java b/src/test/java/org/icatproject/ids/integration/one/GetDataExplicitTest.java index 16026ef5..0e34d5cd 100644 --- a/src/test/java/org/icatproject/ids/integration/one/GetDataExplicitTest.java +++ b/src/test/java/org/icatproject/ids/integration/one/GetDataExplicitTest.java @@ -2,6 +2,8 @@ import java.io.InputStream; import java.util.Arrays; +import static org.hamcrest.Matchers.*; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import org.junit.BeforeClass; @@ -99,12 +101,23 @@ public void forbiddenTest() throws Exception { @Test public void correctBehaviourTestNone() throws Exception { try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafiles(datafileIds), - Flag.NONE, 0, 200)) { + Flag.NONE, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkZipStream(stream, datafileIds, 57L, 0); } - try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafile(datafileIds.get(0)), - Flag.NONE, 0, 200)) { + var datafileId = datafileIds.get(0); + var fileLength = fileLength(datafileId); + try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafile(datafileId), + Flag.NONE, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(not(nullValue()))); + var contentLength = r.getFirstHeader("Content-Length").getValue(); + assertThat(Long.valueOf(contentLength), is(equalTo(fileLength))); + assertThat(r.getFirstHeader("Transfer-Encoding"), is(nullValue())); + })) { checkStream(stream, datafileIds.get(0)); } } @@ -112,12 +125,20 @@ public void correctBehaviourTestNone() throws Exception { @Test public void correctBehaviourTestCompress() throws Exception { try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafiles(datafileIds), - Flag.COMPRESS, 0, 200)) { + Flag.COMPRESS, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkZipStream(stream, datafileIds, 36L, 0); } try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafile(datafileIds.get(0)), - Flag.COMPRESS, 0, 200)) { + Flag.COMPRESS, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkStream(stream, datafileIds.get(0)); } } @@ -125,12 +146,20 @@ public void correctBehaviourTestCompress() throws Exception { @Test public void correctBehaviourTestZip() throws Exception { try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafiles(datafileIds), - Flag.ZIP, 0, 200)) { + Flag.ZIP, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkZipStream(stream, datafileIds, 57L, 0); } try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafile(datafileIds.get(0)), - Flag.ZIP, 0, 200)) { + Flag.ZIP, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkZipStream(stream, datafileIds.subList(0, 1), 57L, 0); } } @@ -138,12 +167,20 @@ public void correctBehaviourTestZip() throws Exception { @Test public void correctBehaviourTestZipAndCompress() throws Exception { try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafiles(datafileIds), - Flag.ZIP_AND_COMPRESS, 0, 200)) { + Flag.ZIP_AND_COMPRESS, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkZipStream(stream, datafileIds, 36L, 0); } try (InputStream stream = testingClient.getData(sessionId, new DataSelection().addDatafile(datafileIds.get(0)), - Flag.ZIP_AND_COMPRESS, 0, 200)) { + Flag.ZIP_AND_COMPRESS, 0, 200, r -> { + assertThat(r.getFirstHeader("Content-Length"), is(nullValue())); + assertThat(r.getFirstHeader("Transfer-Encoding").getValue(), + is(equalToIgnoringCase("chunked"))); + })) { checkZipStream(stream, datafileIds.subList(0, 1), 36L, 0); } } diff --git a/src/test/java/org/icatproject/ids/integration/util/client/TestingClient.java b/src/test/java/org/icatproject/ids/integration/util/client/TestingClient.java index 2f826866..a4bde1a0 100644 --- a/src/test/java/org/icatproject/ids/integration/util/client/TestingClient.java +++ b/src/test/java/org/icatproject/ids/integration/util/client/TestingClient.java @@ -26,6 +26,7 @@ import jakarta.json.JsonObject; import jakarta.json.JsonReader; import jakarta.json.JsonValue; +import java.util.function.Consumer; import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; @@ -266,6 +267,12 @@ public URL getIcatUrl(int sc) throws InternalException, ParseException, NotImple public InputStream getData(String sessionId, DataSelection data, Flag flags, long offset, Integer sc) throws NotImplementedException, BadRequestException, InsufficientPrivilegesException, NotFoundException, InternalException, DataNotOnlineException { + return getData(sessionId, data, flags, offset, sc, null); + } + + public InputStream getData(String sessionId, DataSelection data, Flag flags, long offset, Integer sc, Consumer responseAssertion) + throws NotImplementedException, BadRequestException, InsufficientPrivilegesException, NotFoundException, + InternalException, DataNotOnlineException { URIBuilder uriBuilder = getUriBuilder("getData"); uriBuilder.setParameter("sessionId", sessionId); @@ -293,6 +300,9 @@ public InputStream getData(String sessionId, DataSelection data, Flag flags, lon response = httpclient.execute(httpGet); checkStatus(response, sc); checkResponseConformity(response); + if (responseAssertion != null) { + responseAssertion.accept(response); + } closeNeeded = false; return new HttpInputStream(httpclient, response); } catch (IOException | InsufficientStorageException e) { @@ -928,14 +938,14 @@ public void write(String sessionId, DataSelection data, Integer sc) throws NotIm } private void checkResponseConformity(HttpResponse response) throws InternalException { - + StatusLine status = response.getStatusLine(); var statusCode = status.getStatusCode(); if(statusCode == 200 && response.getEntity() != null) { //we have a status of 200 and a payload. Check for header conformity Boolean chunked = response.getFirstHeader("Transfer-Encoding") != null ? response.getFirstHeader("Transfer-Encoding").getValue().contains("chunked") : false; - + if ( (response.getFirstHeader("Content-Length") == null && !chunked) || (response.getFirstHeader("Content-Length") != null && chunked ) ) {