Skip to content

4875-binary-access-write-doest-trigger-STORAGE-BINARY-ASSIGN-BLOB-ID-PREFIX-pointcut #4876

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

Merged
merged 10 commits into from
May 15, 2023
Merged

4875-binary-access-write-doest-trigger-STORAGE-BINARY-ASSIGN-BLOB-ID-PREFIX-pointcut #4876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7fae598
Add failing test
May 12, 2023
5b27e5f
Add failing test
May 12, 2023
73b9824
Fix and changelog
May 12, 2023
674d43a
Pass content type parameter
May 12, 2023
6d9481c
Back to auto wiring the context
May 12, 2023
abf1bcc
Invoke interceptor only when getting blobId, not also when storing it
May 14, 2023
cff8bad
Avoid breaking implementers
May 15, 2023
c34421f
Address review comment
May 15, 2023
b35dd11
Add new exception Msg code
May 15, 2023
e5d798f
Fix broken test
May 15, 2023
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
5 changes: 5 additions & 0 deletions ...875-binary-access-write-doesnt-trigger-storage-binary-assign-blob-id-prefix-pointcut.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
type: fix
issue: 4875
title: "Previously, `$binary-access-write` operation didn't trigger `STORAGE_BINARY_ASSIGN_BLOB_ID_PREFIX` Pointcut.
This has been fixed."
15 changes: 9 additions & 6 deletions ...aserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import ca.uhn.fhir.jpa.binary.svc.BaseBinaryStorageSvcImpl;
import ca.uhn.fhir.jpa.dao.data.IBinaryStorageEntityDao;
import ca.uhn.fhir.jpa.model.entity.BinaryStorageEntity;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
import com.google.common.hash.HashingInputStream;
import com.google.common.io.ByteStreams;
Expand All @@ -36,6 +37,7 @@
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;

import javax.annotation.Nonnull;
import javax.persistence.EntityManager;
import javax.persistence.PersistenceContext;
import javax.persistence.PersistenceContextType;
Expand All @@ -55,9 +57,11 @@ public class DatabaseBlobBinaryStorageSvcImpl extends BaseBinaryStorageSvcImpl {
@Autowired
private IBinaryStorageEntityDao myBinaryStorageEntityDao;

@Nonnull
@Override
@Transactional(propagation = Propagation.REQUIRED)
public StoredDetails storeBlob(IIdType theResourceId, String theBlobIdOrNull, String theContentType, InputStream theInputStream) throws IOException {
public StoredDetails storeBlob(IIdType theResourceId, String theBlobIdOrNull, String theContentType,
InputStream theInputStream, RequestDetails theRequestDetails) throws IOException {

/*
* Note on transactionality: This method used to have a propagation value of SUPPORTS and then do the actual
Expand All @@ -70,17 +74,16 @@ public StoredDetails storeBlob(IIdType theResourceId, String theBlobIdOrNull, St
HashingInputStream hashingInputStream = createHashingInputStream(theInputStream);
CountingInputStream countingInputStream = createCountingInputStream(hashingInputStream);

String id = super.provideIdForNewBlob(theBlobIdOrNull);

BinaryStorageEntity entity = new BinaryStorageEntity();
entity.setResourceId(theResourceId.toUnqualifiedVersionless().getValue());
entity.setBlobId(id);
entity.setBlobContentType(theContentType);
entity.setPublished(publishedDate);

Session session = (Session) myEntityManager.getDelegate();
LobHelper lobHelper = session.getLobHelper();
byte[] loadedStream = IOUtils.toByteArray(countingInputStream);
String id = super.provideIdForNewBlob(theBlobIdOrNull, loadedStream, theRequestDetails, theContentType);
entity.setBlobId(id);
Blob dataBlob = lobHelper.createBlob(loadedStream);
entity.setBlob(dataBlob);

Expand All @@ -105,7 +108,7 @@ public StoredDetails storeBlob(IIdType theResourceId, String theBlobIdOrNull, St
public StoredDetails fetchBlobDetails(IIdType theResourceId, String theBlobId) {

Optional<BinaryStorageEntity> entityOpt = myBinaryStorageEntityDao.findByIdAndResourceId(theBlobId, theResourceId.toUnqualifiedVersionless().getValue());
if (entityOpt.isPresent() == false) {
if (entityOpt.isEmpty()) {
return null;
}

Expand All @@ -121,7 +124,7 @@ public StoredDetails fetchBlobDetails(IIdType theResourceId, String theBlobId) {
@Override
public boolean writeBlob(IIdType theResourceId, String theBlobId, OutputStream theOutputStream) throws IOException {
Optional<BinaryStorageEntity> entityOpt = myBinaryStorageEntityDao.findByIdAndResourceId(theBlobId, theResourceId.toUnqualifiedVersionless().getValue());
if (entityOpt.isPresent() == false) {
if (entityOpt.isEmpty()) {
return false;
}

Expand Down
11 changes: 5 additions & 6 deletions ...ir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import ca.uhn.fhir.jpa.binary.api.IBinaryStorageSvc;
import ca.uhn.fhir.jpa.binary.api.StoredDetails;
import ca.uhn.fhir.jpa.binary.provider.BinaryAccessProvider;
import ca.uhn.fhir.mdm.util.MessageHelper;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.server.RestfulServer;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
Expand Down Expand Up @@ -39,6 +39,7 @@
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isNull;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -66,8 +67,6 @@ public class BinaryAccessProviderTest {
@Spy
protected IBinaryStorageSvc myBinaryStorageSvc;
@Autowired
private MessageHelper myMessageHelper;
@Autowired
private IInterceptorBroadcaster myInterceptorBroadcaster;


Expand Down Expand Up @@ -157,7 +156,7 @@ public void testBinaryAccessRead_WithoutAttachmentId() throws IOException {
}

@Test
public void testBinaryAccessRead_WithoutAttachmentId_NullData() throws IOException {
public void testBinaryAccessRead_WithoutAttachmentId_NullData() {
DocumentReference docRef = new DocumentReference();
DocumentReference.DocumentReferenceContentComponent content = docRef.addContent();
content.getAttachment().setContentType("application/octet-stream");
Expand Down Expand Up @@ -257,7 +256,7 @@ public void testBinaryAccessWrite_WithContentLength15() throws IOException {
when(theServletRequest.getContentLength()).thenReturn(15);
when(myBinaryStorageSvc.shouldStoreBlob(15, docRef.getIdElement(), "Integer")).thenReturn(true);
myRequestDetails.setServletRequest(theServletRequest);
when(myBinaryStorageSvc.storeBlob(eq(docRef.getIdElement()), isNull(), eq("Integer"), any(InputStream.class))).thenReturn(sd);
doReturn(sd).when(myBinaryStorageSvc).storeBlob(eq(docRef.getIdElement()), isNull(), eq("Integer"), any(InputStream.class), any(RequestDetails.class));
myRequestDetails.setRequestContents(SOME_BYTES);

try {
Expand All @@ -266,7 +265,7 @@ public void testBinaryAccessWrite_WithContentLength15() throws IOException {
assertEquals(docRef.getId(), outcome.getIdElement().getValue());
} catch (IOException e) {
}
verify(myBinaryStorageSvc, times(1)).storeBlob(any(), any(), any(), any());
verify(myBinaryStorageSvc, times(1)).storeBlob(any(), any(), any(), any(), any(ServletRequestDetails.class));
}

@Test
Expand Down
9 changes: 5 additions & 4 deletions ...-test-r4/src/test/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import ca.uhn.fhir.jpa.model.entity.BinaryStorageEntity;
import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
import org.hl7.fhir.r4.model.IdType;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -52,7 +53,7 @@ public void testStoreAndRetrieve() throws IOException {
ByteArrayInputStream inputStream = new ByteArrayInputStream(SOME_BYTES);
String contentType = "image/png";
IdType resourceId = new IdType("Binary/123");
StoredDetails outcome = mySvc.storeBlob(resourceId, null, contentType, inputStream);
StoredDetails outcome = mySvc.storeBlob(resourceId, null, contentType, inputStream, new ServletRequestDetails());

myCaptureQueriesListener.logAllQueriesForCurrentThread();

Expand Down Expand Up @@ -105,7 +106,7 @@ public void testStoreAndRetrieveWithManualId() throws IOException {
ByteArrayInputStream inputStream = new ByteArrayInputStream(SOME_BYTES);
String contentType = "image/png";
IdType resourceId = new IdType("Binary/123");
StoredDetails outcome = mySvc.storeBlob(resourceId, "ABCDEFG", contentType, inputStream);
StoredDetails outcome = mySvc.storeBlob(resourceId, "ABCDEFG", contentType, inputStream, new ServletRequestDetails());
assertEquals("ABCDEFG", outcome.getBlobId());

myCaptureQueriesListener.logAllQueriesForCurrentThread();
Expand Down Expand Up @@ -163,7 +164,7 @@ public void testExpunge() throws IOException {
ByteArrayInputStream inputStream = new ByteArrayInputStream(SOME_BYTES);
String contentType = "image/png";
IdType resourceId = new IdType("Binary/123");
StoredDetails outcome = mySvc.storeBlob(resourceId, null, contentType, inputStream);
StoredDetails outcome = mySvc.storeBlob(resourceId, null, contentType, inputStream, new ServletRequestDetails());
String blobId = outcome.getBlobId();

// Expunge
Expand All @@ -185,7 +186,7 @@ public void testWrongResourceId() throws IOException {
ByteArrayInputStream inputStream = new ByteArrayInputStream(SOME_BYTES);
String contentType = "image/png";
IdType resourceId = new IdType("Binary/123");
StoredDetails outcome = mySvc.storeBlob(resourceId, null, contentType, inputStream);
StoredDetails outcome = mySvc.storeBlob(resourceId, null, contentType, inputStream, new ServletRequestDetails());

// Right ID
ByteArrayOutputStream capture = new ByteArrayOutputStream();
Expand Down
13 changes: 9 additions & 4 deletions ...er-test-r4/src/test/java/ca/uhn/fhir/jpa/binstore/FilesystemBinaryStorageSvcImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package ca.uhn.fhir.jpa.binstore;

import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.interceptor.executor.InterceptorService;
import ca.uhn.fhir.jpa.binary.api.StoredDetails;
import ca.uhn.fhir.rest.server.exceptions.PayloadTooLargeException;
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
import org.apache.commons.io.FileUtils;
import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.r4.model.IdType;
Expand Down Expand Up @@ -34,6 +37,8 @@ public class FilesystemBinaryStorageSvcImplTest {
public void before() {
myPath = new File("./target/fstmp");
mySvc = new FilesystemBinaryStorageSvcImpl(myPath.getAbsolutePath());
mySvc.setFhirContextForTests(FhirContext.forR4Cached());
mySvc.setInterceptorBroadcasterForTests(new InterceptorService());
}

@AfterEach
Expand All @@ -45,7 +50,7 @@ public void after() throws IOException {
public void testStoreAndRetrieve() throws IOException {
IIdType id = new IdType("Patient/123");
String contentType = "image/png";
StoredDetails outcome = mySvc.storeBlob(id, null, contentType, new ByteArrayInputStream(SOME_BYTES));
StoredDetails outcome = mySvc.storeBlob(id, null, contentType, new ByteArrayInputStream(SOME_BYTES), new ServletRequestDetails());

ourLog.info("Got id: {}", outcome);

Expand All @@ -68,7 +73,7 @@ public void testStoreAndRetrieveManualId() throws IOException {
IIdType id = new IdType("Patient/123");
String contentType = "image/png";
String blobId = "ABCDEFGHIJKLMNOPQRSTUV";
StoredDetails outcome = mySvc.storeBlob(id, blobId, contentType, new ByteArrayInputStream(SOME_BYTES));
StoredDetails outcome = mySvc.storeBlob(id, blobId, contentType, new ByteArrayInputStream(SOME_BYTES), new ServletRequestDetails());
assertEquals(blobId, outcome.getBlobId());

ourLog.info("Got id: {}", outcome);
Expand Down Expand Up @@ -103,7 +108,7 @@ public void testFetchBlobUnknown() throws IOException {
public void testExpunge() throws IOException {
IIdType id = new IdType("Patient/123");
String contentType = "image/png";
StoredDetails outcome = mySvc.storeBlob(id, null, contentType, new ByteArrayInputStream(SOME_BYTES));
StoredDetails outcome = mySvc.storeBlob(id, null, contentType, new ByteArrayInputStream(SOME_BYTES), new ServletRequestDetails());

ourLog.info("Got id: {}", outcome);

Expand All @@ -129,7 +134,7 @@ public void testRejectOversized() throws IOException {
IIdType id = new IdType("Patient/123");
String contentType = "image/png";
try {
mySvc.storeBlob(id, null, contentType, new ByteArrayInputStream(SOME_BYTES));
mySvc.storeBlob(id, null, contentType, new ByteArrayInputStream(SOME_BYTES), new ServletRequestDetails());
fail();
} catch (PayloadTooLargeException e) {
assertEquals(Msg.code(1343) + "Binary size exceeds maximum: 5", e.getMessage());
Expand Down
24 changes: 6 additions & 18 deletions ...paserver-test-r4/src/test/java/ca/uhn/fhir/jpa/binstore/NullBinaryStorageSvcImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,43 +18,31 @@ public void shouldStoreBlob() {

@Test
public void storeBlob() {
assertThrows(UnsupportedOperationException.class, () -> {
mySvc.storeBlob(null, null, null, null);
});
assertThrows(UnsupportedOperationException.class, () -> mySvc.storeBlob(null, null, null, null, null));
}

@Test
public void fetchBlobDetails() {
assertThrows(UnsupportedOperationException.class, () -> {
mySvc.fetchBlobDetails(null, null);
});
assertThrows(UnsupportedOperationException.class, () -> mySvc.fetchBlobDetails(null, null));
}

@Test
public void writeBlob() {
assertThrows(UnsupportedOperationException.class, () -> {
mySvc.writeBlob(null, null, null);
});
assertThrows(UnsupportedOperationException.class, () -> mySvc.writeBlob(null, null, null));
}

@Test
public void expungeBlob() {
assertThrows(UnsupportedOperationException.class, () -> {
mySvc.expungeBlob(null, null);
});
assertThrows(UnsupportedOperationException.class, () -> mySvc.expungeBlob(null, null));
}

@Test
public void fetchBlob() {
assertThrows(UnsupportedOperationException.class, () -> {
mySvc.fetchBlob(null, null);
});
assertThrows(UnsupportedOperationException.class, () -> mySvc.fetchBlob(null, null));
}

@Test
public void newBlobId() {
assertThrows(UnsupportedOperationException.class, () -> {
mySvc.newBlobId();
});
assertThrows(UnsupportedOperationException.class, () -> mySvc.newBlobId(null, null, null));
}
}
Loading