diff --git a/.gitignore b/.gitignore index f0784d4fbe..a9aa65c6e3 100644 --- a/.gitignore +++ b/.gitignore @@ -14,7 +14,7 @@ sandbox/ *~ .DS_Store .factorypath -*md +*.md specs/ .specify docs diff --git a/mvnw.cmd b/mvnw.cmd new file mode 100644 index 0000000000..249bdf3822 --- /dev/null +++ b/mvnw.cmd @@ -0,0 +1,149 @@ +<# : batch portion +@REM ---------------------------------------------------------------------------- +@REM Licensed to the Apache Software Foundation (ASF) under one +@REM or more contributor license agreements. See the NOTICE file +@REM distributed with this work for additional information +@REM regarding copyright ownership. The ASF licenses this file +@REM to you under the Apache License, Version 2.0 (the +@REM "License"); you may not use this file except in compliance +@REM with the License. You may obtain a copy of the License at +@REM +@REM http://www.apache.org/licenses/LICENSE-2.0 +@REM +@REM Unless required by applicable law or agreed to in writing, +@REM software distributed under the License is distributed on an +@REM "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +@REM KIND, either express or implied. See the License for the +@REM specific language governing permissions and limitations +@REM under the License. +@REM ---------------------------------------------------------------------------- + +@REM ---------------------------------------------------------------------------- +@REM Apache Maven Wrapper startup batch script, version 3.3.2 +@REM +@REM Optional ENV vars +@REM MVNW_REPOURL - repo url base for downloading maven distribution +@REM MVNW_USERNAME/MVNW_PASSWORD - user and password for downloading maven +@REM MVNW_VERBOSE - true: enable verbose log; others: silence the output +@REM ---------------------------------------------------------------------------- + +@IF "%__MVNW_ARG0_NAME__%"=="" (SET __MVNW_ARG0_NAME__=%~nx0) +@SET __MVNW_CMD__= +@SET __MVNW_ERROR__= +@SET __MVNW_PSMODULEP_SAVE=%PSModulePath% +@SET PSModulePath= +@FOR /F "usebackq tokens=1* delims==" %%A IN (`powershell -noprofile "& {$scriptDir='%~dp0'; $script='%__MVNW_ARG0_NAME__%'; icm -ScriptBlock ([Scriptblock]::Create((Get-Content -Raw '%~f0'))) -NoNewScope}"`) DO @( + IF "%%A"=="MVN_CMD" (set __MVNW_CMD__=%%B) ELSE IF "%%B"=="" (echo %%A) ELSE (echo %%A=%%B) +) +@SET PSModulePath=%__MVNW_PSMODULEP_SAVE% +@SET __MVNW_PSMODULEP_SAVE= +@SET __MVNW_ARG0_NAME__= +@SET MVNW_USERNAME= +@SET MVNW_PASSWORD= +@IF NOT "%__MVNW_CMD__%"=="" (%__MVNW_CMD__% %*) +@echo Cannot start maven from wrapper >&2 && exit /b 1 +@GOTO :EOF +: end batch / begin powershell #> + +$ErrorActionPreference = "Stop" +if ($env:MVNW_VERBOSE -eq "true") { + $VerbosePreference = "Continue" +} + +# calculate distributionUrl, requires .mvn/wrapper/maven-wrapper.properties +$distributionUrl = (Get-Content -Raw "$scriptDir/.mvn/wrapper/maven-wrapper.properties" | ConvertFrom-StringData).distributionUrl +if (!$distributionUrl) { + Write-Error "cannot read distributionUrl property in $scriptDir/.mvn/wrapper/maven-wrapper.properties" +} + +switch -wildcard -casesensitive ( $($distributionUrl -replace '^.*/','') ) { + "maven-mvnd-*" { + $USE_MVND = $true + $distributionUrl = $distributionUrl -replace '-bin\.[^.]*$',"-windows-amd64.zip" + $MVN_CMD = "mvnd.cmd" + break + } + default { + $USE_MVND = $false + $MVN_CMD = $script -replace '^mvnw','mvn' + break + } +} + +# apply MVNW_REPOURL and calculate MAVEN_HOME +# maven home pattern: ~/.m2/wrapper/dists/{apache-maven-,maven-mvnd--}/ +if ($env:MVNW_REPOURL) { + $MVNW_REPO_PATTERN = if ($USE_MVND) { "/org/apache/maven/" } else { "/maven/mvnd/" } + $distributionUrl = "$env:MVNW_REPOURL$MVNW_REPO_PATTERN$($distributionUrl -replace '^.*'+$MVNW_REPO_PATTERN,'')" +} +$distributionUrlName = $distributionUrl -replace '^.*/','' +$distributionUrlNameMain = $distributionUrlName -replace '\.[^.]*$','' -replace '-bin$','' +$MAVEN_HOME_PARENT = "$HOME/.m2/wrapper/dists/$distributionUrlNameMain" +if ($env:MAVEN_USER_HOME) { + $MAVEN_HOME_PARENT = "$env:MAVEN_USER_HOME/wrapper/dists/$distributionUrlNameMain" +} +$MAVEN_HOME_NAME = ([System.Security.Cryptography.MD5]::Create().ComputeHash([byte[]][char[]]$distributionUrl) | ForEach-Object {$_.ToString("x2")}) -join '' +$MAVEN_HOME = "$MAVEN_HOME_PARENT/$MAVEN_HOME_NAME" + +if (Test-Path -Path "$MAVEN_HOME" -PathType Container) { + Write-Verbose "found existing MAVEN_HOME at $MAVEN_HOME" + Write-Output "MVN_CMD=$MAVEN_HOME/bin/$MVN_CMD" + exit $? +} + +if (! $distributionUrlNameMain -or ($distributionUrlName -eq $distributionUrlNameMain)) { + Write-Error "distributionUrl is not valid, must end with *-bin.zip, but found $distributionUrl" +} + +# prepare tmp dir +$TMP_DOWNLOAD_DIR_HOLDER = New-TemporaryFile +$TMP_DOWNLOAD_DIR = New-Item -Itemtype Directory -Path "$TMP_DOWNLOAD_DIR_HOLDER.dir" +$TMP_DOWNLOAD_DIR_HOLDER.Delete() | Out-Null +trap { + if ($TMP_DOWNLOAD_DIR.Exists) { + try { Remove-Item $TMP_DOWNLOAD_DIR -Recurse -Force | Out-Null } + catch { Write-Warning "Cannot remove $TMP_DOWNLOAD_DIR" } + } +} + +New-Item -Itemtype Directory -Path "$MAVEN_HOME_PARENT" -Force | Out-Null + +# Download and Install Apache Maven +Write-Verbose "Couldn't find MAVEN_HOME, downloading and installing it ..." +Write-Verbose "Downloading from: $distributionUrl" +Write-Verbose "Downloading to: $TMP_DOWNLOAD_DIR/$distributionUrlName" + +$webclient = New-Object System.Net.WebClient +if ($env:MVNW_USERNAME -and $env:MVNW_PASSWORD) { + $webclient.Credentials = New-Object System.Net.NetworkCredential($env:MVNW_USERNAME, $env:MVNW_PASSWORD) +} +[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 +$webclient.DownloadFile($distributionUrl, "$TMP_DOWNLOAD_DIR/$distributionUrlName") | Out-Null + +# If specified, validate the SHA-256 sum of the Maven distribution zip file +$distributionSha256Sum = (Get-Content -Raw "$scriptDir/.mvn/wrapper/maven-wrapper.properties" | ConvertFrom-StringData).distributionSha256Sum +if ($distributionSha256Sum) { + if ($USE_MVND) { + Write-Error "Checksum validation is not supported for maven-mvnd. `nPlease disable validation by removing 'distributionSha256Sum' from your maven-wrapper.properties." + } + Import-Module $PSHOME\Modules\Microsoft.PowerShell.Utility -Function Get-FileHash + if ((Get-FileHash "$TMP_DOWNLOAD_DIR/$distributionUrlName" -Algorithm SHA256).Hash.ToLower() -ne $distributionSha256Sum) { + Write-Error "Error: Failed to validate Maven distribution SHA-256, your Maven distribution might be compromised. If you updated your Maven version, you need to update the specified distributionSha256Sum property." + } +} + +# unzip and move +Expand-Archive "$TMP_DOWNLOAD_DIR/$distributionUrlName" -DestinationPath "$TMP_DOWNLOAD_DIR" | Out-Null +Rename-Item -Path "$TMP_DOWNLOAD_DIR/$distributionUrlNameMain" -NewName $MAVEN_HOME_NAME | Out-Null +try { + Move-Item -Path "$TMP_DOWNLOAD_DIR/$MAVEN_HOME_NAME" -Destination $MAVEN_HOME_PARENT | Out-Null +} catch { + if (! (Test-Path -Path "$MAVEN_HOME" -PathType Container)) { + Write-Error "fail to move MAVEN_HOME" + } +} finally { + try { Remove-Item $TMP_DOWNLOAD_DIR -Recurse -Force | Out-Null } + catch { Write-Warning "Cannot remove $TMP_DOWNLOAD_DIR" } +} + +Write-Output "MVN_CMD=$MAVEN_HOME/bin/$MVN_CMD" diff --git a/pom.xml b/pom.xml index d33b0a3e26..b99f747169 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ org.ohdsi WebAPI ${packaging.type} - 3.0.0-SNAPSHOT + 3.0.1-SNAPSHOT WebAPI war diff --git a/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java b/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java index 25d2f4955e..4059b3b3d2 100644 --- a/src/main/java/org/ohdsi/webapi/service/AbstractDaoService.java +++ b/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 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)); } } } diff --git a/src/main/java/org/ohdsi/webapi/tag/TagService.java b/src/main/java/org/ohdsi/webapi/tag/TagService.java index 62fe7717b5..87d422c4e0 100644 --- a/src/main/java/org/ohdsi/webapi/tag/TagService.java +++ b/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 groupIds = tag.getGroups().stream() diff --git a/src/main/java/org/ohdsi/webapi/tag/converter/TagDTOToTagConverter.java b/src/main/java/org/ohdsi/webapi/tag/converter/TagDTOToTagConverter.java index 2f8444f686..97b8d0e08a 100644 --- a/src/main/java/org/ohdsi/webapi/tag/converter/TagDTOToTagConverter.java +++ b/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 groups = source.getGroups().stream() + if (source.getGroups() != null) { + Set 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()); diff --git a/src/test/java/org/ohdsi/webapi/tag/TagServiceTest.java b/src/test/java/org/ohdsi/webapi/tag/TagServiceTest.java new file mode 100644 index 0000000000..cc1418ad3c --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/src/test/java/org/ohdsi/webapi/tagging/CohortTaggingTest.java b/src/test/java/org/ohdsi/webapi/tagging/CohortTaggingTest.java index b986742f5a..12c44fb3f0 100644 --- a/src/test/java/org/ohdsi/webapi/tagging/CohortTaggingTest.java +++ b/src/test/java/org/ohdsi/webapi/tagging/CohortTaggingTest.java @@ -5,12 +5,17 @@ import org.ohdsi.webapi.cohortdefinition.dto.CohortDTO; import org.ohdsi.webapi.cohortdefinition.dto.CohortRawDTO; import org.ohdsi.webapi.tag.domain.Tag; +import org.ohdsi.webapi.tag.domain.TagType; import org.ohdsi.webapi.tag.dto.TagNameListRequestDTO; import org.springframework.beans.factory.annotation.Autowired; import java.io.IOException; +import java.util.Date; +import java.util.HashSet; import java.util.List; +import static org.junit.Assert.assertEquals; + public class CohortTaggingTest extends BaseTaggingTest { private static final String JSON_PATH = "/tagging/cohort.json"; @@ -64,6 +69,44 @@ protected void unassignTag(Integer id, boolean isPermissionProtected) { service.unassignTag(id, getTag(isPermissionProtected).getId()); } + @org.junit.Test + public void unassignGroupedTagRemovesJoinTableRow() { + Tag group = new Tag(); + group.setName("grouped tag parent"); + group.setType(TagType.SYSTEM); + group.setCreatedDate(new Date()); + group.setGroups(new HashSet<>()); + group = tagRepository.save(group); + + Tag child = new Tag(); + child.setName("grouped tag child"); + child.setType(TagType.SYSTEM); + child.setCreatedDate(new Date()); + child.setGroups(new HashSet<>(List.of(group))); + child = tagRepository.save(child); + + try { + service.assignTag(getId(initialDTO), child.getId()); + assertEquals(Integer.valueOf(1), jdbcTemplate.queryForObject( + "SELECT count(*) FROM cohort_tag WHERE asset_id = ? AND tag_id = ?", + Integer.class, + getId(initialDTO), + child.getId())); + + service.unassignTag(getId(initialDTO), child.getId()); + assertEquals(Integer.valueOf(0), jdbcTemplate.queryForObject( + "SELECT count(*) FROM cohort_tag WHERE asset_id = ? AND tag_id = ?", + Integer.class, + getId(initialDTO), + child.getId())); + } finally { + child.setGroups(new HashSet<>()); + tagRepository.save(child); + tagRepository.delete(child); + tagRepository.delete(group); + } + } + @Override protected void assignProtectedTag(Integer id, boolean isPermissionProtected) { service.assignPermissionProtectedTag(id, getTag(isPermissionProtected).getId());