Skip to content

Commit

Permalink
Sonar Fixes (8) (#2220)
Browse files Browse the repository at this point in the history 
Signed-off-by: Avgustin Marinov <Avgustin.Marinov@bosch.com>
  • Loading branch information
@avgustinmm
avgustinmm authored Jan 23, 2025
1 parent bb9c9bf commit 21b901a
Show file tree
Hide file tree
Showing 7 changed files with 171 additions and 141 deletions.
21 changes: 11 additions & 10 deletions ...-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaActionManagement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ protected Action addActionStatus(final JpaActionStatusCreate statusCreate) {
}

log.debug(
"Update of actionStatus {} for action {} not possible since action not active anymore.",
"Update of actionStatus {} for action {} not possible since action not active anymore and not allowed as an action terminating.",
actionStatus.getStatus(), action.getId());
return action;
}
Expand Down Expand Up @@ -133,16 +133,17 @@ private static boolean isIntermediateStatus(final JpaActionStatus actionStatus)
* status updates only once.
*/
private boolean isUpdatingActionStatusAllowed(final JpaAction action, final JpaActionStatus actionStatus) {
if (action.isActive()) {
return true;
}

final boolean intermediateStatus = isIntermediateStatus(actionStatus);

final boolean isAllowedByRepositoryConfiguration = intermediateStatus && !repositoryProperties.isRejectActionStatusForClosedAction();

//in case of download_only action Status#DOWNLOADED is treated as 'final' already, so we accept one final status from device in case it sends
final boolean isAllowedForDownloadOnlyActions = isDownloadOnly(
action) && action.getStatus() == Action.Status.DOWNLOADED && !intermediateStatus;

return action.isActive() || isAllowedByRepositoryConfiguration || isAllowedForDownloadOnlyActions;
if (isIntermediateStatus(actionStatus)) {
return !repositoryProperties.isRejectActionStatusForClosedAction();
} else {
// in case of download_only action Status#DOWNLOADED is treated as 'final' already,
// so we accept one final status from device in case it sends
return isDownloadOnly(action) && action.getStatus() == Action.Status.DOWNLOADED;
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ...sitory-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/repository/ACMRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public interface ACMRepository<T> {
* @return matching entity
*/
@NonNull
Optional<T> findOne(@Nullable AccessController.Operation operation, @Nullable Specification<T> spec);
Optional<T> findOne(@Nullable AccessController.Operation operation, @NonNull Specification<T> spec);

/**
* Returns all entries that match specification and the operation is allowed for.
Expand Down
29 changes: 24 additions & 5 deletions .../src/main/java/org/eclipse/hawkbit/repository/jpa/repository/BaseEntityRepositoryACM.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.util.Set;
import java.util.function.Function;

import jakarta.persistence.EntityManager;
import jakarta.transaction.Transactional;

import lombok.extern.slf4j.Slf4j;
Expand All @@ -39,6 +38,10 @@
@Slf4j
public class BaseEntityRepositoryACM<T extends AbstractJpaTenantAwareBaseEntity> implements BaseEntityRepository<T> {

private static final String SPEC_MUST_NOT_BE_NULL = "Specification must not be null";
private static final String APPENDED_ACCESS_RULES_SPEC_OF_NON_NULL_SPEC_MUST_NOT_BE_NULL =
"Appended access rules specification of non-null specification must not be null";

private final BaseEntityRepository<T> repository;
private final AccessController<T> accessController;

Expand Down Expand Up @@ -147,7 +150,12 @@ public Optional<AccessController<T>> getAccessController() {
@Override
@NonNull
public Optional<T> findOne(final Specification<T> spec) {
return repository.findOne(accessController.appendAccessRules(AccessController.Operation.READ, spec));
Objects.requireNonNull(spec, SPEC_MUST_NOT_BE_NULL);
return repository.findOne(
// spec shall be non-null and the result of appending rules shall be non-null
Objects.requireNonNull(
accessController.appendAccessRules(AccessController.Operation.READ, spec),
APPENDED_ACCESS_RULES_SPEC_OF_NON_NULL_SPEC_MUST_NOT_BE_NULL));
}

@Override
Expand Down Expand Up @@ -186,7 +194,13 @@ public long delete(final Specification<T> spec) {

@Override
public <S extends T, R> R findBy(final Specification<T> spec, final Function<FluentQuery.FetchableFluentQuery<S>, R> queryFunction) {
return repository.findBy(accessController.appendAccessRules(AccessController.Operation.READ, spec), queryFunction);
Objects.requireNonNull(spec, SPEC_MUST_NOT_BE_NULL);
return repository.findBy(
// spec shall be non-null and the result of appending rules shall be non-null
Objects.requireNonNull(
accessController.appendAccessRules(AccessController.Operation.READ, spec),
APPENDED_ACCESS_RULES_SPEC_OF_NON_NULL_SPEC_MUST_NOT_BE_NULL),
queryFunction);
}

@Override
Expand Down Expand Up @@ -232,11 +246,16 @@ public <S extends T> List<S> saveAll(@Nullable AccessController.Operation operat
}

@NonNull
public Optional<T> findOne(@Nullable AccessController.Operation operation, @Nullable Specification<T> spec) {
public Optional<T> findOne(@Nullable AccessController.Operation operation, @NonNull Specification<T> spec) {
Objects.requireNonNull(spec, SPEC_MUST_NOT_BE_NULL);
if (operation == null) {
return repository.findOne(spec);
} else {
return repository.findOne(accessController.appendAccessRules(operation, spec));
return repository.findOne(
// spec shall be non-null and the result of appending rules shall be non-null
Objects.requireNonNull(
accessController.appendAccessRules(operation, spec),
APPENDED_ACCESS_RULES_SPEC_OF_NON_NULL_SPEC_MUST_NOT_BE_NULL));
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...pa/src/main/java/org/eclipse/hawkbit/repository/jpa/repository/HawkbitBaseRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public <S extends T> List<S> saveAll(@Nullable AccessController.Operation operat
}

@NonNull
public Optional<T> findOne(@Nullable AccessController.Operation operation, @Nullable Specification<T> spec) {
public Optional<T> findOne(@Nullable AccessController.Operation operation, @NonNull Specification<T> spec) {
return findOne(spec);
}

Expand Down
6 changes: 3 additions & 3 deletions ...st/java/org/eclipse/hawkbit/repository/jpa/ConcurrentDistributionSetInvalidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,11 @@ void verifyInvalidateDistributionSetWithLargeRolloutThrowsException() {
.until(() -> tenantAware.runAsTenant(tenant, () -> systemSecurityContext
.runAsSystem(() -> rolloutGroupManagement.findByRollout(rollout.getId(), PAGE).getSize() > 0)));

final DistributionSetInvalidation distributionSetInvalidation = new DistributionSetInvalidation(
Collections.singletonList(distributionSet.getId()), CancelationType.SOFT, true);
assertThatExceptionOfType(StopRolloutException.class)
.as("Invalidation of distributionSet should throw an exception")
.isThrownBy(() -> distributionSetInvalidationManagement.invalidateDistributionSet(
new DistributionSetInvalidation(Collections.singletonList(distributionSet.getId()),
CancelationType.SOFT, true)));
.isThrownBy(() -> distributionSetInvalidationManagement.invalidateDistributionSet(distributionSetInvalidation));
}

private Rollout createRollout(final DistributionSet distributionSet) {
Expand Down
10 changes: 5 additions & 5 deletions ...c/test/java/org/eclipse/hawkbit/repository/jpa/management/ConfirmationManagementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,12 @@ void confirmedActionCannotBeConfirmedAgain() {

final List<Action> actions = assignDistributionSet(dsId, controllerId).getAssignedEntity();
assertThat(actions).hasSize(1).allMatch(action -> action.getStatus() == Status.WAIT_FOR_CONFIRMATION);
final Action newAction = confirmationManagement.confirmAction(actions.get(0).getId(), null, null);
final Long actionId = actions.get(0).getId();
final Action newAction = confirmationManagement.confirmAction(actionId, null, null);
// verify action in RUNNING state
assertThat(newAction.getStatus()).isEqualTo(Status.RUNNING);

assertThatThrownBy(() -> confirmationManagement.confirmAction(actions.get(0).getId(), null, null))
assertThatThrownBy(() -> confirmationManagement.confirmAction(actionId, null, null))
.isInstanceOf(InvalidConfirmationFeedbackException.class)
.matches(e -> ((InvalidConfirmationFeedbackException) e)
.getReason() == InvalidConfirmationFeedbackException.Reason.NOT_AWAITING_CONFIRMATION);
Expand All @@ -146,9 +147,8 @@ void confirmedActionCannotBeConfirmedAgain() {
@Description("Verify confirming a closed action will lead to a specific failure")
void confirmedActionCannotBeGivenOnFinishedAction() {
enableConfirmationFlow();
final Action action = prepareFinishedUpdate();

assertThatThrownBy(() -> confirmationManagement.confirmAction(action.getId(), null, null))
final Long actionId = prepareFinishedUpdate().getId();
assertThatThrownBy(() -> confirmationManagement.confirmAction(actionId, null, null))
.isInstanceOf(InvalidConfirmationFeedbackException.class)
.matches(e -> ((InvalidConfirmationFeedbackException) e)
.getReason() == InvalidConfirmationFeedbackException.Reason.ACTION_CLOSED);
Expand Down
Loading

0 comments on commit 21b901a

Please sign in to comment.