Switch completely to locked for checking if DS could be functionally modified (#2766)

Signed-off-by: Avgustin Marinov <Avgustin.Marinov@bosch.com>

hawkbit-repository/hawkbit-repository-api/src/main/java/org/eclipse/hawkbit/repository/exception/EntityReadOnlyException.java

@@ -28,18 +28,6 @@ public class EntityReadOnlyException extends AbstractServerRtException {
 
     private static final SpServerError THIS_ERROR = SpServerError.SP_REPO_ENTITY_READ_ONLY;
 
-    public EntityReadOnlyException() {
-        super(THIS_ERROR);
-    }
-
-    public EntityReadOnlyException(final Throwable cause) {
-        super(THIS_ERROR, cause);
-    }
-
-    public EntityReadOnlyException(final String message, final Throwable cause) {
-        super(THIS_ERROR, message, cause);
-    }
-
     public EntityReadOnlyException(final String message) {
         super(THIS_ERROR, message);
     }

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/AbstractJpaRepositoryManagement.java

@@ -194,7 +194,6 @@ abstract class AbstractJpaRepositoryManagement<T extends AbstractJpaBaseEntity,
     @SuppressWarnings("java:S1066") // javaS1066 - better readable that way
     public T update(final U update) {
         final T entity = getValid(update.getId());
-        checkUpdate(update, entity);
         // update getId has no setter in target JPA entity but shall have getter and the value shall be the same
         // otherwise the Utils will throw an exception that there is no counterpart setter for getId
         if (ObjectCopyUtil.copy(update, entity, false, this::attach)) {
@@ -222,7 +221,6 @@ abstract class AbstractJpaRepositoryManagement<T extends AbstractJpaBaseEntity,
         final List<T> toSave = new ArrayList<>(toUpdate.values());
         for (final U u : update) {
             final T entity = toUpdate.get(u.getId());
-            checkUpdate(u, entity);
             // update getId has no setter in target JPA entity but shall have getter and the value shall be the same
             // otherwise the Utils will throw an exception that there is no counterpart setter for getId
             if (ObjectCopyUtil.copy(u, entity, false, this::attach)) {
@@ -262,8 +260,6 @@ abstract class AbstractJpaRepositoryManagement<T extends AbstractJpaBaseEntity,
                 .orElseGet(() -> List.of(QLSupport.getInstance().buildSpec(rsql, fieldsClass())));
     }
 
-    protected void checkUpdate(final U update, final T distributionSet) {}
-
     // return which are for soft deletion
     @SuppressWarnings("java:S1172") // java:S1172 - it is intended to be used by subclasses
     protected Collection<T> softDelete(final Collection<T> toDelete) {

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaArtifactManagement.java

@@ -32,6 +32,7 @@ import org.eclipse.hawkbit.repository.exception.EntityAlreadyExistsException;
 import org.eclipse.hawkbit.repository.exception.InvalidMd5HashException;
 import org.eclipse.hawkbit.repository.exception.InvalidSha1HashException;
 import org.eclipse.hawkbit.repository.exception.InvalidSha256HashException;
+import org.eclipse.hawkbit.repository.exception.LockedException;
 import org.eclipse.hawkbit.repository.jpa.JpaManagementHelper;
 import org.eclipse.hawkbit.repository.jpa.acm.AccessController;
 import org.eclipse.hawkbit.repository.jpa.configuration.Constants;
@@ -109,6 +110,12 @@ public class JpaArtifactManagement implements ArtifactManagement {
                 softwareModuleId -> artifactRepository.count(null, ArtifactSpecifications.bySoftwareModuleId(softwareModuleId)));
 
         final JpaSoftwareModule softwareModule = softwareModuleRepository.getById(moduleId);
+        if (softwareModule.isLocked()) {
+            // check in order to:
+            // - on non-existing artifact - skip binary storing before, eventual, failing in new JpaSoftwareModule.addArtifact
+            // - if existing and overriding - no check will be made in new JpaSoftwareModule.addArtifact, so we sh to fail here
+            throw new LockedException(JpaSoftwareModule.class, softwareModule.getId(), "ADD_ARTIFACT");
+        }
 
         final String filename = artifactUpload.filename();
         final Artifact existing = softwareModule.getArtifactByFilename(filename).orElse(null);
@@ -243,12 +250,13 @@ public class JpaArtifactManagement implements ArtifactManagement {
     }
 
     private Artifact storeArtifactMetadata(
-            final SoftwareModule softwareModule, final String providedFilename,
+            final JpaSoftwareModule softwareModule, final String providedFilename,
             final ArtifactHashes hash, final long fileSize,
             final Artifact existing) {
         final JpaArtifact artifact;
         if (existing == null) {
             artifact = new JpaArtifact(hash.sha1(), providedFilename, softwareModule);
+            softwareModule.addArtifact(artifact);
         } else {
             artifact = (JpaArtifact) existing;
             artifact.setSha1Hash(hash.sha1());

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaDistributionSetManagement.java

@@ -36,7 +36,6 @@ import org.eclipse.hawkbit.repository.RepositoryProperties;
 import org.eclipse.hawkbit.repository.TenantConfigurationManagement;
 import org.eclipse.hawkbit.repository.exception.DeletedException;
 import org.eclipse.hawkbit.repository.exception.EntityNotFoundException;
-import org.eclipse.hawkbit.repository.exception.EntityReadOnlyException;
 import org.eclipse.hawkbit.repository.exception.IncompleteDistributionSetException;
 import org.eclipse.hawkbit.repository.exception.InvalidDistributionSetException;
 import org.eclipse.hawkbit.repository.exception.RSQLParameterSyntaxException;
@@ -47,7 +46,6 @@ import org.eclipse.hawkbit.repository.jpa.model.JpaDistributionSet_;
 import org.eclipse.hawkbit.repository.jpa.model.JpaSoftwareModule;
 import org.eclipse.hawkbit.repository.jpa.ql.Node;
 import org.eclipse.hawkbit.repository.jpa.ql.QLSupport;
-import org.eclipse.hawkbit.repository.jpa.repository.ActionRepository;
 import org.eclipse.hawkbit.repository.jpa.repository.DistributionSetRepository;
 import org.eclipse.hawkbit.repository.jpa.repository.DistributionSetTagRepository;
 import org.eclipse.hawkbit.repository.jpa.repository.SoftwareModuleRepository;
@@ -85,7 +83,6 @@ public class JpaDistributionSetManagement
     private final SoftwareModuleRepository softwareModuleRepository;
     private final DistributionSetTagRepository distributionSetTagRepository;
     private final TargetFilterQueryRepository targetFilterQueryRepository;
-    private final ActionRepository actionRepository;
     private final QuotaManagement quotaManagement;
     private final TenantConfigHelper tenantConfigHelper;
     private final RepositoryProperties repositoryProperties;
@@ -98,7 +95,6 @@ public class JpaDistributionSetManagement
             final SoftwareModuleRepository softwareModuleRepository,
             final DistributionSetTagRepository distributionSetTagRepository,
             final TargetFilterQueryRepository targetFilterQueryRepository,
-            final ActionRepository actionRepository,
             final QuotaManagement quotaManagement,
             final SystemSecurityContext systemSecurityContext,
             final TenantConfigurationManagement tenantConfigurationManagement,
@@ -108,7 +104,6 @@ public class JpaDistributionSetManagement
         this.softwareModuleRepository = softwareModuleRepository;
         this.distributionSetTagRepository = distributionSetTagRepository;
         this.targetFilterQueryRepository = targetFilterQueryRepository;
-        this.actionRepository = actionRepository;
         this.quotaManagement = quotaManagement;
         this.tenantConfigHelper = TenantConfigHelper.usingContext(systemSecurityContext, tenantConfigurationManagement);
         this.repositoryProperties = repositoryProperties;
@@ -175,13 +170,6 @@ public class JpaDistributionSetManagement
         return updated;
     }
 
-    @Override
-    protected void checkUpdate(final Update update, final JpaDistributionSet distributionSet) {
-        if (update.getRequiredMigrationStep() != null && !update.getRequiredMigrationStep().equals(distributionSet.isRequiredMigrationStep())) {
-            assertDistributionSetIsNotAssignedToTargets(update.getId());
-        }
-    }
-
     @Override
     protected Collection<JpaDistributionSet> softDelete(final Collection<JpaDistributionSet> toDelete) {
         // soft delete assigned
@@ -284,7 +272,6 @@ public class JpaDistributionSetManagement
     @Retryable(retryFor = { ConcurrencyFailureException.class }, maxAttempts = TX_RT_MAX, backoff = @Backoff(delay = TX_RT_DELAY))
     public JpaDistributionSet assignSoftwareModules(final long id, final Collection<Long> softwareModuleId) {
         final JpaDistributionSet set = getValid0(id);
-        assertDistributionSetIsNotAssignedToTargets(id);
         assertSoftwareModuleQuota(id, softwareModuleId.size());
 
         final Collection<JpaSoftwareModule> modules = softwareModuleRepository.findAllById(softwareModuleId);
@@ -303,7 +290,6 @@ public class JpaDistributionSetManagement
     @Retryable(retryFor = { ConcurrencyFailureException.class }, maxAttempts = TX_RT_MAX, backoff = @Backoff(delay = TX_RT_DELAY))
     public JpaDistributionSet unassignSoftwareModule(final long id, final long moduleId) {
         final JpaDistributionSet set = getValid0(id);
-        assertDistributionSetIsNotAssignedToTargets(id);
 
         final JpaSoftwareModule module = softwareModuleRepository.getById(moduleId);
         set.removeModule(module);
@@ -464,13 +450,6 @@ public class JpaDistributionSetManagement
                 SoftwareModule.class, DistributionSet.class, softwareModuleRepository::countByAssignedToId);
     }
 
-    private void assertDistributionSetIsNotAssignedToTargets(final Long distributionSet) {
-        if (actionRepository.countByDistributionSetId(distributionSet) > 0) {
-            throw new EntityReadOnlyException(String.format(
-                    "Distribution set %s is already assigned to targets and cannot be changed", distributionSet));
-        }
-    }
-
     private void lockSoftwareModules(final JpaDistributionSet distributionSet) {
         distributionSet.getModules().forEach(module -> {
             if (!module.isLocked()) {

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/model/JpaArtifact.java

@@ -61,7 +61,7 @@ public class JpaArtifact extends AbstractJpaTenantAwareBaseEntity implements Art
     @NotNull
     private String filename;
 
-    @Column(name = "md5_hash", length = 32, updatable = false, nullable = true)
+    @Column(name = "md5_hash", length = 32, updatable = false)
     private String md5Hash;
 
     @Column(name = "sha1_hash", length = 40, nullable = false, updatable = false)
@@ -69,7 +69,7 @@ public class JpaArtifact extends AbstractJpaTenantAwareBaseEntity implements Art
     @NotNull
     private String sha1Hash;
 
-    @Column(name = "sha256_hash", length = 64, updatable = false, nullable = true)
+    @Column(name = "sha256_hash", length = 64, updatable = false)
     private String sha256Hash;
 
     @Column(name = "file_size", updatable = false)
@@ -79,7 +79,6 @@ public class JpaArtifact extends AbstractJpaTenantAwareBaseEntity implements Art
         this.sha1Hash = sha1Hash;
         this.filename = filename;
         this.softwareModule = (JpaSoftwareModule) softwareModule;
-        this.softwareModule.addArtifact(this);
     }
 
     @Override

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/model/JpaDistributionSet.java

@@ -134,7 +134,6 @@ public class JpaDistributionSet
     @Column(name = "valid")
     private boolean valid;
 
-    @Setter
     @Column(name = "required_migration_step")
     private boolean requiredMigrationStep;
 
@@ -159,6 +158,17 @@ public class JpaDistributionSet
         return this;
     }
 
+    @SuppressWarnings("java:S1144") // used via reflection copy utils
+    private void setRequiredMigrationStep(final boolean requiredMigrationStep) {
+        if (this.requiredMigrationStep != requiredMigrationStep) {
+            if (isLocked()) {
+                throw new LockedException(JpaDistributionSet.class, getId(), "CHANGE_REQUIRED_MIGRATION_STEP");
+            }
+
+            this.requiredMigrationStep = requiredMigrationStep;
+        }
+    }
+
     @Override
     public boolean isComplete() {
         return Optional.ofNullable(type).map(dsType -> {

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/model/JpaSoftwareModule.java

@@ -79,7 +79,6 @@ public class JpaSoftwareModule
     @Serial
     private static final long serialVersionUID = 1L;
 
-    @Setter
     @ManyToOne
     @JoinColumn(name = "sm_type", nullable = false, updatable = false,
             foreignKey = @ForeignKey(value = ConstraintMode.CONSTRAINT, name = "fk_software_module_sm_type"))
@@ -91,17 +90,14 @@ public class JpaSoftwareModule
             targetEntity = JpaArtifact.class, orphanRemoval = true)
     private List<JpaArtifact> artifacts;
 
-    @Setter
     @Column(name = "vendor", length = SoftwareModule.VENDOR_MAX_SIZE)
     @Size(max = SoftwareModule.VENDOR_MAX_SIZE)
     private String vendor;
 
-    @Setter
     @Column(name = "encrypted")
     private boolean encrypted;
 
     // no cascade option on an ElementCollection, the target objects are always persisted, merged, removed with their parent.
-    @Getter
     @ElementCollection
     @CollectionTable(
             name = "sp_sm_metadata",
@@ -130,23 +126,6 @@ public class JpaSoftwareModule
         return artifacts == null ? Collections.emptyList() : Collections.unmodifiableList(artifacts);
     }
 
-    public void addArtifact(final Artifact artifact) {
-        if (isLocked()) {
-            throw new LockedException(JpaSoftwareModule.class, getId(), "ADD_ARTIFACT");
-        }
-
-        if (artifact instanceof JpaArtifact jpaArtifact) {
-            if (artifacts == null) {
-                artifacts = new ArrayList<>(4);
-                artifacts.add(jpaArtifact);
-            } else if (!artifacts.contains(jpaArtifact)) {
-                artifacts.add(jpaArtifact);
-            }
-        } else {
-            throw new UnsupportedOperationException("Only JpaArtifact is supported");
-        }
-    }
-
     @Override
     public boolean isComplete() {
         return Optional.ofNullable(type).map(smType -> {
@@ -158,6 +137,18 @@ public class JpaSoftwareModule
         }).orElse(true);
     }
 
+    public void addArtifact(final JpaArtifact artifact) {
+        if (isLocked()) {
+            throw new LockedException(JpaSoftwareModule.class, getId(), "ADD_ARTIFACT");
+        }
+        if (artifacts == null) {
+            artifacts = new ArrayList<>(4);
+            artifacts.add(artifact);
+        } else if (!artifacts.contains(artifact)) {
+            artifacts.add(artifact);
+        }
+    }
+
     public void removeArtifact(final Artifact artifact) {
         if (isLocked()) {
             throw new LockedException(JpaSoftwareModule.class, getId(), "REMOVE_ARTIFACT");

hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/DistributionSetManagementTest.java

@@ -43,7 +43,6 @@ import org.eclipse.hawkbit.repository.event.remote.entity.SoftwareModuleCreatedE
 import org.eclipse.hawkbit.repository.exception.AssignmentQuotaExceededException;
 import org.eclipse.hawkbit.repository.exception.EntityAlreadyExistsException;
 import org.eclipse.hawkbit.repository.exception.EntityNotFoundException;
-import org.eclipse.hawkbit.repository.exception.EntityReadOnlyException;
 import org.eclipse.hawkbit.repository.exception.IncompleteDistributionSetException;
 import org.eclipse.hawkbit.repository.exception.InvalidDistributionSetException;
 import org.eclipse.hawkbit.repository.exception.LockedException;
@@ -288,13 +287,11 @@ class DistributionSetManagementTest extends AbstractRepositoryManagementWithMeta
 
         final Long dsId = ds.getId();
         // not allowed as it is assigned now
-        assertThatThrownBy(() -> distributionSetManagement.assignSoftwareModules(dsId, os2Id))
-                .isInstanceOf(EntityReadOnlyException.class);
+        assertThatThrownBy(() -> distributionSetManagement.assignSoftwareModules(dsId, os2Id)).isInstanceOf(LockedException.class);
 
         // not allowed as it is assigned now
         final Long appId = findFirstModuleByType(ds, appType).map(Identifiable::getId).orElseThrow();
-        assertThatThrownBy(() -> distributionSetManagement.unassignSoftwareModule(dsId, appId))
-                .isInstanceOf(EntityReadOnlyException.class);
+        assertThatThrownBy(() -> distributionSetManagement.unassignSoftwareModule(dsId, appId)).isInstanceOf(LockedException.class);
     }
 
     /**