TagService Fixes

.gitignore

@@ -14,7 +14,7 @@ sandbox/
 *~
 .DS_Store
 .factorypath
-*md
+*.md
 specs/
 .specify
 docs

pom.xml

@@ -6,7 +6,7 @@
   <groupId>org.ohdsi</groupId>
   <artifactId>WebAPI</artifactId>
   <packaging>${packaging.type}</packaging>
-  <version>3.0.0-SNAPSHOT</version>
+  <version>3.0.1-SNAPSHOT</version>
   <name>WebAPI</name>
   <properties>
     <packaging.type>war</packaging.type>

src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java

@@ -368,14 +368,16 @@ protected void assignTag(CommonEntityExt<?> entity, int tagId) {
                   tag.getName(), entity.getClass().getSimpleName(), entity.getId()));
         }
 
-        // unassign tags from the same group if group marked as multi_selection=false
+        // Unassign tags from the same group if group marked as multi_selection=false.
+        // Snapshot ids first because unassignTag mutates Hibernate's managed collection.
         tag.getGroups().stream().findFirst().ifPresent(group -> {
           if (!group.isMultiSelection()) {
-            entity.getTags().forEach(t -> {
-              if (t.getGroups().stream().anyMatch(g -> g.getId().equals(group.getId()))) {
-                unassignTag(entity, t.getId());
-              }
-            });
+            entity.getTags().stream()
+                .filter(existing -> existing.getGroups().stream()
+                    .anyMatch(existingGroup -> existingGroup.getId().equals(group.getId())))
+                .map(Tag::getId)
+                .toList()
+                .forEach(existingTagId -> unassignTag(entity, existingTagId));
           }
         });
 
@@ -392,10 +394,13 @@ protected void unassignTag(CommonEntityExt<?> entity, int tagId) {
           throw new AccessDeniedException(String.format("No permission to unassign protected tag '%s' from %s (id=%s).",
                   tag.getName(), entity.getClass().getSimpleName(), entity.getId()));
         }
-        Set<Tag> tags = entity.getTags().stream()
-                .filter(t -> t.getId() != tagId)
-                .collect(Collectors.toSet());
-        entity.setTags(tags);
+        // Keep Hibernate's managed collection intact. Replacing it with a new
+        // Set can make concurrent tag mutations reconcile the entire join table
+        // snapshot, restoring another tag removed by a parallel request.
+        entity.getTags().stream()
+          .filter(existing -> Objects.equals(existing.getId(), tagId))
+          .findFirst()
+          .ifPresent(existing -> entity.getTags().remove(existing));
       }
     }
   }

src/main/java/org/ohdsi/webapi/tag/TagService.java

@@ -73,7 +73,7 @@ public TagDTO create(@RequestBody TagDTO dto) {
 
     public Tag create(Tag tag) {
         tag.setType(TagType.CUSTOM);
-        if (tag.getGroups() == null || tag.getGroups().isEmpty()) {
+        if (tag.getGroups() == null) {
             throw new BadRequestAtlasException("A tag must be assigned to at least one tag group.");
         }
         List<Integer> groupIds = tag.getGroups().stream()

src/main/java/org/ohdsi/webapi/tag/converter/TagDTOToTagConverter.java

@@ -17,10 +17,12 @@ protected Tag createResultObject() {
     @Override
     protected void doConvert(TagDTO source, Tag target) {
         target.setId(source.getId());
-        Set<Tag> groups = source.getGroups().stream()
+        if (source.getGroups() != null) {
+            Set<Tag> groups = source.getGroups().stream()
                 .map(t -> conversionService.convert(t, Tag.class))
                 .collect(Collectors.toSet());
-        target.setGroups(groups);
+            target.setGroups(groups);
+        }
         target.setName(source.getName());
         target.setType(source.getType());
         target.setCount(source.getCount());

src/test/java/org/ohdsi/webapi/tag/TagServiceTest.java

@@ -0,0 +1,110 @@
+package org.ohdsi.webapi.tag;
+
+import org.junit.After;
+import org.junit.Test;
+import org.ohdsi.webapi.AbstractDatabaseTest;
+import org.ohdsi.webapi.exception.BadRequestAtlasException;
+import org.ohdsi.webapi.tag.domain.Tag;
+import org.ohdsi.webapi.tag.domain.TagType;
+import org.ohdsi.webapi.tag.dto.TagDTO;
+import org.ohdsi.webapi.tag.repository.TagRepository;
+import org.springframework.beans.factory.annotation.Autowired;
+
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+public class TagServiceTest extends AbstractDatabaseTest {
+
+    @Autowired
+    private TagService tagService;
+
+    @Autowired
+    private TagRepository tagRepository;
+
+    @After
+    public void cleanupTags() {
+        tagRepository.deleteAll();
+    }
+
+    @Test
+    public void createsUngroupedTagWhenGroupsAreExplicitlyEmpty() {
+        TagDTO tag = tagDTO("tag-group-" + System.nanoTime());
+        tag.setGroups(Collections.emptySet());
+        tag.setShowGroup(true);
+        tag.setMultiSelection(true);
+        tag.setAllowCustom(true);
+
+        TagDTO created = tagService.create(tag);
+
+        assertEquals(TagType.CUSTOM, created.getType());
+        assertTrue(created.getGroups().isEmpty());
+        assertTrue(created.isShowGroup());
+        assertTrue(created.isMultiSelection());
+        assertTrue(created.isAllowCustom());
+    }
+
+    @Test
+    public void createsTagInExistingGroupThatAllowsCustomTags() {
+        Tag parent = saveGroup("parent-" + System.nanoTime(), true);
+        TagDTO tag = tagDTO("child-" + System.nanoTime());
+        tag.setGroups(Collections.singleton(groupDTO(parent.getId())));
+
+        TagDTO created = tagService.create(tag);
+
+        assertEquals(TagType.CUSTOM, created.getType());
+        assertEquals(1, created.getGroups().size());
+        assertEquals(parent.getId(), created.getGroups().iterator().next().getId());
+    }
+
+    @Test
+    public void rejectsTagInExistingGroupThatDoesNotAllowCustomTags() {
+        Tag parent = saveGroup("restricted-parent-" + System.nanoTime(), false);
+        TagDTO tag = tagDTO("child-" + System.nanoTime());
+        tag.setGroups(Collections.singleton(groupDTO(parent.getId())));
+
+        BadRequestAtlasException exception = assertThrows(
+                BadRequestAtlasException.class,
+                () -> tagService.create(tag));
+
+        assertTrue(exception.getMessage().contains(parent.getName()));
+    }
+
+    @Test
+    public void rejectsTagWhenGroupsAreNull() {
+        TagDTO tag = tagDTO("tag-without-groups-" + System.nanoTime());
+
+        BadRequestAtlasException exception = assertThrows(
+                BadRequestAtlasException.class,
+                () -> tagService.create(tag));
+
+        assertEquals("A tag must be assigned to at least one tag group.", exception.getMessage());
+    }
+
+    private Tag saveGroup(String name, boolean allowCustom) {
+        Tag group = new Tag();
+        group.setName(name);
+        group.setType(TagType.CUSTOM);
+        group.setAllowCustom(allowCustom);
+        group.setGroups(new HashSet<>());
+        group.setCreatedDate(new Date());
+        return tagRepository.saveAndFlush(group);
+    }
+
+    private TagDTO tagDTO(String name) {
+        TagDTO tag = new TagDTO();
+        tag.setName(name);
+        return tag;
+    }
+
+    private TagDTO groupDTO(Integer id) {
+        TagDTO group = new TagDTO();
+        group.setId(id);
+        return group;
+    }
+}