Fix: Accession API upsert fails for "storage" and conversion to array does not handle null values

- Added JUnit test for /upsert
- Fixed AccessionId#removeEmpty (it must reuse existing collection)

src/main/java/org/genesys2/server/api/v0/AccessionController.java

@@ -79,11 +79,11 @@ import io.swagger.annotations.Api;
 
 @RestController("accessionApi0")
 @PreAuthorize("isAuthenticated()")
-@RequestMapping(value = { AccessionController.CONTROLLER_URL, "/json/v0/acn" })
+@RequestMapping(value = { AccessionController.API_BASE, "/json/v0/acn" })
 @Api(tags = { "accession" })
 public class AccessionController extends ApiBaseController {
 	private static final int UPLOAD_RETRIES = 10;
-	protected static final String CONTROLLER_URL = ApiBaseController.APIv0_BASE + "/acn";
+	public static final String API_BASE = ApiBaseController.APIv0_BASE + "/acn";
 
 	private final ObjectMapper mapper = new ObjectMapper();
 
@@ -449,11 +449,13 @@ public class AccessionController extends ApiBaseController {
 
 	private void upgradeToArray(ObjectNode obj, String fieldName) {
 		JsonNode node = obj.get(fieldName);
-		if (node != null && !node.isArray()) {
-			ArrayNode arrNode = obj.putArray(fieldName);
-			for (String x : node.asText().split(";")) {
-				if (StringUtils.isNotBlank(x)) {
-					arrNode.add(StringUtils.trim(x));
+		if (node != null) {
+			if (! node.isNull() && ! node.isArray()) {
+				ArrayNode arrNode = obj.putArray(fieldName);
+				for (String x : node.asText().split(";")) {
+					if (StringUtils.isNotBlank(x)) {
+						arrNode.add(StringUtils.trim(x));
+					}
 				}
 			}
 		}

src/main/java/org/genesys2/server/model/genesys/AccessionId.java

@@ -100,7 +100,7 @@ public class AccessionId extends AuditedVersionedModel implements IdUUID {
 	@CollectionTable(name = "accession_storage", joinColumns = @JoinColumn(name = "accessionId", referencedColumnName = "id"))
 	@OrderBy("storage")
 	@Field(type = FieldType.Auto, index = FieldIndex.not_analyzed)
-	private Set<Integer> storage = null;
+	private Set<Integer> storage;
 
 	@Column(name = "duplSite", nullable = false, length = 9)
 	@ElementCollection(fetch = FetchType.LAZY)
@@ -179,13 +179,15 @@ public class AccessionId extends AuditedVersionedModel implements IdUUID {
 		if (set == null || set.isEmpty()) {
 			return set;
 		}
-		return set.stream().filter(s -> {
-			if (s != null && s instanceof String) {
-				return StringUtils.isNotBlank((String) s);
+		set.removeAll(set.stream().filter(s -> {
+			if (s instanceof String) {
+				return StringUtils.isBlank((String) s);
 			} else {
-				return s != null;
+				return s == null;
 			}
-		}).collect(Collectors.toSet());
+		}).collect(Collectors.toSet()));
+		
+		return set;
 	}
 
 	@Override

src/main/java/org/genesys2/server/service/worker/AccessionUploader.java

@@ -398,7 +398,7 @@ public class AccessionUploader implements InitializingBean {
 		}
 	}
 
-	@SuppressWarnings("rawtypes")
+	@SuppressWarnings({ "unchecked" })
 	private <T> void copy(Class<T> clazz, T source, T target, Iterator<String> fieldNames) throws IllegalArgumentException, IllegalAccessException {
 		String fieldName = null;
 		while (fieldNames.hasNext() && (fieldName = fieldNames.next()) != null) {
@@ -409,10 +409,14 @@ public class AccessionUploader implements InitializingBean {
 				final Object srcValue = field.get(source);
 				final Object dest = field.get(target);
 				// handle collections better
-				if (srcValue instanceof Collection && dest != null) {
-					Collection collection = (Collection) dest;
+				if (dest != null && dest instanceof Collection) {
+					Collection<Object> collection = (Collection<Object>) dest;
 					collection.clear();
-					collection.addAll((Collection) srcValue);
+					if (srcValue != null && srcValue instanceof Collection) {
+						collection.addAll(((Collection<Object>) srcValue).stream().filter(o -> o != null).collect(Collectors.toList()));
+					} else {
+						LOG.trace("Clearing {}, source was empty", fieldName);
+					}
 				} else {
 					field.set(target, srcValue);
 				}

src/test/java/org/genesys/test/server/api/v1/AccessionControllerTest.java

+/*
+ * Copyright 2018 Global Crop Diversity Trust
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.genesys.test.server.api.v1;
+
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.assertThat;
+import static org.springframework.restdocs.mockmvc.MockMvcRestDocumentation.documentationConfiguration;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
+
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.genesys.test.base.AbstractApiTest;
+import org.genesys2.server.api.v0.AccessionController;
+import org.genesys2.server.model.genesys.Accession;
+import org.genesys2.server.model.impl.FaoInstitute;
+import org.genesys2.server.persistence.AccessionIdRepository;
+import org.genesys2.server.persistence.AccessionRepository;
+import org.genesys2.server.persistence.FaoInstituteRepository;
+import org.genesys2.server.persistence.Taxonomy2Repository;
+import org.genesys2.server.service.InstituteService;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.http.MediaType;
+import org.springframework.restdocs.JUnitRestDocumentation;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.ResultActions;
+import org.springframework.test.web.servlet.setup.MockMvcBuilders;
+import org.springframework.transaction.annotation.Transactional;
+import org.springframework.web.context.WebApplicationContext;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.google.common.collect.Lists;
+
+/**
+ * @author Matija Obreza
+ */
+public class AccessionControllerTest extends AbstractApiTest {
+
+	private static final int STORAGE_10 = 10;
+	private static final int STORAGE_11 = 11;
+
+	@Rule
+	public final JUnitRestDocumentation restDocumentation = new JUnitRestDocumentation("target/generated-snippets");
+
+	@Autowired
+	private WebApplicationContext webApplicationContext;
+	// @Autowired
+	// private AccessionService accessionService;
+	@Autowired
+	private AccessionRepository accessionRepository;
+	@Autowired
+	private AccessionIdRepository accessionIdRepository;
+	@Autowired
+	private FaoInstituteRepository instituteRepository;
+	@Autowired
+	private InstituteService instituteService;
+	@Autowired
+	private Taxonomy2Repository taxonomyRepository;
+
+	MockMvc mockMvc;
+
+	private FaoInstitute institute;
+	private AtomicInteger acceNumb = new AtomicInteger(1);
+
+	protected static final ObjectMapper objectMapper;
+
+	protected static final ObjectMapper verboseMapper = new ObjectMapper();
+
+	static {
+		objectMapper = new ObjectMapper();
+		objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
+		objectMapper.configure(DeserializationFeature.ACCEPT_SINGLE_VALUE_AS_ARRAY, true);
+		objectMapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY);
+	}
+
+	@After
+	@Override
+	@Transactional
+	public void cleanup() throws Exception {
+		accessionRepository.deleteAll();
+		accessionIdRepository.deleteAll();
+		taxonomyRepository.deleteAll();
+		instituteRepository.deleteAll();
+		super.cleanup();
+	}
+
+	@Before
+	@Override
+	@Transactional
+	public void beforeTest() throws Exception {
+		super.beforeTest();
+		mockMvc = MockMvcBuilders.webAppContextSetup(webApplicationContext).apply(documentationConfiguration(this.restDocumentation).uris().withScheme("https").withHost(
+			"sandbox.genesys-pgr.org").withPort(443)).build();
+		institute = new FaoInstitute();
+		institute.setCode("INS001");
+		institute.setFullName("An institute");
+		instituteService.update(Lists.newArrayList(institute));
+		institute = instituteService.getInstitute("INS001");
+	}
+
+	@Test
+	public void createAccessionTest() throws Exception {
+		ObjectNode accessionJson = setUpAccession();
+		final String s = "[" + verboseMapper.writeValueAsString(accessionJson) + "]";
+
+		/*@formatter:off*/
+        mockMvc.perform(post(AccessionController.API_BASE + "/" + institute.getCode() + "/upsert")
+        	.contentType(MediaType.APPLICATION_JSON)
+            .content(s))
+//            .andDo(MockMvcResultHandlers.print())
+            .andExpect(status().isOk())
+            .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8))
+            .andExpect(jsonPath("$", not(nullValue())))
+            .andExpect(jsonPath("$[0].instCode", is(institute.getCode())))
+            .andExpect(jsonPath("$[0].acceNumb", is(accessionJson.get("accessionNumber").textValue())))
+            .andExpect(jsonPath("$[0].genus", is(accessionJson.get("taxonomy").get("genus").textValue())))
+            .andExpect(jsonPath("$[0].error", nullValue()))
+        	.andExpect(jsonPath("$[0].result.action", equalTo("INSERT")));
+		/*@formatter:on*/
+
+		assertThat(accessionRepository.count(), is(1L));
+	}
+
+	@Test
+	public void updateAccessionNoChange() throws Exception {
+		ObjectNode accessionJson = setUpAccession();
+
+		/*@formatter:off*/
+		upsert(accessionJson)
+			.andExpect(jsonPath("$[0].result.action", equalTo("INSERT")));
+		/*@formatter:on*/
+
+		assertThat(accessionRepository.count(), is(1L));
+
+		/*@formatter:off*/
+        upsert(accessionJson)
+        	.andExpect(jsonPath("$[0].result.action", equalTo("UPDATE")))
+        	.andExpect(jsonPath("$[0].result.uuid", notNullValue()));
+		/*@formatter:on*/
+
+		assertThat(accessionRepository.count(), is(1L));
+	}
+
+	@Test
+	public void updateAccessionStorage() throws Exception {
+		ObjectNode accessionJson = setUpAccession();
+
+		upsert(accessionJson);
+
+		assertThat(accessionRepository.count(), is(1L));
+		ArrayNode storage = accessionJson.putArray("storage");
+		storage.add(STORAGE_10);
+
+		/*@formatter:off*/
+        upsert(accessionJson)
+        	.andExpect(jsonPath("$[0].result.action", equalTo("UPDATE")));
+		/*@formatter:on*/
+
+		assertThat(accessionRepository.count(), is(1L));
+		UUID uuid = accessionRepository.findAll().get(0).getUuid();
+		Accession result = fetch(uuid);
+		assertThat(result.getAccessionId().getStorage(), containsInAnyOrder(STORAGE_10));
+
+		// More storage
+		storage.add(STORAGE_11);
+		upsert(accessionJson);
+		result = fetch(uuid);
+		assertThat(result.getAccessionId().getStorage(), containsInAnyOrder(STORAGE_10, STORAGE_11));
+
+		// More storage
+		storage.add((Long) null);
+		upsert(accessionJson);
+		result = fetch(uuid);
+		assertThat(result.getAccessionId().getStorage(), containsInAnyOrder(STORAGE_10, STORAGE_11));
+
+		// No storage
+		accessionJson.set("storage", null);
+		upsert(accessionJson);
+		result = fetch(uuid);
+		assertThat(result.getAccessionId().getStorage().size(), is(0));
+	}
+	
+
+	@Test
+	public void testUpgradeToArray() throws Exception {
+		ObjectNode accessionJson = setUpAccession();
+
+		upsert(accessionJson);
+		assertThat(accessionRepository.count(), is(1L));
+
+		accessionJson.put("breederCode", "BREEDER1");
+		/*@formatter:off*/
+        upsert(accessionJson)
+        	.andExpect(jsonPath("$[0].result.action", equalTo("UPDATE")));
+		/*@formatter:on*/
+
+		assertThat(accessionRepository.count(), is(1L));
+		UUID uuid = accessionRepository.findAll().get(0).getUuid();
+		Accession result = fetch(uuid);
+		assertThat(result.getAccessionId().getBreederCode(), containsInAnyOrder("BREEDER1"));
+		
+		accessionJson.set("breederCode", null);
+		upsert(accessionJson);
+		result = fetch(uuid);
+		assertThat("breederCode must be an empty array", result.getAccessionId().getBreederCode().size(), is(0));
+	}
+
+	private ResultActions upsert(ObjectNode accessionJson) throws Exception, JsonProcessingException {
+		/*@formatter:off*/
+		return mockMvc.perform(post(AccessionController.API_BASE + "/" + institute.getCode() + "/upsert")
+                .contentType(MediaType.APPLICATION_JSON)
+                .content("[" + verboseMapper.writeValueAsString(accessionJson) + "]"))
+        		.andExpect(status().isOk())
+//        		.andDo(MockMvcResultHandlers.print())
+                .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8))
+                .andExpect(jsonPath("$", not(nullValue())))
+                .andExpect(jsonPath("$[0].instCode", is(institute.getCode())))
+                .andExpect(jsonPath("$[0].acceNumb", is(accessionJson.get("accessionNumber").textValue())))
+                .andExpect(jsonPath("$[0].genus", is(accessionJson.get("taxonomy").get("genus").textValue())))
+                .andExpect(jsonPath("$[0].error", nullValue()));
+		/*@formatter:on*/
+	}
+
+	private Accession fetch(UUID uuid) throws Exception, JsonProcessingException {
+		/*@formatter:off*/
+		String responseBody = mockMvc.perform(get(org.genesys2.server.api.v1.AccessionController.API_BASE+ "/" + uuid))
+        		.andExpect(status().isOk())
+//        		.andDo(MockMvcResultHandlers.print())
+                .andExpect(content().contentType(MediaType.APPLICATION_JSON_UTF8))
+                .andExpect(jsonPath("$", not(nullValue())))
+                .andExpect(jsonPath("$.institute.code", is(institute.getCode())))
+                .andExpect(jsonPath("$.accessionNumber", notNullValue()))
+                .andExpect(jsonPath("$.uuid", equalTo(uuid.toString())))
+                .andReturn().getResponse().getContentAsString();
+		/*@formatter:on*/
+
+		return objectMapper.readValue(responseBody, Accession.class);
+	}
+
+	private ObjectNode setUpAccession() {
+		ObjectNode accession = objectMapper.createObjectNode();
+		accession.put("instCode", institute.getCode());
+		accession.put("accessionNumber", "A" + acceNumb.incrementAndGet());
+		ObjectNode taxa = accession.putObject("taxonomy");
+		taxa.put("genus", "Manihot");
+		return accession;
+	}
+}