Fix REST endpoints for adding tags

The `/tag/{name}/project` endpoint did not add the provided tag to projects which already had tags assigned to them. The `/tag/{name}/policy` and `/tag/{name}/notificationRule` endpoints replaced existing tags with the provided tag, which is not the intended behavior. Since only one tag can be provided via path parameter, the tag should be *added* to existing tags, not replace them. Fixes #4539 Signed-off-by: nscuro <[email protected]>
DependencyTrack · Jan 11, 2025 · cb4590f · cb4590f
1 parent bcd2651
commit cb4590f
Show file tree

Hide file tree

Showing 6 changed files with 123 additions and 44 deletions.
diff --git a/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java b/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java
@@ -239,21 +239,23 @@ public void deleteNotificationPublisher(final NotificationPublisher notification
     }
 
     /**
-     * @since 4.12.0
+     * @since 4.12.3
      */
     @Override
-    public boolean bind(final NotificationRule notificationRule, final Collection<Tag> tags) {
+    public boolean bind(final NotificationRule notificationRule, final Collection<Tag> tags, final boolean keepExisting) {
         assertPersistent(notificationRule, "notificationRule must be persistent");
         assertPersistentAll(tags, "tags must be persistent");
 
         return callInTransaction(() -> {
             boolean modified = false;
 
-            for (final Tag existingTag : notificationRule.getTags()) {
-                if (!tags.contains(existingTag)) {
-                    notificationRule.getTags().remove(existingTag);
-                    existingTag.getNotificationRules().remove(notificationRule);
-                    modified = true;
+            if (!keepExisting) {
+                for (final Tag existingTag : notificationRule.getTags()) {
+                    if (!tags.contains(existingTag)) {
+                        notificationRule.getTags().remove(existingTag);
+                        existingTag.getNotificationRules().remove(notificationRule);
+                        modified = true;
+                    }
                 }
             }
 
@@ -275,4 +277,12 @@ public boolean bind(final NotificationRule notificationRule, final Collection<Ta
         });
     }
 
+    /**
+     * @since 4.12.0
+     */
+    @Override
+    public boolean bind(final NotificationRule notificationRule, final Collection<Tag> tags) {
+        return bind(notificationRule, tags, /* keepExisting */ false);
+    }
+
 }
diff --git a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java
@@ -831,21 +831,23 @@ void preprocessACLs(final Query<?> query, final String inputFilter, final Map<St
     }
 
     /**
-     * @since 4.12.0
+     * @since 4.12.3
      */
     @Override
-    public boolean bind(final Policy policy, final Collection<Tag> tags) {
+    public boolean bind(final Policy policy, final Collection<Tag> tags, final boolean keepExisting) {
         assertPersistent(policy, "policy must be persistent");
         assertPersistentAll(tags, "tags must be persistent");
 
         return callInTransaction(() -> {
             boolean modified = false;
 
-            for (final Tag existingTag : policy.getTags()) {
-                if (!tags.contains(existingTag)) {
-                    policy.getTags().remove(existingTag);
-                    existingTag.getPolicies().remove(policy);
-                    modified = true;
+            if (!keepExisting) {
+                for (final Tag existingTag : policy.getTags()) {
+                    if (!tags.contains(existingTag)) {
+                        policy.getTags().remove(existingTag);
+                        existingTag.getPolicies().remove(policy);
+                        modified = true;
+                    }
                 }
             }
 
@@ -867,4 +869,12 @@ public boolean bind(final Policy policy, final Collection<Tag> tags) {
         });
     }
 
+    /**
+     * @since 4.12.0
+     */
+    @Override
+    public boolean bind(final Policy policy, final Collection<Tag> tags) {
+        return bind(policy, tags, /* keepExisting */ false);
+    }
+
 }
diff --git a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java
@@ -49,13 +49,13 @@
 import org.dependencytrack.model.FindingAttribution;
 import org.dependencytrack.model.PolicyViolation;
 import org.dependencytrack.model.Project;
+import org.dependencytrack.model.ProjectCollectionLogic;
 import org.dependencytrack.model.ProjectMetadata;
 import org.dependencytrack.model.ProjectProperty;
 import org.dependencytrack.model.ProjectVersion;
 import org.dependencytrack.model.ServiceComponent;
 import org.dependencytrack.model.Tag;
 import org.dependencytrack.model.Vulnerability;
-import org.dependencytrack.model.ProjectCollectionLogic;
 import org.dependencytrack.notification.NotificationConstants;
 import org.dependencytrack.notification.NotificationGroup;
 import org.dependencytrack.notification.NotificationScope;
@@ -83,6 +83,8 @@
 import java.util.stream.Collectors;
 
 import static org.datanucleus.PropertyNames.PROPERTY_QUERY_SQL_ALLOWALL;
+import static org.dependencytrack.util.PersistenceUtil.assertPersistent;
+import static org.dependencytrack.util.PersistenceUtil.assertPersistentAll;
 
 final class ProjectQueryManager extends QueryManager implements IQueryManager {
 
@@ -1370,32 +1372,54 @@ public List<ProjectProperty> getProjectProperties(final Project project) {
     }
 
     /**
-     * Binds the two objects together in a corresponding join table.
-     * @param project a Project object
-     * @param tags a List of Tag objects
+     * @since 4.12.3
      */
     @Override
-    public void bind(Project project, List<Tag> tags) {
-        runInTransaction(() -> {
-            final Query<Tag> query = pm.newQuery(Tag.class, "projects.contains(:project)");
-            query.setParameters(project);
-            final List<Tag> currentProjectTags = executeAndCloseList(query);
-
-            for (final Tag tag : currentProjectTags) {
-                if (!tags.contains(tag)) {
-                    tag.getProjects().remove(project);
+    public boolean bind(final Project project, final Collection<Tag> tags, final boolean keepExisting) {
+        assertPersistent(project, "project must be persistent");
+        assertPersistentAll(tags, "tags must be persistent");
+
+        return callInTransaction(() -> {
+            boolean modified = false;
+
+            if (!keepExisting) {
+                for (final Tag existingTag : project.getTags()) {
+                    if (!tags.contains(existingTag)) {
+                        project.getTags().remove(existingTag);
+                        existingTag.getProjects().remove(project);
+                        modified = true;
+                    }
                 }
             }
-            project.setTags(tags);
+
             for (final Tag tag : tags) {
-                final List<Project> projects = tag.getProjects();
-                if (!projects.contains(project)) {
-                    projects.add(project);
+                if (!project.getTags().contains(tag)) {
+                    project.getTags().add(tag);
+
+                    if (tag.getProjects() == null) {
+                        tag.setProjects(new ArrayList<>(List.of(project)));
+                    } else if (!tag.getProjects().contains(project)) {
+                        tag.getProjects().add(project);
+                    }
+
+                    modified = true;
                 }
             }
+
+            return modified;
         });
     }
 
+    /**
+     * Binds the two objects together in a corresponding join table.
+     * @param project a Project object
+     * @param tags a List of Tag objects
+     */
+    @Override
+    public void bind(final Project project, final List<Tag> tags) {
+        bind(project, tags, /* keepExisting */ false);
+    }
+
     /**
      * Updates the last time a bom was imported.
      * @param date the date of the last bom import

diff --git a/src/main/java/org/dependencytrack/persistence/QueryManager.java b/src/main/java/org/dependencytrack/persistence/QueryManager.java
@@ -1318,14 +1318,26 @@ public void clearComponentAnalysisCache(Date threshold) {
         getCacheQueryManager().clearComponentAnalysisCache(threshold);
     }
 
+    public boolean bind(final NotificationRule notificationRule, final Collection<Tag> tags, final boolean keepExisting) {
+        return getNotificationQueryManager().bind(notificationRule, tags, keepExisting);
+    }
+
     public boolean bind(final NotificationRule notificationRule, final Collection<Tag> tags) {
         return getNotificationQueryManager().bind(notificationRule, tags);
     }
 
+    public boolean bind(final Project project, final Collection<Tag> tags, final boolean keepExisting) {
+        return getProjectQueryManager().bind(project, tags, keepExisting);
+    }
+
     public void bind(Project project, List<Tag> tags) {
         getProjectQueryManager().bind(project, tags);
     }
 
+    public boolean bind(final Policy policy, final Collection<Tag> tags, final boolean keepExisting) {
+        return getPolicyQueryManager().bind(policy, tags, keepExisting);
+    }
+
     public boolean bind(final Policy policy, final Collection<Tag> tags) {
         return getPolicyQueryManager().bind(policy, tags);
     }

diff --git a/src/main/java/org/dependencytrack/persistence/TagQueryManager.java b/src/main/java/org/dependencytrack/persistence/TagQueryManager.java
@@ -459,14 +459,7 @@ public void tagProjects(final String tagName, final Collection<String> projectUu
             final List<Project> projects = executeAndCloseList(projectsQuery);
 
             for (final Project project : projects) {
-                if (project.getTags() == null || project.getTags().isEmpty()) {
-                    project.setTags(List.of(tag));
-                    continue;
-                }
-
-                if (!project.getTags().contains(tag)) {
-                    project.getTags().add(tag);
-                }
+                bind(project, List.of(tag), /* keepExisting */ true);
             }
         });
     }
@@ -569,7 +562,7 @@ public void tagPolicies(final String tagName, final Collection<String> policyUui
             final List<Policy> policies = executeAndCloseList(policiesQuery);
 
             for (final Policy policy : policies) {
-                bind(policy, List.of(tag));
+                bind(policy, List.of(tag), /* keepExisting */ true);
             }
         });
     }
@@ -694,7 +687,7 @@ public void tagNotificationRules(final String tagName, final Collection<String>
             final List<NotificationRule> notificationRules = executeAndCloseList(notificationRulesQuery);
 
             for (final NotificationRule notificationRule : notificationRules) {
-                bind(notificationRule, List.of(tag));
+                bind(notificationRule, List.of(tag), /* keepExisting */ true);
             }
         });
     }

diff --git a/src/test/java/org/dependencytrack/resources/v1/TagResourceTest.java b/src/test/java/org/dependencytrack/resources/v1/TagResourceTest.java
@@ -700,15 +700,24 @@ public void tagProjectsTest() {
 
         qm.createTag("foo");
 
+        final var projectC = new Project();
+        projectC.setName("acme-app-c");
+        qm.persist(projectC);
+
+        qm.bind(projectC, List.of(qm.createTag("bar")));
+
         final Response response = jersey.target(V1_TAG + "/foo/project")
                 .request()
                 .header(X_API_KEY, apiKey)
-                .post(Entity.json(List.of(projectA.getUuid(), projectB.getUuid())));
+                .post(Entity.json(List.of(projectA.getUuid(), projectB.getUuid(), projectC.getUuid())));
         assertThat(response.getStatus()).isEqualTo(204);
 
         qm.getPersistenceManager().evictAll();
         assertThat(projectA.getTags()).satisfiesExactly(projectTag -> assertThat(projectTag.getName()).isEqualTo("foo"));
         assertThat(projectB.getTags()).satisfiesExactly(projectTag -> assertThat(projectTag.getName()).isEqualTo("foo"));
+        assertThat(projectC.getTags()).satisfiesExactlyInAnyOrder(
+                projectTag -> assertThat(projectTag.getName()).isEqualTo("foo"),
+                projectTag -> assertThat(projectTag.getName()).isEqualTo("bar"));
     }
 
     @Test
@@ -1126,15 +1135,26 @@ public void tagPoliciesTest() {
 
         qm.createTag("foo");
 
+        final var policyC = new Policy();
+        policyC.setName("policy-c");
+        policyC.setOperator(Policy.Operator.ALL);
+        policyC.setViolationState(Policy.ViolationState.INFO);
+        qm.persist(policyC);
+
+        qm.bind(policyC, List.of(qm.createTag("bar")));
+
         final Response response = jersey.target(V1_TAG + "/foo/policy")
                 .request()
                 .header(X_API_KEY, apiKey)
-                .post(Entity.json(List.of(policyA.getUuid(), policyB.getUuid())));
+                .post(Entity.json(List.of(policyA.getUuid(), policyB.getUuid(), policyC.getUuid())));
         assertThat(response.getStatus()).isEqualTo(204);
 
         qm.getPersistenceManager().evictAll();
         assertThat(policyA.getTags()).satisfiesExactly(policyTag -> assertThat(policyTag.getName()).isEqualTo("foo"));
         assertThat(policyB.getTags()).satisfiesExactly(policyTag -> assertThat(policyTag.getName()).isEqualTo("foo"));
+        assertThat(policyC.getTags()).satisfiesExactlyInAnyOrder(
+                projectTag -> assertThat(projectTag.getName()).isEqualTo("foo"),
+                projectTag -> assertThat(projectTag.getName()).isEqualTo("bar"));
     }
 
     @Test
@@ -1514,15 +1534,25 @@ public void tagNotificationRulesTest() {
 
         qm.createTag("foo");
 
+        final var notificationRuleC = new NotificationRule();
+        notificationRuleC.setName("rule-c");
+        notificationRuleC.setScope(NotificationScope.PORTFOLIO);
+        qm.persist(notificationRuleC);
+
+        qm.bind(notificationRuleC, List.of(qm.createTag("bar")));
+
         final Response response = jersey.target(V1_TAG + "/foo/notificationRule")
                 .request()
                 .header(X_API_KEY, apiKey)
-                .post(Entity.json(List.of(notificationRuleA.getUuid(), notificationRuleB.getUuid())));
+                .post(Entity.json(List.of(notificationRuleA.getUuid(), notificationRuleB.getUuid(), notificationRuleC.getUuid())));
         assertThat(response.getStatus()).isEqualTo(204);
 
         qm.getPersistenceManager().evictAll();
         assertThat(notificationRuleA.getTags()).satisfiesExactly(ruleTag -> assertThat(ruleTag.getName()).isEqualTo("foo"));
         assertThat(notificationRuleB.getTags()).satisfiesExactly(ruleTag -> assertThat(ruleTag.getName()).isEqualTo("foo"));
+        assertThat(notificationRuleC.getTags()).satisfiesExactlyInAnyOrder(
+                projectTag -> assertThat(projectTag.getName()).isEqualTo("foo"),
+                projectTag -> assertThat(projectTag.getName()).isEqualTo("bar"));
     }
 
     @Test