From 9b71ab864481d417f3bc9cf4d0733b674aff5b9b Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Fri, 5 Jul 2024 14:28:12 +0200 Subject: [PATCH 01/22] SPAW-942 implementation draft --- .../api/OfflineRetransmissionRequest.java | 23 +++++++++++------ .../hermes/api/OfflineRetransmissionTask.java | 4 +++ .../constraints/OneSourceRetransmission.java | 22 ++++++++++++++++ .../OneSourceRetransmissionValidator.java | 25 +++++++++++++++++++ .../api/OfflineRetransmissionEndpoint.java | 13 ++++++---- .../OfflineRetransmissionManagementTest.java | 25 +++++++++++-------- 6 files changed, 89 insertions(+), 23 deletions(-) create mode 100644 hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java create mode 100644 hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java index e21fcc2d31..948ef55502 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java @@ -7,6 +7,7 @@ import jakarta.validation.constraints.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import pl.allegro.tech.hermes.api.constraints.OneSourceRetransmission; import pl.allegro.tech.hermes.api.jackson.InstantIsoSerializer; import java.time.Instant; @@ -15,6 +16,7 @@ import java.time.format.DateTimeParseException; import java.util.List; +@OneSourceRetransmission public class OfflineRetransmissionRequest { private static final List formatters = List.of( @@ -24,7 +26,7 @@ public class OfflineRetransmissionRequest { ); private static final Logger logger = LoggerFactory.getLogger(OfflineRetransmissionRequest.class); - @NotEmpty + private final String sourceView; private final String sourceTopic; @NotEmpty private final String targetTopic; @@ -35,10 +37,12 @@ public class OfflineRetransmissionRequest { @JsonCreator public OfflineRetransmissionRequest( + @JsonProperty("sourceView") String sourceView, @JsonProperty("sourceTopic") String sourceTopic, @JsonProperty("targetTopic") String targetTopic, @JsonProperty("startTimestamp") String startTimestamp, @JsonProperty("endTimestamp") String endTimestamp) { + this.sourceView = sourceView; this.sourceTopic = sourceTopic; this.targetTopic = targetTopic; this.startTimestamp = initializeTimestamp(startTimestamp); @@ -62,6 +66,10 @@ private Instant initializeTimestamp(String timestamp) { return null; } + public String getSourceView() { + return sourceView; + } + public String getSourceTopic() { return sourceTopic; } @@ -82,11 +90,12 @@ public Instant getEndTimestamp() { @Override public String toString() { - return "OfflineRetransmissionRequest{" - + "sourceTopic='" + sourceTopic + '\'' - + ", targetTopic='" + targetTopic + '\'' - + ", startTimestamp=" + startTimestamp - + ", endTimestamp=" + endTimestamp - + '}'; + return "OfflineRetransmissionRequest{" + + "sourceView='" + sourceView + '\'' + + ", sourceTopic='" + sourceTopic + '\'' + + ", targetTopic='" + targetTopic + '\'' + + ", startTimestamp=" + startTimestamp + + ", endTimestamp=" + endTimestamp + + '}'; } } diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java index 9f7d68d0a3..04902819d4 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java @@ -16,12 +16,14 @@ public class OfflineRetransmissionTask { @JsonCreator public OfflineRetransmissionTask( @JsonProperty("taskId") String taskId, + @JsonProperty("sourceView") String sourceView, @JsonProperty("sourceTopic") String sourceTopic, @JsonProperty("targetTopic") String targetTopic, @JsonProperty("startTimestamp") Instant startTimestamp, @JsonProperty("endTimestamp") Instant endTimestamp, @JsonProperty("createdAt") Instant createdAt) { this(taskId, new OfflineRetransmissionRequest( + sourceView, sourceTopic, targetTopic, startTimestamp.toString(), @@ -43,6 +45,8 @@ public String getSourceTopic() { return request.getSourceTopic(); } + public String getSourceView() {return request.getSourceView();} + public String getTargetTopic() { return request.getTargetTopic(); } diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java new file mode 100644 index 0000000000..52625ed94f --- /dev/null +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java @@ -0,0 +1,22 @@ +package pl.allegro.tech.hermes.api.constraints; + +import jakarta.validation.Constraint; + +import java.lang.annotation.Documented; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.TYPE; + +@Documented +@Retention(RetentionPolicy.RUNTIME) +@Target({TYPE}) +@Constraint(validatedBy = OneSourceRetransmissionValidator.class) +public @interface OneSourceRetransmission { + String message() default "Only one source of retransmission data is allowed - source topic or source view"; + + Class[] groups() default {}; + + Class[] payload() default {}; +} diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java new file mode 100644 index 0000000000..f91d85a047 --- /dev/null +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -0,0 +1,25 @@ +package pl.allegro.tech.hermes.api.constraints; + +import jakarta.validation.ConstraintValidator; +import jakarta.validation.ConstraintValidatorContext; +import pl.allegro.tech.hermes.api.OfflineRetransmissionRequest; + +public class OneSourceRetransmissionValidator implements ConstraintValidator { + + @Override + public void initialize(OneSourceRetransmission oneSourceRetransmission) { + + } + + @Override + public boolean isValid(OfflineRetransmissionRequest offlineRetransmissionRequest, ConstraintValidatorContext context) { + var sourceView = offlineRetransmissionRequest.getSourceView(); + var sourceTopic = offlineRetransmissionRequest.getSourceTopic(); + + return !(sourceView == null || sourceTopic == null) || !(nonBlank(sourceView) || nonBlank(sourceTopic)); + } + + private static boolean nonBlank(String sourceView) { + return sourceView != null && !sourceView.isBlank(); + } +} diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index 23662d469d..e1d09e297e 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -17,7 +17,6 @@ import org.springframework.stereotype.Component; import pl.allegro.tech.hermes.api.OfflineRetransmissionRequest; import pl.allegro.tech.hermes.api.OfflineRetransmissionTask; -import pl.allegro.tech.hermes.api.Topic; import pl.allegro.tech.hermes.api.TopicName; import pl.allegro.tech.hermes.domain.topic.TopicRepository; import pl.allegro.tech.hermes.management.api.auth.ManagementRights; @@ -46,7 +45,7 @@ public OfflineRetransmissionEndpoint(OfflineRetransmissionService retransmission @POST @Consumes(APPLICATION_JSON) - public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { + public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { retransmissionService.validateRequest(request); permissions.ensurePermissionsToBothTopics(request, requestContext); OfflineRetransmissionTask task = retransmissionService.createTask(request); @@ -78,14 +77,18 @@ private RetransmissionPermissions(TopicRepository topicRepository, ManagementRig } private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, ContainerRequestContext requestContext) { - Topic sourceTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getSourceTopic())); - Topic targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); - boolean hasPermissions = managementRights.isUserAllowedToManageTopic(sourceTopic, requestContext) + var targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); + boolean hasPermissions = validateSourceTopic(request.getSourceTopic(), requestContext) && managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); if (!hasPermissions) { throw new PermissionDeniedException("User needs permissions to source and target topics."); } } + + private boolean validateSourceTopic(String sourceTopic, ContainerRequestContext requestContext) { + var topic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(sourceTopic)); + return topic == null || managementRights.isUserAllowedToManageTopic(topic, requestContext); + } } private static class OfflineRetransmissionAuditor { diff --git a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java index 27847ad2e6..ee44ead5e2 100644 --- a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java +++ b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java @@ -45,7 +45,7 @@ public void shouldCreateRetransmissionTask() { // when OfflineRetransmissionRequest request = createRequest( sourceTopic.getQualifiedName(), - targetTopic.getQualifiedName()); + targetTopic.getQualifiedName(), null); WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); Instant now = Instant.now(); @@ -73,6 +73,7 @@ public void shouldReturnEmptyListIfThereAreNoTasks() { public void shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData() { // given OfflineRetransmissionRequest request = new OfflineRetransmissionRequest( + "", "", "", null, @@ -86,10 +87,10 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData() { response.expectStatus().isBadRequest(); assertThat(response.expectBody(String.class).returnResult().getResponseBody()).contains( List.of( - "sourceTopic must not be empty", - "targetTopic must not be empty", - "startTimestamp must not be null", - "endTimestamp must not be null") + "sourceTopic must not be empty", + "targetTopic must not be empty", + "startTimestamp must not be null", + "endTimestamp must not be null") ); } @@ -98,7 +99,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNotExistingSo // given Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = createRequest("not.existing.sourceTopic", - targetTopic.getQualifiedName()); + targetTopic.getQualifiedName(), null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -114,7 +115,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNotExistingTa // given Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), "not.existing.targetTopic"); + sourceTopic.getQualifiedName(), "not.existing.targetTopic", null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -131,6 +132,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNegativeTimeR Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = new OfflineRetransmissionRequest( + "", sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), Instant.now().toString(), @@ -151,7 +153,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithTargetTopicSt Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().withOfflineStorage(1).build()); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), targetTopic.getQualifiedName()); + sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -170,7 +172,7 @@ public void shouldDeleteRetransmissionTask() { Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), targetTopic.getQualifiedName()); + sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), null); hermes.api().createOfflineRetransmissionTask(request); List allTasks = getOfflineRetransmissionTasks(); @@ -203,7 +205,7 @@ public void shouldThrowAccessDeniedWhenTryingToCreateTaskWithoutPermissionsToSou Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); TestSecurityProvider.setUserIsAdmin(false); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), targetTopic.getQualifiedName()); + sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -218,8 +220,9 @@ public void shouldThrowAccessDeniedWhenTryingToCreateTaskWithoutPermissionsToSou TestSecurityProvider.reset(); } - private OfflineRetransmissionRequest createRequest(String sourceTopic, String targetTopic) { + private OfflineRetransmissionRequest createRequest(String sourceTopic, String targetTopic, String sourceView) { return new OfflineRetransmissionRequest( + sourceView, sourceTopic, targetTopic, Instant.now().minusSeconds(1).toString(), From 6dcb8477400b1275c844c254d634fff7742677eb Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Tue, 9 Jul 2024 14:29:39 +0200 Subject: [PATCH 02/22] SPAW-942 test coverage & cleanup --- .../OneSourceRetransmissionValidator.java | 8 ++-- ...neSourceRetransmissionValidatorTest.groovy | 37 +++++++++++++++++++ .../api/OfflineRetransmissionEndpoint.java | 12 +++--- .../OfflineRetransmissionService.java | 7 ++-- .../OfflineRetransmissionManagementTest.java | 24 ++++++++++++ 5 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 hermes-api/src/test/groovy/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidatorTest.groovy diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java index f91d85a047..2271503240 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -8,7 +8,6 @@ public class OneSourceRetransmissionValidator implements ConstraintValidator Date: Thu, 11 Jul 2024 15:30:03 +0200 Subject: [PATCH 03/22] SPAW-942 test coverage & cleanup --- .../subscription/validator/InflightSizeValidatorTest.groovy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy index 74c534654e..be13fa4402 100644 --- a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy +++ b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy @@ -72,7 +72,8 @@ class InflightSizeValidatorTest extends Specification { then: def exception = thrown(ConstraintViolationException) - exception.message == "serialSubscriptionPolicy.inflightSize: must be greater than or equal to 1" + exception.message == "serialSubscriptionPolicy.inflightSize: must be greater than or equal to 1" || + "serialSubscriptionPolicy.inflightSize: musi być równe lub większe od 1" where: inflightSize << [0, -1] From cac7b3dc6a79aabec7df932b09be4902b94ec8bc Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Fri, 5 Jul 2024 14:28:12 +0200 Subject: [PATCH 04/22] commit msg adjustment --- .../api/OfflineRetransmissionRequest.java | 23 +++++++++++------ .../hermes/api/OfflineRetransmissionTask.java | 4 +++ .../constraints/OneSourceRetransmission.java | 22 ++++++++++++++++ .../OneSourceRetransmissionValidator.java | 25 +++++++++++++++++++ .../api/OfflineRetransmissionEndpoint.java | 13 ++++++---- .../OfflineRetransmissionManagementTest.java | 25 +++++++++++-------- 6 files changed, 89 insertions(+), 23 deletions(-) create mode 100644 hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java create mode 100644 hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java index e21fcc2d31..948ef55502 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java @@ -7,6 +7,7 @@ import jakarta.validation.constraints.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import pl.allegro.tech.hermes.api.constraints.OneSourceRetransmission; import pl.allegro.tech.hermes.api.jackson.InstantIsoSerializer; import java.time.Instant; @@ -15,6 +16,7 @@ import java.time.format.DateTimeParseException; import java.util.List; +@OneSourceRetransmission public class OfflineRetransmissionRequest { private static final List formatters = List.of( @@ -24,7 +26,7 @@ public class OfflineRetransmissionRequest { ); private static final Logger logger = LoggerFactory.getLogger(OfflineRetransmissionRequest.class); - @NotEmpty + private final String sourceView; private final String sourceTopic; @NotEmpty private final String targetTopic; @@ -35,10 +37,12 @@ public class OfflineRetransmissionRequest { @JsonCreator public OfflineRetransmissionRequest( + @JsonProperty("sourceView") String sourceView, @JsonProperty("sourceTopic") String sourceTopic, @JsonProperty("targetTopic") String targetTopic, @JsonProperty("startTimestamp") String startTimestamp, @JsonProperty("endTimestamp") String endTimestamp) { + this.sourceView = sourceView; this.sourceTopic = sourceTopic; this.targetTopic = targetTopic; this.startTimestamp = initializeTimestamp(startTimestamp); @@ -62,6 +66,10 @@ private Instant initializeTimestamp(String timestamp) { return null; } + public String getSourceView() { + return sourceView; + } + public String getSourceTopic() { return sourceTopic; } @@ -82,11 +90,12 @@ public Instant getEndTimestamp() { @Override public String toString() { - return "OfflineRetransmissionRequest{" - + "sourceTopic='" + sourceTopic + '\'' - + ", targetTopic='" + targetTopic + '\'' - + ", startTimestamp=" + startTimestamp - + ", endTimestamp=" + endTimestamp - + '}'; + return "OfflineRetransmissionRequest{" + + "sourceView='" + sourceView + '\'' + + ", sourceTopic='" + sourceTopic + '\'' + + ", targetTopic='" + targetTopic + '\'' + + ", startTimestamp=" + startTimestamp + + ", endTimestamp=" + endTimestamp + + '}'; } } diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java index 9f7d68d0a3..04902819d4 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java @@ -16,12 +16,14 @@ public class OfflineRetransmissionTask { @JsonCreator public OfflineRetransmissionTask( @JsonProperty("taskId") String taskId, + @JsonProperty("sourceView") String sourceView, @JsonProperty("sourceTopic") String sourceTopic, @JsonProperty("targetTopic") String targetTopic, @JsonProperty("startTimestamp") Instant startTimestamp, @JsonProperty("endTimestamp") Instant endTimestamp, @JsonProperty("createdAt") Instant createdAt) { this(taskId, new OfflineRetransmissionRequest( + sourceView, sourceTopic, targetTopic, startTimestamp.toString(), @@ -43,6 +45,8 @@ public String getSourceTopic() { return request.getSourceTopic(); } + public String getSourceView() {return request.getSourceView();} + public String getTargetTopic() { return request.getTargetTopic(); } diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java new file mode 100644 index 0000000000..52625ed94f --- /dev/null +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java @@ -0,0 +1,22 @@ +package pl.allegro.tech.hermes.api.constraints; + +import jakarta.validation.Constraint; + +import java.lang.annotation.Documented; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.TYPE; + +@Documented +@Retention(RetentionPolicy.RUNTIME) +@Target({TYPE}) +@Constraint(validatedBy = OneSourceRetransmissionValidator.class) +public @interface OneSourceRetransmission { + String message() default "Only one source of retransmission data is allowed - source topic or source view"; + + Class[] groups() default {}; + + Class[] payload() default {}; +} diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java new file mode 100644 index 0000000000..f91d85a047 --- /dev/null +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -0,0 +1,25 @@ +package pl.allegro.tech.hermes.api.constraints; + +import jakarta.validation.ConstraintValidator; +import jakarta.validation.ConstraintValidatorContext; +import pl.allegro.tech.hermes.api.OfflineRetransmissionRequest; + +public class OneSourceRetransmissionValidator implements ConstraintValidator { + + @Override + public void initialize(OneSourceRetransmission oneSourceRetransmission) { + + } + + @Override + public boolean isValid(OfflineRetransmissionRequest offlineRetransmissionRequest, ConstraintValidatorContext context) { + var sourceView = offlineRetransmissionRequest.getSourceView(); + var sourceTopic = offlineRetransmissionRequest.getSourceTopic(); + + return !(sourceView == null || sourceTopic == null) || !(nonBlank(sourceView) || nonBlank(sourceTopic)); + } + + private static boolean nonBlank(String sourceView) { + return sourceView != null && !sourceView.isBlank(); + } +} diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index 23662d469d..e1d09e297e 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -17,7 +17,6 @@ import org.springframework.stereotype.Component; import pl.allegro.tech.hermes.api.OfflineRetransmissionRequest; import pl.allegro.tech.hermes.api.OfflineRetransmissionTask; -import pl.allegro.tech.hermes.api.Topic; import pl.allegro.tech.hermes.api.TopicName; import pl.allegro.tech.hermes.domain.topic.TopicRepository; import pl.allegro.tech.hermes.management.api.auth.ManagementRights; @@ -46,7 +45,7 @@ public OfflineRetransmissionEndpoint(OfflineRetransmissionService retransmission @POST @Consumes(APPLICATION_JSON) - public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { + public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { retransmissionService.validateRequest(request); permissions.ensurePermissionsToBothTopics(request, requestContext); OfflineRetransmissionTask task = retransmissionService.createTask(request); @@ -78,14 +77,18 @@ private RetransmissionPermissions(TopicRepository topicRepository, ManagementRig } private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, ContainerRequestContext requestContext) { - Topic sourceTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getSourceTopic())); - Topic targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); - boolean hasPermissions = managementRights.isUserAllowedToManageTopic(sourceTopic, requestContext) + var targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); + boolean hasPermissions = validateSourceTopic(request.getSourceTopic(), requestContext) && managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); if (!hasPermissions) { throw new PermissionDeniedException("User needs permissions to source and target topics."); } } + + private boolean validateSourceTopic(String sourceTopic, ContainerRequestContext requestContext) { + var topic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(sourceTopic)); + return topic == null || managementRights.isUserAllowedToManageTopic(topic, requestContext); + } } private static class OfflineRetransmissionAuditor { diff --git a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java index 27847ad2e6..ee44ead5e2 100644 --- a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java +++ b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java @@ -45,7 +45,7 @@ public void shouldCreateRetransmissionTask() { // when OfflineRetransmissionRequest request = createRequest( sourceTopic.getQualifiedName(), - targetTopic.getQualifiedName()); + targetTopic.getQualifiedName(), null); WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); Instant now = Instant.now(); @@ -73,6 +73,7 @@ public void shouldReturnEmptyListIfThereAreNoTasks() { public void shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData() { // given OfflineRetransmissionRequest request = new OfflineRetransmissionRequest( + "", "", "", null, @@ -86,10 +87,10 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData() { response.expectStatus().isBadRequest(); assertThat(response.expectBody(String.class).returnResult().getResponseBody()).contains( List.of( - "sourceTopic must not be empty", - "targetTopic must not be empty", - "startTimestamp must not be null", - "endTimestamp must not be null") + "sourceTopic must not be empty", + "targetTopic must not be empty", + "startTimestamp must not be null", + "endTimestamp must not be null") ); } @@ -98,7 +99,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNotExistingSo // given Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = createRequest("not.existing.sourceTopic", - targetTopic.getQualifiedName()); + targetTopic.getQualifiedName(), null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -114,7 +115,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNotExistingTa // given Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), "not.existing.targetTopic"); + sourceTopic.getQualifiedName(), "not.existing.targetTopic", null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -131,6 +132,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNegativeTimeR Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = new OfflineRetransmissionRequest( + "", sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), Instant.now().toString(), @@ -151,7 +153,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithTargetTopicSt Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().withOfflineStorage(1).build()); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), targetTopic.getQualifiedName()); + sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -170,7 +172,7 @@ public void shouldDeleteRetransmissionTask() { Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), targetTopic.getQualifiedName()); + sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), null); hermes.api().createOfflineRetransmissionTask(request); List allTasks = getOfflineRetransmissionTasks(); @@ -203,7 +205,7 @@ public void shouldThrowAccessDeniedWhenTryingToCreateTaskWithoutPermissionsToSou Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); TestSecurityProvider.setUserIsAdmin(false); OfflineRetransmissionRequest request = createRequest( - sourceTopic.getQualifiedName(), targetTopic.getQualifiedName()); + sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), null); // when WebTestClient.ResponseSpec response = hermes.api().createOfflineRetransmissionTask(request); @@ -218,8 +220,9 @@ public void shouldThrowAccessDeniedWhenTryingToCreateTaskWithoutPermissionsToSou TestSecurityProvider.reset(); } - private OfflineRetransmissionRequest createRequest(String sourceTopic, String targetTopic) { + private OfflineRetransmissionRequest createRequest(String sourceTopic, String targetTopic, String sourceView) { return new OfflineRetransmissionRequest( + sourceView, sourceTopic, targetTopic, Instant.now().minusSeconds(1).toString(), From 32fe56de48268ce8dafcfe920a3352cd88407197 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Tue, 9 Jul 2024 14:29:39 +0200 Subject: [PATCH 05/22] test coverage & cleanup --- .../OneSourceRetransmissionValidator.java | 8 ++-- ...neSourceRetransmissionValidatorTest.groovy | 37 +++++++++++++++++++ .../api/OfflineRetransmissionEndpoint.java | 12 +++--- .../OfflineRetransmissionService.java | 7 ++-- .../OfflineRetransmissionManagementTest.java | 24 ++++++++++++ 5 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 hermes-api/src/test/groovy/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidatorTest.groovy diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java index f91d85a047..2271503240 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -8,7 +8,6 @@ public class OneSourceRetransmissionValidator implements ConstraintValidator Date: Thu, 11 Jul 2024 15:30:03 +0200 Subject: [PATCH 06/22] test coverage & cleanup --- .../subscription/validator/InflightSizeValidatorTest.groovy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy index 74c534654e..be13fa4402 100644 --- a/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy +++ b/hermes-management/src/test/groovy/pl/allegro/tech/hermes/management/domain/subscription/validator/InflightSizeValidatorTest.groovy @@ -72,7 +72,8 @@ class InflightSizeValidatorTest extends Specification { then: def exception = thrown(ConstraintViolationException) - exception.message == "serialSubscriptionPolicy.inflightSize: must be greater than or equal to 1" + exception.message == "serialSubscriptionPolicy.inflightSize: must be greater than or equal to 1" || + "serialSubscriptionPolicy.inflightSize: musi być równe lub większe od 1" where: inflightSize << [0, -1] From c42f1af55062ad027e575daa6e5b3a58a44c971b Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Mon, 15 Jul 2024 16:53:41 +0200 Subject: [PATCH 07/22] checkstyle --- .../hermes/api/OfflineRetransmissionRequest.java | 14 +++++++------- .../tech/hermes/api/OfflineRetransmissionTask.java | 4 +++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java index 948ef55502..4e1dfefe2c 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java @@ -90,12 +90,12 @@ public Instant getEndTimestamp() { @Override public String toString() { - return "OfflineRetransmissionRequest{" + - "sourceView='" + sourceView + '\'' + - ", sourceTopic='" + sourceTopic + '\'' + - ", targetTopic='" + targetTopic + '\'' + - ", startTimestamp=" + startTimestamp + - ", endTimestamp=" + endTimestamp + - '}'; + return "OfflineRetransmissionRequest{" + + "sourceTopic='" + sourceTopic + '\'' + + ", sourceView='" + sourceView + '\'' + + ", targetTopic='" + targetTopic + '\'' + + ", startTimestamp=" + startTimestamp + + ", endTimestamp=" + endTimestamp + + '}'; } } diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java index 04902819d4..26aab3963a 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java @@ -45,7 +45,9 @@ public String getSourceTopic() { return request.getSourceTopic(); } - public String getSourceView() {return request.getSourceView();} + public String getSourceView() { + return request.getSourceView(); + } public String getTargetTopic() { return request.getTargetTopic(); From faabfd293dcff8b247f9b691c3add077caa12765 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Tue, 16 Jul 2024 14:30:05 +0200 Subject: [PATCH 08/22] retransmission view name refactor --- .../api/OfflineRetransmissionRequest.java | 12 +++++----- .../hermes/api/OfflineRetransmissionTask.java | 8 +++---- .../OneSourceRetransmissionValidator.java | 4 ++-- ...neSourceRetransmissionValidatorTest.groovy | 22 +++++++++---------- .../OfflineRetransmissionManagementTest.java | 7 +++--- 5 files changed, 26 insertions(+), 27 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java index 4e1dfefe2c..a424891039 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java @@ -26,7 +26,7 @@ public class OfflineRetransmissionRequest { ); private static final Logger logger = LoggerFactory.getLogger(OfflineRetransmissionRequest.class); - private final String sourceView; + private final String sourceViewPath; private final String sourceTopic; @NotEmpty private final String targetTopic; @@ -37,12 +37,12 @@ public class OfflineRetransmissionRequest { @JsonCreator public OfflineRetransmissionRequest( - @JsonProperty("sourceView") String sourceView, + @JsonProperty("sourceViewPath") String sourceViewPath, @JsonProperty("sourceTopic") String sourceTopic, @JsonProperty("targetTopic") String targetTopic, @JsonProperty("startTimestamp") String startTimestamp, @JsonProperty("endTimestamp") String endTimestamp) { - this.sourceView = sourceView; + this.sourceViewPath = sourceViewPath; this.sourceTopic = sourceTopic; this.targetTopic = targetTopic; this.startTimestamp = initializeTimestamp(startTimestamp); @@ -66,8 +66,8 @@ private Instant initializeTimestamp(String timestamp) { return null; } - public String getSourceView() { - return sourceView; + public String getSourceViewPath() { + return sourceViewPath; } public String getSourceTopic() { @@ -92,7 +92,7 @@ public Instant getEndTimestamp() { public String toString() { return "OfflineRetransmissionRequest{" + "sourceTopic='" + sourceTopic + '\'' - + ", sourceView='" + sourceView + '\'' + + ", sourceViewPath='" + sourceViewPath + '\'' + ", targetTopic='" + targetTopic + '\'' + ", startTimestamp=" + startTimestamp + ", endTimestamp=" + endTimestamp diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java index 26aab3963a..12c3713b47 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java @@ -16,14 +16,14 @@ public class OfflineRetransmissionTask { @JsonCreator public OfflineRetransmissionTask( @JsonProperty("taskId") String taskId, - @JsonProperty("sourceView") String sourceView, + @JsonProperty("sourceViewPath") String sourceViewPath, @JsonProperty("sourceTopic") String sourceTopic, @JsonProperty("targetTopic") String targetTopic, @JsonProperty("startTimestamp") Instant startTimestamp, @JsonProperty("endTimestamp") Instant endTimestamp, @JsonProperty("createdAt") Instant createdAt) { this(taskId, new OfflineRetransmissionRequest( - sourceView, + sourceViewPath, sourceTopic, targetTopic, startTimestamp.toString(), @@ -45,8 +45,8 @@ public String getSourceTopic() { return request.getSourceTopic(); } - public String getSourceView() { - return request.getSourceView(); + public String getSourceViewPath() { + return request.getSourceViewPath(); } public String getTargetTopic() { diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java index 2271503240..771c200ecd 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -12,10 +12,10 @@ public void initialize(OneSourceRetransmission oneSourceRetransmission) { @Override public boolean isValid(OfflineRetransmissionRequest offlineRetransmissionRequest, ConstraintValidatorContext context) { - var sourceView = offlineRetransmissionRequest.getSourceView(); + var sourceViewPath = offlineRetransmissionRequest.getSourceViewPath(); var sourceTopic = offlineRetransmissionRequest.getSourceTopic(); - return (nonBlank(sourceView) && sourceTopic == null) || (nonBlank(sourceTopic) && sourceView == null); + return (nonBlank(sourceViewPath) && sourceTopic == null) || (nonBlank(sourceTopic) && sourceViewPath == null); } private static boolean nonBlank(String value) { diff --git a/hermes-api/src/test/groovy/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidatorTest.groovy b/hermes-api/src/test/groovy/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidatorTest.groovy index d861e52fcd..e7aa7a42c3 100644 --- a/hermes-api/src/test/groovy/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidatorTest.groovy +++ b/hermes-api/src/test/groovy/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidatorTest.groovy @@ -10,10 +10,10 @@ class OneSourceRetransmissionValidatorTest extends Specification { OneSourceRetransmissionValidator validator = new OneSourceRetransmissionValidator() ConstraintValidatorContext mockContext = Mock() - def "Validator should validate retransmission request when sourceView is '#sourceView' and sourceTopic is '#sourceTopic'"() { + def "Validator should validate retransmission request when sourceViewPath is '#sourceViewPath' and sourceTopic is '#sourceTopic'"() { given: def request = new OfflineRetransmissionRequest( - sourceView, + sourceViewPath, sourceTopic, "someTargetTopic", "2024-07-08T12:00:00", @@ -23,15 +23,15 @@ class OneSourceRetransmissionValidatorTest extends Specification { validator.isValid(request, mockContext) == isValid where: - sourceView | sourceTopic | isValid - null | "testTopic" | true - "testView" | null | true - null | null | false - "testView" | "testTopic" | false - "" | "" | false - " " | " " | false - "" | "testTopic" | false - "testView" | " " | false + sourceViewPath | sourceTopic | isValid + null | "testTopic" | true + "testView" | null | true + null | null | false + "testView" | "testTopic" | false + "" | "" | false + " " | " " | false + "" | "testTopic" | false + "testView" | " " | false } } diff --git a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java index 7c1f6d8f4e..9da718416c 100644 --- a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java +++ b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java @@ -18,7 +18,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static pl.allegro.tech.hermes.test.helper.builder.TopicBuilder.topicWithRandomName; -import static pl.allegro.tech.hermes.test.helper.builder.TopicBuilder.topicWithRandomNameContaining; public class OfflineRetransmissionManagementTest { @@ -82,7 +81,7 @@ public void shouldCreateRetransmissionTaskWithViewInsteadTopic() { assertThat(allTasks.get(0).getStartTimestamp()).isEqualTo(request.getStartTimestamp()); assertThat(allTasks.get(0).getEndTimestamp()).isEqualTo(request.getEndTimestamp()); assertThat(allTasks.get(0).getSourceTopic()).isEqualTo(null); - assertThat(allTasks.get(0).getSourceView()).isEqualTo("testView"); + assertThat(allTasks.get(0).getSourceViewPath()).isEqualTo("testViewPath"); assertThat(allTasks.get(0).getTargetTopic()).isEqualTo(request.getTargetTopic()); assertThat(allTasks.get(0).getCreatedAt()).isBefore(now); } @@ -244,9 +243,9 @@ public void shouldThrowAccessDeniedWhenTryingToCreateTaskWithoutPermissionsToSou TestSecurityProvider.reset(); } - private OfflineRetransmissionRequest createRequest(String sourceTopic, String targetTopic, String sourceView) { + private OfflineRetransmissionRequest createRequest(String sourceTopic, String targetTopic, String sourceViewPath) { return new OfflineRetransmissionRequest( - sourceView, + sourceViewPath, sourceTopic, targetTopic, Instant.now().minusSeconds(1).toString(), From 567df51041ed1aa6dcc0b6da5365e5c9e5dceac8 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 24 Jul 2024 10:18:38 +0200 Subject: [PATCH 09/22] pr adjustments --- .../tech/hermes/api/OfflineRetransmissionRequest.java | 9 +++++---- .../tech/hermes/api/OfflineRetransmissionTask.java | 5 +++-- .../constraints/OneSourceRetransmissionValidator.java | 5 ++++- .../management/api/OfflineRetransmissionEndpoint.java | 7 ++++--- .../domain/retransmit/OfflineRetransmissionService.java | 2 +- 5 files changed, 17 insertions(+), 11 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java index a424891039..a63b80f073 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionRequest.java @@ -15,6 +15,7 @@ import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; import java.util.List; +import java.util.Optional; @OneSourceRetransmission public class OfflineRetransmissionRequest { @@ -66,12 +67,12 @@ private Instant initializeTimestamp(String timestamp) { return null; } - public String getSourceViewPath() { - return sourceViewPath; + public Optional getSourceViewPath() { + return Optional.ofNullable(sourceViewPath); } - public String getSourceTopic() { - return sourceTopic; + public Optional getSourceTopic() { + return Optional.ofNullable(sourceTopic); } public String getTargetTopic() { diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java index 12c3713b47..c3fb94f05c 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/OfflineRetransmissionTask.java @@ -7,6 +7,7 @@ import pl.allegro.tech.hermes.api.jackson.InstantIsoSerializer; import java.time.Instant; +import java.util.Optional; public class OfflineRetransmissionTask { private final String taskId; @@ -41,11 +42,11 @@ public String getTaskId() { return taskId; } - public String getSourceTopic() { + public Optional getSourceTopic() { return request.getSourceTopic(); } - public String getSourceViewPath() { + public Optional getSourceViewPath() { return request.getSourceViewPath(); } diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java index 771c200ecd..8dad23e509 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -6,6 +6,8 @@ public class OneSourceRetransmissionValidator implements ConstraintValidator { + public static final String EMPTY_STRING = ""; + @Override public void initialize(OneSourceRetransmission oneSourceRetransmission) { } @@ -15,7 +17,8 @@ public boolean isValid(OfflineRetransmissionRequest offlineRetransmissionRequest var sourceViewPath = offlineRetransmissionRequest.getSourceViewPath(); var sourceTopic = offlineRetransmissionRequest.getSourceTopic(); - return (nonBlank(sourceViewPath) && sourceTopic == null) || (nonBlank(sourceTopic) && sourceViewPath == null); + return (nonBlank(sourceViewPath.orElse(EMPTY_STRING)) && sourceTopic.isEmpty()) || + (nonBlank(sourceTopic.orElse(EMPTY_STRING)) && sourceViewPath.isEmpty()); } private static boolean nonBlank(String value) { diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index 29d5611020..c5cb7f376f 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -24,6 +24,7 @@ import pl.allegro.tech.hermes.management.domain.retransmit.OfflineRetransmissionService; import java.util.List; +import java.util.Optional; import static jakarta.ws.rs.core.MediaType.APPLICATION_JSON; @@ -85,9 +86,9 @@ private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, } } - private boolean validateSourceTopic(String sourceTopic, ContainerRequestContext requestContext) { - return sourceTopic == null || managementRights.isUserAllowedToManageTopic( - topicRepository.getTopicDetails(TopicName.fromQualifiedName(sourceTopic)), + private boolean validateSourceTopic(Optional sourceTopic, ContainerRequestContext requestContext) { + return sourceTopic.isEmpty() || managementRights.isUserAllowedToManageTopic( + topicRepository.getTopicDetails(TopicName.fromQualifiedName(sourceTopic.get())), requestContext ); } diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java index 0c107d4808..e418482bfb 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java @@ -21,7 +21,7 @@ public OfflineRetransmissionService(OfflineRetransmissionRepository offlineRetra } public void validateRequest(OfflineRetransmissionRequest request) { - TopicName sourceTopicName = TopicName.fromQualifiedName(request.getSourceTopic()); + TopicName sourceTopicName = TopicName.fromQualifiedName(request.getSourceTopic().orElse("")); TopicName targetTopicName = TopicName.fromQualifiedName(request.getTargetTopic()); ensureTopicsExist(sourceTopicName, targetTopicName); From 315b51fb452b03f809e92a55918804030240ad58 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 24 Jul 2024 12:05:01 +0200 Subject: [PATCH 10/22] checkstyle --- .../api/constraints/OneSourceRetransmissionValidator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java index 8dad23e509..e3383b208f 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -17,8 +17,8 @@ public boolean isValid(OfflineRetransmissionRequest offlineRetransmissionRequest var sourceViewPath = offlineRetransmissionRequest.getSourceViewPath(); var sourceTopic = offlineRetransmissionRequest.getSourceTopic(); - return (nonBlank(sourceViewPath.orElse(EMPTY_STRING)) && sourceTopic.isEmpty()) || - (nonBlank(sourceTopic.orElse(EMPTY_STRING)) && sourceViewPath.isEmpty()); + return (nonBlank(sourceViewPath.orElse(EMPTY_STRING)) && sourceTopic.isEmpty()) + || (nonBlank(sourceTopic.orElse(EMPTY_STRING)) && sourceViewPath.isEmpty()); } private static boolean nonBlank(String value) { From 181b1831f6b225f08085b3aecc8d38936e89f46a Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Tue, 30 Jul 2024 13:26:08 +0200 Subject: [PATCH 11/22] jdk8Module registered in ObjectMapper --- .../management/config/ManagementConfiguration.java | 3 ++- .../OfflineRetransmissionManagementTest.java | 11 ++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/config/ManagementConfiguration.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/config/ManagementConfiguration.java index e4cc665d9a..c4fbe3150f 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/config/ManagementConfiguration.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/config/ManagementConfiguration.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.databind.InjectableValues; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.datatype.jdk8.Jdk8Module; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; import io.micrometer.core.instrument.MeterRegistry; import org.springframework.beans.factory.annotation.Autowired; @@ -42,7 +43,7 @@ public ObjectMapper objectMapper() { mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL); mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); mapper.disable(SerializationFeature.WRITE_NULL_MAP_VALUES); - mapper.registerModule(new JavaTimeModule()); + mapper.registerModules(new JavaTimeModule(), new Jdk8Module()); final InjectableValues defaultSchemaIdAwareSerializationEnabled = new InjectableValues.Std().addValue( Topic.DEFAULT_SCHEMA_ID_SERIALIZATION_ENABLED_KEY, diff --git a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java index 9da718416c..ba8120131d 100644 --- a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java +++ b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java @@ -15,6 +15,7 @@ import java.time.Instant; import java.util.List; +import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; import static pl.allegro.tech.hermes.test.helper.builder.TopicBuilder.topicWithRandomName; @@ -68,7 +69,7 @@ public void shouldCreateRetransmissionTaskWithViewInsteadTopic() { var targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); // when - var request = createRequest(null, targetTopic.getQualifiedName(), "testView"); + var request = createRequest(null, targetTopic.getQualifiedName(), "testViewPath"); var response = hermes.api().createOfflineRetransmissionTask(request); var now = Instant.now(); @@ -80,8 +81,8 @@ public void shouldCreateRetransmissionTaskWithViewInsteadTopic() { assertThat(allTasks.size()).isEqualTo(1); assertThat(allTasks.get(0).getStartTimestamp()).isEqualTo(request.getStartTimestamp()); assertThat(allTasks.get(0).getEndTimestamp()).isEqualTo(request.getEndTimestamp()); - assertThat(allTasks.get(0).getSourceTopic()).isEqualTo(null); - assertThat(allTasks.get(0).getSourceViewPath()).isEqualTo("testViewPath"); + assertThat(allTasks.get(0).getSourceTopic()).isEqualTo(Optional.empty()); + assertThat(allTasks.get(0).getSourceViewPath()).isEqualTo(Optional.of("testViewPath")); assertThat(allTasks.get(0).getTargetTopic()).isEqualTo(request.getTargetTopic()); assertThat(allTasks.get(0).getCreatedAt()).isBefore(now); } @@ -96,7 +97,7 @@ public void shouldReturnEmptyListIfThereAreNoTasks() { public void shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData() { // given OfflineRetransmissionRequest request = new OfflineRetransmissionRequest( - "", + null, "", "", null, @@ -155,7 +156,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithNegativeTimeR Topic sourceTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); Topic targetTopic = hermes.initHelper().createTopic(topicWithRandomName().build()); OfflineRetransmissionRequest request = new OfflineRetransmissionRequest( - "", + null, sourceTopic.getQualifiedName(), targetTopic.getQualifiedName(), Instant.now().toString(), From dfda5661e3b7b017b68070adcdec3214e3bd20b4 Mon Sep 17 00:00:00 2001 From: Adam Izydorczyk Date: Tue, 30 Jul 2024 14:56:28 +0200 Subject: [PATCH 12/22] validation check fix in shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData test --- .../tech/hermes/api/constraints/OneSourceRetransmission.java | 2 +- .../management/OfflineRetransmissionManagementTest.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java index 52625ed94f..9c920a322b 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmission.java @@ -14,7 +14,7 @@ @Target({TYPE}) @Constraint(validatedBy = OneSourceRetransmissionValidator.class) public @interface OneSourceRetransmission { - String message() default "Only one source of retransmission data is allowed - source topic or source view"; + String message() default "must contain one defined source of retransmission data - source topic or source view"; Class[] groups() default {}; diff --git a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java index ba8120131d..1d9043ab39 100644 --- a/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java +++ b/integration-tests/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/management/OfflineRetransmissionManagementTest.java @@ -111,8 +111,7 @@ public void shouldReturnClientErrorWhenRequestingRetransmissionWithEmptyData() { response.expectStatus().isBadRequest(); assertThat(response.expectBody(String.class).returnResult().getResponseBody()).contains( List.of( - "sourceTopic must not be empty", - "targetTopic must not be empty", + "must contain one defined source of retransmission data - source topic or source view", "startTimestamp must not be null", "endTimestamp must not be null") ); From 0ef11b0a874216eb50c2cf437e1d4b3e63337420 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 31 Jul 2024 12:26:16 +0200 Subject: [PATCH 13/22] validation adjustment --- .../domain/retransmit/OfflineRetransmissionService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java index e418482bfb..0c7259da15 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/domain/retransmit/OfflineRetransmissionService.java @@ -21,7 +21,7 @@ public OfflineRetransmissionService(OfflineRetransmissionRepository offlineRetra } public void validateRequest(OfflineRetransmissionRequest request) { - TopicName sourceTopicName = TopicName.fromQualifiedName(request.getSourceTopic().orElse("")); + TopicName sourceTopicName = TopicName.fromQualifiedName(request.getSourceTopic().orElse(null)); TopicName targetTopicName = TopicName.fromQualifiedName(request.getTargetTopic()); ensureTopicsExist(sourceTopicName, targetTopicName); From 241787154097acc1867da5b66ae255a417dc80a7 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 31 Jul 2024 13:42:19 +0200 Subject: [PATCH 14/22] validation adjustment --- .../hermes/management/api/OfflineRetransmissionEndpoint.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index c5cb7f376f..c45f6a610d 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -47,6 +47,7 @@ public OfflineRetransmissionEndpoint(OfflineRetransmissionService retransmission @POST @Consumes(APPLICATION_JSON) public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { + System.out.println("Offline retransmission request: " + request); retransmissionService.validateRequest(request); permissions.ensurePermissionsToBothTopics(request, requestContext); var task = retransmissionService.createTask(request); From 046392984b31297100cd13cfb84e0186472224d0 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 31 Jul 2024 13:44:02 +0200 Subject: [PATCH 15/22] validation testing --- .../src/main/java/pl/allegro/tech/hermes/api/TopicName.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java index 32c903fdb2..6de3a4e1f6 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java @@ -39,6 +39,7 @@ public static TopicName fromQualifiedName(String qualifiedName) { int index = qualifiedName.lastIndexOf(GROUP_SEPARATOR); if (index == -1) { + System.out.println("Invalid qualified name: " + qualifiedName); throw new IllegalArgumentException("Missing group"); } From fba4255f32ca1c7bebd7d2f3359b46319e631ad3 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 31 Jul 2024 14:34:10 +0200 Subject: [PATCH 16/22] validation testing --- .../src/main/java/pl/allegro/tech/hermes/api/TopicName.java | 5 ++++- .../hermes/management/api/OfflineRetransmissionEndpoint.java | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java index 6de3a4e1f6..6fea97fb21 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java @@ -5,6 +5,8 @@ import com.google.common.base.Strings; import jakarta.validation.constraints.NotEmpty; import jakarta.validation.constraints.Pattern; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.Objects; @@ -13,6 +15,7 @@ public class TopicName { public static final char GROUP_SEPARATOR = '.'; + private static final Logger logger = LoggerFactory.getLogger(TopicName.class); @NotEmpty @Pattern(regexp = ALLOWED_NAME_REGEX) @@ -39,7 +42,7 @@ public static TopicName fromQualifiedName(String qualifiedName) { int index = qualifiedName.lastIndexOf(GROUP_SEPARATOR); if (index == -1) { - System.out.println("Invalid qualified name: " + qualifiedName); + logger.info("Invalid qualified name {}", qualifiedName); throw new IllegalArgumentException("Missing group"); } diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index c45f6a610d..cfd6f5c3c7 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -36,6 +36,7 @@ public class OfflineRetransmissionEndpoint { private final OfflineRetransmissionService retransmissionService; private final RetransmissionPermissions permissions; private final OfflineRetransmissionAuditor auditor; + private final Logger logger = LoggerFactory.getLogger(OfflineRetransmissionEndpoint.class); public OfflineRetransmissionEndpoint(OfflineRetransmissionService retransmissionService, TopicRepository topicRepository, ManagementRights managementRights) { @@ -47,7 +48,7 @@ public OfflineRetransmissionEndpoint(OfflineRetransmissionService retransmission @POST @Consumes(APPLICATION_JSON) public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { - System.out.println("Offline retransmission request: " + request); + logger.info("Offline retransmission request: {}", request); retransmissionService.validateRequest(request); permissions.ensurePermissionsToBothTopics(request, requestContext); var task = retransmissionService.createTask(request); From 857e77806e3abd2f0174ed707af39152f5a8d8c1 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 31 Jul 2024 15:46:36 +0200 Subject: [PATCH 17/22] validation testing --- .../api/OfflineRetransmissionEndpoint.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index cfd6f5c3c7..38e39a205c 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -20,7 +20,6 @@ import pl.allegro.tech.hermes.api.TopicName; import pl.allegro.tech.hermes.domain.topic.TopicRepository; import pl.allegro.tech.hermes.management.api.auth.ManagementRights; -import pl.allegro.tech.hermes.management.domain.PermissionDeniedException; import pl.allegro.tech.hermes.management.domain.retransmit.OfflineRetransmissionService; import java.util.List; @@ -72,7 +71,7 @@ public Response deleteRetransmissionTask(@PathParam("taskId") String taskId) { private static class RetransmissionPermissions { private final TopicRepository topicRepository; private final ManagementRights managementRights; - + private final static Logger logger = LoggerFactory.getLogger(RetransmissionPermissions.class); private RetransmissionPermissions(TopicRepository topicRepository, ManagementRights managementRights) { this.topicRepository = topicRepository; @@ -81,10 +80,15 @@ private RetransmissionPermissions(TopicRepository topicRepository, ManagementRig private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, ContainerRequestContext requestContext) { var targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); - var hasPermissions = validateSourceTopic(request.getSourceTopic(), requestContext) - && managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); + var topicValidation = validateSourceTopic(request.getSourceTopic(), requestContext); + var userValidation = managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); + var hasPermissions = topicValidation + && userValidation; if (!hasPermissions) { - throw new PermissionDeniedException("User needs permissions to source and target topics."); + logger.info("target topic {}", targetTopic); + logger.info("topic validation: {}, user validation: {}", topicValidation, userValidation); + logger.info("ContainerRequestContext: {}", requestContext); +// throw new PermissionDeniedException("User needs permissions to source and target topics."); } } From a7440e432a6d217c8c5568b1cffd91012b20a176 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 31 Jul 2024 16:30:46 +0200 Subject: [PATCH 18/22] validation testing --- .../api/OfflineRetransmissionEndpoint.java | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index 38e39a205c..ff2113b123 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -20,6 +20,7 @@ import pl.allegro.tech.hermes.api.TopicName; import pl.allegro.tech.hermes.domain.topic.TopicRepository; import pl.allegro.tech.hermes.management.api.auth.ManagementRights; +import pl.allegro.tech.hermes.management.domain.PermissionDeniedException; import pl.allegro.tech.hermes.management.domain.retransmit.OfflineRetransmissionService; import java.util.List; @@ -80,15 +81,10 @@ private RetransmissionPermissions(TopicRepository topicRepository, ManagementRig private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, ContainerRequestContext requestContext) { var targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); - var topicValidation = validateSourceTopic(request.getSourceTopic(), requestContext); - var userValidation = managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); - var hasPermissions = topicValidation - && userValidation; + var hasPermissions = validateSourceTopic(request.getSourceTopic(), requestContext) + && managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); if (!hasPermissions) { - logger.info("target topic {}", targetTopic); - logger.info("topic validation: {}, user validation: {}", topicValidation, userValidation); - logger.info("ContainerRequestContext: {}", requestContext); -// throw new PermissionDeniedException("User needs permissions to source and target topics."); + throw new PermissionDeniedException("User needs permissions to source and target topics."); } } From 366878142d3bea018154a4cb87fc04c72e2a3451 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Wed, 7 Aug 2024 16:33:25 +0200 Subject: [PATCH 19/22] pr adjustments --- .../src/main/java/pl/allegro/tech/hermes/api/TopicName.java | 6 +----- .../api/constraints/OneSourceRetransmissionValidator.java | 4 ---- .../management/api/OfflineRetransmissionEndpoint.java | 4 ++-- .../hermes/management/config/ManagementConfiguration.java | 2 +- .../subscription/validator/InflightSizeValidatorTest.groovy | 3 +-- .../management/OfflineRetransmissionManagementTest.java | 5 ++--- 6 files changed, 7 insertions(+), 17 deletions(-) diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java index 6fea97fb21..3c9704f211 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicName.java @@ -5,8 +5,6 @@ import com.google.common.base.Strings; import jakarta.validation.constraints.NotEmpty; import jakarta.validation.constraints.Pattern; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.util.Objects; @@ -15,7 +13,6 @@ public class TopicName { public static final char GROUP_SEPARATOR = '.'; - private static final Logger logger = LoggerFactory.getLogger(TopicName.class); @NotEmpty @Pattern(regexp = ALLOWED_NAME_REGEX) @@ -42,8 +39,7 @@ public static TopicName fromQualifiedName(String qualifiedName) { int index = qualifiedName.lastIndexOf(GROUP_SEPARATOR); if (index == -1) { - logger.info("Invalid qualified name {}", qualifiedName); - throw new IllegalArgumentException("Missing group"); + throw new IllegalArgumentException("Invalid qualified name " + qualifiedName); } String groupName = qualifiedName.substring(0, index); diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java index e3383b208f..a311b4b263 100644 --- a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java +++ b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/constraints/OneSourceRetransmissionValidator.java @@ -8,10 +8,6 @@ public class OneSourceRetransmissionValidator implements ConstraintValidator Date: Wed, 7 Aug 2024 16:56:59 +0200 Subject: [PATCH 20/22] checkstyle --- .../hermes/management/api/OfflineRetransmissionEndpoint.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index d15f71c736..08716a1dc6 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -70,9 +70,9 @@ public Response deleteRetransmissionTask(@PathParam("taskId") String taskId) { } private static class RetransmissionPermissions { + private final Logger logger = LoggerFactory.getLogger(RetransmissionPermissions.class); private final TopicRepository topicRepository; private final ManagementRights managementRights; - private final static Logger logger = LoggerFactory.getLogger(RetransmissionPermissions.class); private RetransmissionPermissions(TopicRepository topicRepository, ManagementRights managementRights) { this.topicRepository = topicRepository; From a9b3a47b1b1a082cc2f37096d92dace497243038 Mon Sep 17 00:00:00 2001 From: bartekdrobczyk <165910486+bartekdrobczyk@users.noreply.github.com> Date: Tue, 13 Aug 2024 10:50:17 +0200 Subject: [PATCH 21/22] Update hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java Co-authored-by: Maciej Moscicki --- .../hermes/management/api/OfflineRetransmissionEndpoint.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index 08716a1dc6..c9eeac0d13 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -83,7 +83,7 @@ private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, var targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); var hasPermissions = validateSourceTopic(request.getSourceTopic(), requestContext) && managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); if (!hasPermissions) { - logger.info("User {} has no permissions to make retransmission {}", requestContext.getSecurityContext().getUserPrincipal(), request); + logger.info("User {} has no permissions to make offline retransmission {}", requestContext.getSecurityContext().getUserPrincipal(), request); throw new PermissionDeniedException("User needs permissions to source and target topics."); } } From 8d83cdf4597e708b86ef58a9898e60fddc03b892 Mon Sep 17 00:00:00 2001 From: "bartlomiej.drobczyk" Date: Tue, 13 Aug 2024 11:51:55 +0200 Subject: [PATCH 22/22] permission method better naming --- .../hermes/management/api/OfflineRetransmissionEndpoint.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java index 08716a1dc6..802a55dc68 100644 --- a/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java +++ b/hermes-management/src/main/java/pl/allegro/tech/hermes/management/api/OfflineRetransmissionEndpoint.java @@ -50,7 +50,7 @@ public OfflineRetransmissionEndpoint(OfflineRetransmissionService retransmission public Response createRetransmissionTask(@Valid OfflineRetransmissionRequest request, @Context ContainerRequestContext requestContext) { logger.info("Offline retransmission request: {}", request); retransmissionService.validateRequest(request); - permissions.ensurePermissionsToBothTopics(request, requestContext); + permissions.ensurePermissions(request, requestContext); var task = retransmissionService.createTask(request); auditor.auditRetransmissionCreation(request, requestContext, task); return Response.status(Response.Status.CREATED).build(); @@ -79,7 +79,7 @@ private RetransmissionPermissions(TopicRepository topicRepository, ManagementRig this.managementRights = managementRights; } - private void ensurePermissionsToBothTopics(OfflineRetransmissionRequest request, ContainerRequestContext requestContext) { + private void ensurePermissions(OfflineRetransmissionRequest request, ContainerRequestContext requestContext) { var targetTopic = topicRepository.getTopicDetails(TopicName.fromQualifiedName(request.getTargetTopic())); var hasPermissions = validateSourceTopic(request.getSourceTopic(), requestContext) && managementRights.isUserAllowedToManageTopic(targetTopic, requestContext); if (!hasPermissions) {