SPDX expression support improvements

src/main/java/org/dependencytrack/model/Component.java

@@ -28,6 +28,7 @@
 import com.github.packageurl.MalformedPackageURLException;
 import com.github.packageurl.PackageURL;
 import org.apache.commons.lang3.StringUtils;
+import org.dependencytrack.model.validation.ValidSpdxExpression;
 import org.dependencytrack.resources.v1.serializers.CustomPackageURLSerializer;
 
 import javax.jdo.annotations.Column;
@@ -286,6 +287,7 @@ public enum FetchGroup {
     @Persistent
     @Column(name = "LICENSE_EXPRESSION", jdbcType = "CLOB", allowsNull = "true")
     @Pattern(regexp = RegexSequence.Definition.PRINTABLE_CHARS, message = "The license expression may only contain printable characters")
+    @ValidSpdxExpression
     private String licenseExpression;
 
     @Persistent

src/main/java/org/dependencytrack/model/validation/SpdxExpressionValidator.java

@@ -0,0 +1,40 @@
+/*
+ * This file is part of Dependency-Track.
+ *
+ * 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright (c) Steve Springett. All Rights Reserved.
+ */
+package org.dependencytrack.model.validation;
+
+import org.dependencytrack.parser.spdx.expression.SpdxExpressionParser;
+import org.dependencytrack.parser.spdx.expression.model.SpdxExpression;
+
+import javax.validation.ConstraintValidator;
+import javax.validation.ConstraintValidatorContext;
+import java.util.Objects;
+
+public class SpdxExpressionValidator implements ConstraintValidator<ValidSpdxExpression, String> {
+
+    @Override
+    public boolean isValid(final String expressionString, final ConstraintValidatorContext validatorContext) {
+        if (expressionString == null) {
+            // null-ness is expected to be validated using @NotNull
+            return true;
+        }
+
+        return !Objects.equals(new SpdxExpressionParser().parse(expressionString), SpdxExpression.INVALID);
+    }
+
+}

src/main/java/org/dependencytrack/model/validation/ValidSpdxExpression.java

@@ -0,0 +1,41 @@
+/*
+ * This file is part of Dependency-Track.
+ *
+ * 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright (c) Steve Springett. All Rights Reserved.
+ */
+package org.dependencytrack.model.validation;
+
+import javax.validation.Constraint;
+import javax.validation.Payload;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.FIELD, ElementType.PARAMETER})
+@Constraint(validatedBy = SpdxExpressionValidator.class)
+public @interface ValidSpdxExpression {
+
+    String message() default "The license expression must be a valid SPDX expression";
+
+    Class<?>[] groups() default {};
+
+    Class<? extends Payload>[] payload() default {};
+
+}

src/main/java/org/dependencytrack/parser/cyclonedx/util/ModelConverter.java

@@ -68,6 +68,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.UUID;
 
 public class ModelConverter {
@@ -160,19 +161,25 @@ public static Component convert(final QueryManager qm, final org.cyclonedx.model
         if (licenseChoice != null) {
             final List<org.cyclonedx.model.License> licenseOptions = new ArrayList<>();
             if (licenseChoice.getExpression() != null) {
-                // store license expression, but don't overwrite manual changes to the field
-                if (component.getLicenseExpression() == null) {
-                    component.setLicenseExpression(licenseChoice.getExpression());
-                }
-                // if the expression just consists of one license id, we can add it as another license option
-                SpdxExpressionParser parser = new SpdxExpressionParser();
-                SpdxExpression parsedExpression = parser.parse(licenseChoice.getExpression());
-                if (parsedExpression.getSpdxLicenseId() != null) {
-                    org.cyclonedx.model.License expressionLicense = null;
-                    expressionLicense = new org.cyclonedx.model.License();
-                    expressionLicense.setId(parsedExpression.getSpdxLicenseId());
-                    expressionLicense.setName(parsedExpression.getSpdxLicenseId());
-                    licenseOptions.add(expressionLicense);
+                final var expressionParser = new SpdxExpressionParser();
+                final SpdxExpression parsedExpression = expressionParser.parse(licenseChoice.getExpression());
+                if (!Objects.equals(parsedExpression, SpdxExpression.INVALID)) {
+                    // store license expression, but don't overwrite manual changes to the field
+                    if (component.getLicenseExpression() == null) {
+                        component.setLicenseExpression(licenseChoice.getExpression());
+                    }
+                    // if the expression just consists of one license id, we can add it as another license option
+                    if (parsedExpression.getSpdxLicenseId() != null) {
+                        org.cyclonedx.model.License expressionLicense = new org.cyclonedx.model.License();
+                        expressionLicense.setId(parsedExpression.getSpdxLicenseId());
+                        licenseOptions.add(expressionLicense);
+                    }
+                } else {
+                    LOGGER.warn("""
+                            Encountered invalid license expression "%s" for \
+                            Component{group=%s, name=%s, version=%s, bomRef=%s}; Skipping\
+                            """.formatted(licenseChoice.getExpression(), component.getGroup(),
+                            component.getName(), component.getVersion(), component.getBomRef()));
                 }
             }
             // add license options from the component's license array. These will have higher priority

src/main/java/org/dependencytrack/parser/spdx/expression/SpdxExpressionParser.java

@@ -31,7 +31,7 @@
  * SpdxExpressions and SpdxExpressionOperations
  * 
  * @author hborchardt
- * @since 4.8.0
+ * @since 4.9.0
  */
 public class SpdxExpressionParser {
 

src/main/java/org/dependencytrack/parser/spdx/expression/model/SpdxExpression.java

@@ -25,7 +25,7 @@
  * inner node, containss an operation.
  * 
  * @author hborchardt
- * @since 4.8.0
+ * @since 4.9.0
  */
 public class SpdxExpression {
     public static final SpdxExpression INVALID = new SpdxExpression(null);

src/main/java/org/dependencytrack/parser/spdx/expression/model/SpdxExpressionOperation.java

@@ -26,7 +26,7 @@
  * to that operator.
  * 
  * @author hborchardt
- * @since 4.8.0
+ * @since 4.9.0
  */
 public class SpdxExpressionOperation {
     private SpdxOperator operator;

src/main/java/org/dependencytrack/parser/spdx/expression/model/SpdxOperator.java

@@ -22,7 +22,7 @@
  * One of the SPDX expression operators as defined in the spec, together with their precedence.
  * 
  * @author hborchardt
- * @since 4.8.0
+ * @since 4.9.0
  */
 public enum SpdxOperator {
     OR(1, "OR"), AND(2, "AND"), WITH(3, "WITH"), PLUS(4, "+");

src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java

@@ -371,6 +371,8 @@ public Component cloneComponent(Component sourceComponent, Project destinationPr
         component.setDescription(sourceComponent.getDescription());
         component.setCopyright(sourceComponent.getCopyright());
         component.setLicense(sourceComponent.getLicense());
+        component.setLicenseExpression(sourceComponent.getLicenseExpression());
+        component.setLicenseUrl(sourceComponent.getLicenseUrl());
         component.setResolvedLicense(sourceComponent.getResolvedLicense());
         component.setAuthor(sourceComponent.getAuthor());
         // TODO Add support for parent component and children components
@@ -399,6 +401,8 @@ public Component updateComponent(Component transientComponent, boolean commitInd
         component.setDescription(transientComponent.getDescription());
         component.setCopyright(transientComponent.getCopyright());
         component.setLicense(transientComponent.getLicense());
+        component.setLicenseExpression(transientComponent.getLicenseExpression());
+        component.setLicenseUrl(transientComponent.getLicenseUrl());
         component.setResolvedLicense(transientComponent.getResolvedLicense());
         component.setParent(transientComponent.getParent());
         component.setCpe(transientComponent.getCpe());

src/main/java/org/dependencytrack/resources/v1/ComponentResource.java

@@ -252,6 +252,8 @@ public Response createComponent(@PathParam("uuid") String uuid, Component jsonCo
                 validator.validateProperty(jsonComponent, "group"),
                 validator.validateProperty(jsonComponent, "description"),
                 validator.validateProperty(jsonComponent, "license"),
+                validator.validateProperty(jsonComponent, "licenseExpression"),
+                validator.validateProperty(jsonComponent, "licenseUrl"),
                 validator.validateProperty(jsonComponent, "filename"),
                 validator.validateProperty(jsonComponent, "classifier"),
                 validator.validateProperty(jsonComponent, "cpe"),
@@ -305,12 +307,20 @@ public Response createComponent(@PathParam("uuid") String uuid, Component jsonCo
             component.setSha3_512(StringUtils.trimToNull(jsonComponent.getSha3_512()));
             if (resolvedLicense != null) {
                 component.setLicense(null);
+                component.setLicenseExpression(null);
+                component.setLicenseUrl(StringUtils.trimToNull(jsonComponent.getLicenseUrl()));
                 component.setResolvedLicense(resolvedLicense);
-            } else {
-                component.setLicense(StringUtils.trimToNull(jsonComponent.getLicense()));
+            } else if (StringUtils.trimToNull(jsonComponent.getLicense()) != null) {
+                component.setLicense(StringUtils.trim(jsonComponent.getLicense()));
+                component.setLicenseExpression(null);
+                component.setLicenseUrl(StringUtils.trimToNull(jsonComponent.getLicenseUrl()));
+                component.setResolvedLicense(null);
+            } else if (StringUtils.trimToNull(jsonComponent.getLicenseExpression()) != null) {
+                component.setLicense(null);
+                component.setLicenseExpression(StringUtils.trim(jsonComponent.getLicenseExpression()));
+                component.setLicenseUrl(null);
                 component.setResolvedLicense(null);
             }
-            component.setLicenseUrl(StringUtils.trimToNull(jsonComponent.getLicenseUrl()));
             component.setParent(parent);
             component.setNotes(StringUtils.trimToNull(jsonComponent.getNotes()));
 
@@ -347,6 +357,7 @@ public Response updateComponent(Component jsonComponent) {
                 validator.validateProperty(jsonComponent, "group"),
                 validator.validateProperty(jsonComponent, "description"),
                 validator.validateProperty(jsonComponent, "license"),
+                validator.validateProperty(jsonComponent, "licenseExpression"),
                 validator.validateProperty(jsonComponent, "licenseUrl"),
                 validator.validateProperty(jsonComponent, "filename"),
                 validator.validateProperty(jsonComponent, "classifier"),
@@ -395,12 +406,25 @@ public Response updateComponent(Component jsonComponent) {
                 final License resolvedLicense = qm.getLicense(jsonComponent.getLicense());
                 if (resolvedLicense != null) {
                     component.setLicense(null);
+                    component.setLicenseExpression(null);
+                    component.setLicenseUrl(StringUtils.trimToNull(jsonComponent.getLicenseUrl()));
                     component.setResolvedLicense(resolvedLicense);
+                } else if (StringUtils.trimToNull(jsonComponent.getLicense()) != null) {
+                    component.setLicense(StringUtils.trim(jsonComponent.getLicense()));
+                    component.setLicenseExpression(null);
+                    component.setLicenseUrl(StringUtils.trimToNull(jsonComponent.getLicenseUrl()));
+                    component.setResolvedLicense(null);
+                } else if (StringUtils.trimToNull(jsonComponent.getLicenseExpression()) != null) {
+                    component.setLicense(null);
+                    component.setLicenseExpression(StringUtils.trim(jsonComponent.getLicenseExpression()));
+                    component.setLicenseUrl(null);
+                    component.setResolvedLicense(null);
                 } else {
-                    component.setLicense(StringUtils.trimToNull(jsonComponent.getLicense()));
+                    component.setLicense(null);
+                    component.setLicenseExpression(null);
+                    component.setLicenseUrl(null);
                     component.setResolvedLicense(null);
                 }
-                component.setLicenseUrl(StringUtils.trimToNull(jsonComponent.getLicenseUrl()));
                 component.setNotes(StringUtils.trimToNull(jsonComponent.getNotes()));
 
                 component = qm.updateComponent(component, true);

src/main/java/org/dependencytrack/upgrade/v490/v490Updater.java

@@ -21,7 +21,6 @@
 import alpine.common.logging.Logger;
 import alpine.persistence.AlpineQueryManager;
 import alpine.server.upgrade.AbstractUpgradeItem;
-import alpine.server.util.DbUtil;
 
 import java.sql.Connection;
 import java.sql.PreparedStatement;
@@ -40,7 +39,6 @@ public String getSchemaVersion() {
     @Override
     public void executeUpgrade(final AlpineQueryManager qm, final Connection connection) throws Exception {
         updateDefaultSnykApiVersion(connection);
-        addLicenseExpressionColumnToComponents(connection);
     }
 
     /**
@@ -64,16 +62,4 @@ private static void updateDefaultSnykApiVersion(final Connection connection) thr
         }
     }
 
-    private void addLicenseExpressionColumnToComponents(Connection connection) throws Exception {
-        // The JDBC type "CLOB" is mapped to the type CLOB for H2, MEDIUMTEXT for MySQL, and TEXT for PostgreSQL and SQL Server.
-        LOGGER.info("Adding \"LICENSE_EXPRESSION\" to \"COMPONENTS\"");
-        if (DbUtil.isH2()) {
-            DbUtil.executeUpdate(connection, "ALTER TABLE \"COMPONENTS\" ADD \"LICENSE_EXPRESSION\" CLOB");
-        } else if (DbUtil.isMysql()) {
-            DbUtil.executeUpdate(connection, "ALTER TABLE \"COMPONENTS\" ADD \"LICENSE_EXPRESSION\" MEDIUMTEXT");
-        } else {
-            DbUtil.executeUpdate(connection, "ALTER TABLE \"COMPONENTS\" ADD \"LICENSE_EXPRESSION\" TEXT");
-        }
-    }
-
 }

src/test/java/org/dependencytrack/PersistenceCapableTest.java

@@ -41,6 +41,14 @@ public void before() throws Exception {
 
     @After
     public void after() {
+        // PersistenceManager will refuse to close when there's an active transaction
+        // that was neither committed nor rolled back. Unfortunately some areas of the
+        // code base can leave such a broken state behind if they run into unexpected
+        // errors. See: https://github.com/DependencyTrack/dependency-track/issues/2677
+        if (qm.getPersistenceManager().currentTransaction().isActive()) {
+            qm.getPersistenceManager().currentTransaction().rollback();
+        }
+
         PersistenceManagerFactory.tearDown();
     }
 

src/test/java/org/dependencytrack/model/validation/SpdxExpressionValidatorTest.java

@@ -0,0 +1,63 @@
+/*
+ * This file is part of Dependency-Track.
+ *
+ * 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.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright (c) Steve Springett. All Rights Reserved.
+ */
+package org.dependencytrack.model.validation;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.validation.ConstraintViolation;
+import javax.validation.Validation;
+import javax.validation.Validator;
+import javax.validation.ValidatorFactory;
+import java.util.Set;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class SpdxExpressionValidatorTest {
+
+    private Validator validator;
+
+    private record TestRecord(@ValidSpdxExpression String expression) {
+    }
+
+    @Before
+    public void setUp() {
+        final ValidatorFactory validatorFactory = Validation.buildDefaultValidatorFactory();
+        validator = validatorFactory.getValidator();
+    }
+
+    @Test
+    public void testWithValidExpression() {
+        final Set<ConstraintViolation<TestRecord>> violations = validator.validate(new TestRecord("Apache-2.0 OR MIT"));
+        assertThat(violations).isEmpty();
+    }
+
+    @Test
+    public void testWithInvalidExpression() {
+        final Set<ConstraintViolation<TestRecord>> violations = validator.validate(new TestRecord("(Apache-2.0"));
+        assertThat(violations).isNotEmpty();
+    }
+
+    @Test
+    public void testWithNullExpression() {
+        final Set<ConstraintViolation<TestRecord>> violations = validator.validate(new TestRecord(null));
+        assertThat(violations).isEmpty();
+    }
+
+}