fix: always evaluate if-match headers when setting content, not just … (

#1797) * fix: always evaluate if-match headers when setting content, not just when content exists * test: add test coverage for issue 1795
paulcwarren · Feb 7, 2024 · 23782f0 · 23782f0
1 parent bdeb045
commit 23782f0
Show file tree

Hide file tree

Showing 8 changed files with 65 additions and 17 deletions.
diff --git a/.../main/java/internal/org/springframework/content/rest/controllers/StoreRestController.java b/.../main/java/internal/org/springframework/content/rest/controllers/StoreRestController.java
@@ -191,9 +191,8 @@ protected void handleMultipart(HttpServletRequest request, HttpServletResponse r
 
         boolean isNew = false;
 
-        if (target.exists()) {
-            HeaderUtils.evaluateHeaderConditions(headers, targetETag != null ? targetETag.toString() : null, new Date(resolveLastModified(target)));
-        } else {
+        HeaderUtils.evaluateHeaderConditions(headers, targetETag != null ? targetETag.toString() : null, new Date(resolveLastModified(target)));
+        if (target.exists() == false) {
             isNew = true;
         }
 

diff --git a/...c/main/java/internal/org/springframework/content/rest/io/AssociatedStoreResourceImpl.java b/...c/main/java/internal/org/springframework/content/rest/io/AssociatedStoreResourceImpl.java
@@ -175,15 +175,19 @@ public long lastModified() throws IOException {
 
      // TODO: can we remove this is all properties are effectively embedded?
         Object lastModified = null; //BeanUtils.getFieldWithAnnotation(property, LastModifiedDate.class);
-        if (lastModified == null) {
+        if (lastModified == null && getDelegate() != null) {
             return getDelegate().lastModified();
         }
 
-        return Stream.of(lastModified)
-        .map(it -> getConversionService().convert(it, Date.class))//
-        .map(it -> getConversionService().convert(it, Instant.class))//
-        .map(it -> it.toEpochMilli())
-        .findFirst().orElseThrow(() -> new IllegalArgumentException(format("Invalid data type for @LastModifiedDate on Entity %s", this.getAssociation())));
+        if (lastModified != null) {
+            return Stream.of(lastModified)
+                    .map(it -> getConversionService().convert(it, Date.class))//
+                    .map(it -> getConversionService().convert(it, Instant.class))//
+                    .map(it -> it.toEpochMilli())
+                    .findFirst().orElseThrow(() -> new IllegalArgumentException(format("Invalid data type for @LastModifiedDate on Entity %s", this.getAssociation())));
+        }
+
+        return -1L;
     }
 
     @Override

diff --git a/...est/src/test/java/it/internal/org/springframework/content/rest/controllers/BaseUriIT.java b/...est/src/test/java/it/internal/org/springframework/content/rest/controllers/BaseUriIT.java
@@ -10,6 +10,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.content.rest.config.HypermediaConfiguration;
 import org.springframework.content.rest.config.RestConfiguration;
 import org.springframework.data.rest.webmvc.config.RepositoryRestMvcConfiguration;
 import org.springframework.test.context.ActiveProfiles;
@@ -44,7 +45,8 @@
 		EntityConfig.class,
 		DelegatingWebMvcConfiguration.class,
 		RepositoryRestMvcConfiguration.class,
-		RestConfiguration.class })
+		RestConfiguration.class,
+		HypermediaConfiguration.class })
 @Transactional
 @ActiveProfiles("store")
 public class BaseUriIT {
@@ -142,6 +144,8 @@ public class BaseUriIT {
 						version.setEntity(testEntity4);
 						version.setMvc(mvc);
 						version.setUrl(url);
+						version.setCollectionUrl("/api/testEntity4s");
+						version.setContentLinkRel("content");
 						version.setRepo(repo4);
 						version.setStore(store4);
 						version.setEtag(format("\"%s\"", testEntity4.getVersion()));

diff --git a/...t/internal/org/springframework/content/rest/controllers/ContentEntityRestEndpointsIT.java b/...t/internal/org/springframework/content/rest/controllers/ContentEntityRestEndpointsIT.java
@@ -18,6 +18,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.content.rest.config.HypermediaConfiguration;
 import org.springframework.content.rest.config.RestConfiguration;
 import org.springframework.data.rest.webmvc.config.RepositoryRestMvcConfiguration;
 import org.springframework.mock.web.MockHttpServletResponse;
@@ -41,7 +42,8 @@
 		EntityConfig.class,
 		DelegatingWebMvcConfiguration.class,
 		RepositoryRestMvcConfiguration.class,
-		RestConfiguration.class
+		RestConfiguration.class,
+		HypermediaConfiguration.class
 })
 @Transactional
 @ActiveProfiles("store")
@@ -175,6 +177,8 @@ public class ContentEntityRestEndpointsIT {
 
 						version.setMvc(mvc);
 						version.setUrl(url);
+						version.setCollectionUrl("/testEntity4s");
+						version.setContentLinkRel("content");
 						version.setRepo(repo4);
 						version.setStore(store4);
 						version.setEtag(format("\"%s\"", testEntity4.getVersion()));

diff --git a/.../org/springframework/content/rest/controllers/NestedContentPropertiesRestEndpointsIT.java b/.../org/springframework/content/rest/controllers/NestedContentPropertiesRestEndpointsIT.java
@@ -27,6 +27,7 @@
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.content.commons.property.PropertyPath;
+import org.springframework.content.rest.config.HypermediaConfiguration;
 import org.springframework.content.rest.config.RestConfiguration;
 import org.springframework.core.io.WritableResource;
 import org.springframework.data.rest.webmvc.config.RepositoryRestMvcConfiguration;
@@ -50,7 +51,9 @@
       StoreConfig.class,
       DelegatingWebMvcConfiguration.class,
       RepositoryRestMvcConfiguration.class,
-      RestConfiguration.class })
+      RestConfiguration.class,
+	  HypermediaConfiguration.class
+})
 @Transactional
 @ActiveProfiles("store")
 public class NestedContentPropertiesRestEndpointsIT {
@@ -171,6 +174,8 @@ public class NestedContentPropertiesRestEndpointsIT {
 
 				  versionTests.setMvc(mvc);
 				  versionTests.setUrl("/testEntity10s/" + testEntity10.getId() + "/child/content");
+				  versionTests.setCollectionUrl("/testEntity10s");
+				  versionTests.setContentLinkRel("child/content");
 				  versionTests.setRepo(repository);
 				  versionTests.setStore(store);
 				  versionTests.setEtag(format("\"%s\"", testEntity10.getVersion()));

diff --git a/...al/org/springframework/content/rest/controllers/NestedContentPropertyRestEndpointsIT.java b/...al/org/springframework/content/rest/controllers/NestedContentPropertyRestEndpointsIT.java
@@ -30,6 +30,7 @@
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.content.commons.property.PropertyPath;
+import org.springframework.content.rest.config.HypermediaConfiguration;
 import org.springframework.content.rest.config.RestConfiguration;
 import org.springframework.core.io.WritableResource;
 import org.springframework.data.rest.webmvc.config.RepositoryRestMvcConfiguration;
@@ -58,7 +59,8 @@
       StoreConfig.class,
       DelegatingWebMvcConfiguration.class,
       RepositoryRestMvcConfiguration.class,
-      RestConfiguration.class })
+      RestConfiguration.class,
+	  HypermediaConfiguration.class})
 @Transactional
 @ActiveProfiles("store")
 public class NestedContentPropertyRestEndpointsIT {
@@ -194,6 +196,8 @@ public class NestedContentPropertyRestEndpointsIT {
 
 				  versionTests.setMvc(mvc);
 				  versionTests.setUrl("/testEntity8s/" + testEntity8.getId() + "/child");
+				  versionTests.setCollectionUrl("/testEntity8s");
+				  versionTests.setContentLinkRel("child");
 				  versionTests.setRepo(repository2);
 				  versionTests.setStore(store);
 				  versionTests.setEtag(format("\"%s\"", testEntity8.getVersion()));

diff --git a/...ernal/org/springframework/content/rest/controllers/RestResourceMappedRestEndpointsIT.java b/...ernal/org/springframework/content/rest/controllers/RestResourceMappedRestEndpointsIT.java
@@ -157,6 +157,8 @@ public class RestResourceMappedRestEndpointsIT {
 
 				  versionTests.setMvc(mvc);
 				  versionTests.setUrl("/testEntity11s/" + testEntity11.getId() + "/package/content");
+				  versionTests.setCollectionUrl("/testEntity11s");
+				  versionTests.setContentLinkRel("package/content");
 				  versionTests.setRepo(repo);
 				  versionTests.setStore(store);
 				  versionTests.setEtag(format("\"%s\"", testEntity11.getVersion()));

diff --git a/...-rest/src/test/java/it/internal/org/springframework/content/rest/controllers/Version.java b/...-rest/src/test/java/it/internal/org/springframework/content/rest/controllers/Version.java
@@ -1,8 +1,11 @@
 package it.internal.org.springframework.content.rest.controllers;
 
+import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.util.Optional;
 
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import internal.org.springframework.content.rest.support.ContentEntity;
 import internal.org.springframework.content.rest.support.TestEntity2;
 import internal.org.springframework.content.rest.support.TestEntity4;
@@ -25,10 +28,7 @@
 import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.greaterThan;
-import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
-import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
-import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.multipart;
-import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 
@@ -37,6 +37,8 @@ public class Version {
 
     private MockMvc mvc;
     private String url;
+    private String collectionUrl;
+    private String contentLinkRel;
     private CrudRepository repo;
     private Store store;
     private String etag;
@@ -47,6 +49,30 @@ public static Version tests() {
     }
 
     {
+        Context("#Issue 1975", () -> {
+            It("should always evaluate if-match header even after content deletion", () -> {
+                String entityUrl = mvc.perform(post(collectionUrl).content("{}"))
+                        .andExpect(status().is2xxSuccessful()).andReturn().getResponse().getHeader("Location");
+                assertThat(entityUrl, is(not(nullValue())));
+
+                String body = mvc.perform(get(entityUrl).accept("application/json"))
+                        .andExpect(status().is2xxSuccessful()).andReturn().getResponse().getContentAsString();
+
+                ObjectMapper objectMapper = new ObjectMapper();
+                JsonNode responseJson = objectMapper.readTree(body);
+                JsonNode linksNode = responseJson.get("_links");
+                JsonNode contentNode = linksNode.get(contentLinkRel);
+                String contentHref = contentNode.get("href").asText();
+
+                mvc.perform(put(contentHref).header("If-Match", "\"0\"").contentType("text/plain").content("Hello world!"))
+                        .andExpect(status().is2xxSuccessful());  // Content created
+                mvc.perform(delete(contentHref).header("If-Match", "\"1\""))
+                        .andExpect(status().is2xxSuccessful());  // User A
+                mvc.perform(put(contentHref).header("If-Match", "\"1\"").contentType("text/plain").content("foo bar"))
+                        .andExpect(status().isPreconditionFailed());  // User B
+            });
+        });
+
         Context("a GET request to /{store}/{id}", () -> {
             It("should return an etag header", () -> {
                 MockHttpServletResponse response = mvc