Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: separate unexpected from expected exceptions #1744

Merged
merged 5 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,6 @@ public DataAddress resolveForAsset(String assetId) {
Objects.requireNonNull(assetId, "assetId");
lock.readLock().lock();
try {
if (!dataAddresses.containsKey(assetId) || dataAddresses.get(assetId) == null) {
throw new IllegalArgumentException("No DataAddress found for Asset ID=" + assetId);
}
return dataAddresses.get(assetId);
} finally {
lock.readLock().unlock();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,40 +14,39 @@

package org.eclipse.dataspaceconnector.core.defaults.assetindex;

import org.assertj.core.api.Assertions;
import org.eclipse.dataspaceconnector.spi.types.domain.DataAddress;
import org.eclipse.dataspaceconnector.spi.types.domain.asset.Asset;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.UUID;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

class InMemoryDataAddressResolverTest {
private InMemoryAssetIndex resolver;


@BeforeEach
void setUp() {
resolver = new InMemoryAssetIndex();
}

@Test
void resolveForAsset() {
String id = UUID.randomUUID().toString();
var id = UUID.randomUUID().toString();
var testAsset = createAsset("foobar", id);
DataAddress address = createDataAddress(testAsset);
var address = createDataAddress(testAsset);
resolver.accept(testAsset, address);

Assertions.assertThat(resolver.resolveForAsset(testAsset.getId())).isEqualTo(address);
assertThat(resolver.resolveForAsset(testAsset.getId())).isEqualTo(address);
}

@Test
void resolveForAsset_assetNull_raisesException() {
String id = UUID.randomUUID().toString();
var id = UUID.randomUUID().toString();
var testAsset = createAsset("foobar", id);
DataAddress address = createDataAddress(testAsset);
var address = createDataAddress(testAsset);
resolver.accept(testAsset, address);

assertThatThrownBy(() -> resolver.resolveForAsset(null)).isInstanceOf(NullPointerException.class);
Expand All @@ -58,11 +57,9 @@ void resolveForAsset_whenAssetDeleted_raisesException() {
var testAsset = createAsset("foobar", UUID.randomUUID().toString());
var address = createDataAddress(testAsset);
resolver.accept(testAsset, address);

resolver.deleteById(testAsset.getId());
assertThatThrownBy(() -> resolver.resolveForAsset(testAsset.getId()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(String.format("No DataAddress found for Asset ID=%s", testAsset.getId()));

assertThat(resolver.resolveForAsset(testAsset.getId())).isNull();
}

private Asset createAsset(String name, String id) {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import org.eclipse.dataspaceconnector.api.result.ServiceResult;
import org.eclipse.dataspaceconnector.spi.EdcException;
import org.eclipse.dataspaceconnector.spi.exception.InvalidRequestException;
import org.eclipse.dataspaceconnector.spi.exception.ObjectExistsException;
import org.eclipse.dataspaceconnector.spi.exception.ObjectNotFoundException;
import org.jetbrains.annotations.NotNull;
Expand All @@ -37,7 +38,7 @@ public class ServiceResultHandler {
* <td>CONFLICT</td> <td>ObjectExistsException</td>
* </tr>
* <tr>
* <td>BAD_REQUEST</td> <td>IllegalArgumentException</td>
* <td>BAD_REQUEST</td> <td>InvalidRequestException</td>
* </tr>
* <tr>
* <td>other</td> <td>EdcException</td>
Expand All @@ -51,16 +52,16 @@ public class ServiceResultHandler {
* {@link org.eclipse.dataspaceconnector.api.result.ServiceFailure.Reason#BAD_REQUEST}.
* @return Exception mapped from failure reason.
*/
public static RuntimeException mapToException(@NotNull ServiceResult<?> result, @NotNull Class<?> clazz, @Nullable String id) {
public static EdcException mapToException(@NotNull ServiceResult<?> result, @NotNull Class<?> clazz, @Nullable String id) {
switch (result.reason()) {
case NOT_FOUND:
return new ObjectNotFoundException(clazz, id);
case CONFLICT:
return new ObjectExistsException(clazz, id);
case BAD_REQUEST:
return new IllegalArgumentException(result.getFailureDetail());
return new InvalidRequestException(result.getFailureMessages());
default:
return new EdcException("unexpected error");
return new EdcException("unexpected error: " + result.getFailureDetail());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.eclipse.dataspaceconnector.api.datamanagement.asset.service.AssetService;
import org.eclipse.dataspaceconnector.api.query.QuerySpecDto;
import org.eclipse.dataspaceconnector.api.transformer.DtoTransformerRegistry;
import org.eclipse.dataspaceconnector.spi.exception.InvalidRequestException;
import org.eclipse.dataspaceconnector.spi.exception.ObjectNotFoundException;
import org.eclipse.dataspaceconnector.spi.monitor.Monitor;
import org.eclipse.dataspaceconnector.spi.query.QuerySpec;
Expand All @@ -40,9 +41,10 @@

import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static java.lang.String.format;
import static java.util.stream.Collectors.toList;
import static org.eclipse.dataspaceconnector.api.ServiceResultHandler.mapToException;

@Consumes({ MediaType.APPLICATION_JSON })
Expand All @@ -67,7 +69,8 @@ public void createAsset(@Valid AssetEntryDto assetEntryDto) {
var dataAddressResult = transformerRegistry.transform(assetEntryDto.getDataAddress(), DataAddress.class);

if (assetResult.failed() || dataAddressResult.failed()) {
throw new IllegalArgumentException("Request is not well formatted");
var errorMessages = Stream.concat(assetResult.getFailureMessages().stream(), dataAddressResult.getFailureMessages().stream());
throw new InvalidRequestException(errorMessages.collect(toList()));
}

var dataAddress = dataAddressResult.getContent();
Expand All @@ -87,8 +90,7 @@ public void createAsset(@Valid AssetEntryDto assetEntryDto) {
public List<AssetDto> getAllAssets(@Valid @BeanParam QuerySpecDto querySpecDto) {
var transformationResult = transformerRegistry.transform(querySpecDto, QuerySpec.class);
if (transformationResult.failed()) {
monitor.warning("Error transforming QuerySpec: " + String.join(", ", transformationResult.getFailureMessages()));
throw new IllegalArgumentException("Cannot transform QuerySpecDto object");
throw new InvalidRequestException(transformationResult.getFailureMessages());
}

var spec = transformationResult.getContent();
Expand All @@ -107,7 +109,7 @@ public List<AssetDto> getAllAssets(@Valid @BeanParam QuerySpecDto querySpecDto)
.map(it -> transformerRegistry.transform(it, AssetDto.class))
.filter(Result::succeeded)
.map(Result::getContent)
.collect(Collectors.toList());
.collect(toList());
}

@GET
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
package org.eclipse.dataspaceconnector.api.datamanagement.asset.model;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import jakarta.validation.constraints.AssertTrue;
import jakarta.validation.constraints.NotNull;

import java.util.Map;
Expand All @@ -30,6 +32,12 @@ public class AssetDto {
private AssetDto() {
}

@JsonIgnore
@AssertTrue(message = "no empty property keys")
public boolean isValid() {
return properties != null && properties.keySet().stream().noneMatch(it -> it == null || it.isBlank());
}

public Map<String, Object> getProperties() {
return properties;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;

@JsonDeserialize(builder = AssetEntryDto.Builder.class)
public class AssetEntryDto {
@NotNull(message = "asset cannot be null")
@Valid
private AssetDto asset;
@NotNull(message = "dataAddress cannot be null")
@Valid
private DataAddressDto dataAddress;

private AssetEntryDto() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
package org.eclipse.dataspaceconnector.api.datamanagement.asset.model;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import jakarta.validation.constraints.AssertTrue;
import jakarta.validation.constraints.NotNull;

import java.util.Map;
Expand All @@ -34,6 +36,12 @@ public Map<String, String> getProperties() {
return properties;
}

@JsonIgnore
@AssertTrue(message = "property keys cannot be blank and property 'type' is mandatory")
public boolean isValid() {
return properties != null && properties.keySet().stream().noneMatch(it -> it == null || it.isBlank()) && properties.containsKey("type");
}

@JsonPOJOBuilder(withPrefix = "")
public static final class Builder {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.eclipse.dataspaceconnector.api.query.QuerySpecDto;
import org.eclipse.dataspaceconnector.api.result.ServiceResult;
import org.eclipse.dataspaceconnector.api.transformer.DtoTransformerRegistry;
import org.eclipse.dataspaceconnector.spi.exception.InvalidRequestException;
import org.eclipse.dataspaceconnector.spi.exception.ObjectExistsException;
import org.eclipse.dataspaceconnector.spi.exception.ObjectNotFoundException;
import org.eclipse.dataspaceconnector.spi.monitor.Monitor;
Expand All @@ -47,7 +48,6 @@

public class AssetApiControllerTest {


private final AssetService service = mock(AssetService.class);
private final DtoTransformerRegistry transformerRegistry = mock(DtoTransformerRegistry.class);
private AssetApiController controller;
Expand Down Expand Up @@ -100,7 +100,7 @@ void createAsset_transformFails() {
when(transformerRegistry.transform(isA(DataAddressDto.class), eq(DataAddress.class))).thenReturn(Result.failure("failed"));
when(service.create(any(), any())).thenReturn(ServiceResult.conflict("already exists"));

assertThatThrownBy(() -> controller.createAsset(assetEntry)).isInstanceOf(IllegalArgumentException.class);
assertThatThrownBy(() -> controller.createAsset(assetEntry)).isInstanceOf(InvalidRequestException.class);
}

@Test
Expand Down Expand Up @@ -137,7 +137,7 @@ void getAll_throwsExceptionIfQuerySpecTransformFails() {
when(transformerRegistry.transform(isA(QuerySpecDto.class), eq(QuerySpec.class)))
.thenReturn(Result.failure("Cannot transform"));

assertThatThrownBy(() -> controller.getAllAssets(QuerySpecDto.Builder.newInstance().build())).isInstanceOf(IllegalArgumentException.class);
assertThatThrownBy(() -> controller.getAllAssets(QuerySpecDto.Builder.newInstance().build())).isInstanceOf(InvalidRequestException.class);
}

@Test
Expand All @@ -147,7 +147,7 @@ void getAll_throwsExceptionIfQueryFails() {

when(service.query(any())).thenReturn(ServiceResult.badRequest("error"));

assertThatThrownBy(() -> controller.getAllAssets(QuerySpecDto.Builder.newInstance().build())).isInstanceOf(IllegalArgumentException.class);
assertThatThrownBy(() -> controller.getAllAssets(QuerySpecDto.Builder.newInstance().build())).isInstanceOf(InvalidRequestException.class);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void verifyValidation_assetEntryDto_missingAsset() {

var result = validator.validate(entry);

assertThat(result).hasSize(1).allSatisfy(cv -> assertThat(cv.getMessage()).isEqualTo("asset cannot be null"));
assertThat(result).hasSize(3).anySatisfy(cv -> assertThat(cv.getMessage()).isEqualTo("asset cannot be null"));
}

@Test
Expand All @@ -54,7 +54,7 @@ void verifyValidation_assetEntryDto_missingDataAddress() {

var result = validator.validate(entry);

assertThat(result).hasSize(1).allSatisfy(cv -> assertThat(cv.getMessage()).isEqualTo("dataAddress cannot be null"));
assertThat(result).hasSize(3).anySatisfy(cv -> assertThat(cv.getMessage()).isEqualTo("dataAddress cannot be null"));
}

@Test
Expand All @@ -65,6 +65,6 @@ void verifyValidation_assetDto_missingProperties() {

var result = validator.validate(asset);

assertThat(result).allSatisfy(cv -> assertThat(cv.getMessage()).isEqualTo("properties cannot be null"));
assertThat(result).anySatisfy(cv -> assertThat(cv.getMessage()).isEqualTo("properties cannot be null"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.eclipse.dataspaceconnector.api.datamanagement.catalog.service.CatalogService;
import org.eclipse.dataspaceconnector.api.query.QuerySpecDto;
import org.eclipse.dataspaceconnector.api.transformer.DtoTransformerRegistry;
import org.eclipse.dataspaceconnector.spi.exception.InvalidRequestException;
import org.eclipse.dataspaceconnector.spi.monitor.Monitor;
import org.eclipse.dataspaceconnector.spi.query.QuerySpec;
import org.jetbrains.annotations.NotNull;
Expand All @@ -52,14 +53,14 @@ public void getCatalog(@QueryParam("providerUrl") String providerUrl, @Valid @Be
if (querySpecDto != null) {
var result = transformerRegistry.transform(querySpecDto, QuerySpec.class);
if (result.failed()) {
monitor.warning("Error transforming QuerySpec: " + String.join(", ", result.getFailureMessages()));
throw new IllegalArgumentException("Cannot transform QuerySpecDto object");
throw new InvalidRequestException(result.getFailureMessages());
}
spec = result.getContent();
} else {
spec = QuerySpec.max();
monitor.debug("No paging parameters were supplied, using 0...Integer.MAX_VALUE");
}

service.getByProviderUrl(providerUrl, spec)
.whenComplete((content, throwable) -> {
if (throwable == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.eclipse.dataspaceconnector.api.datamanagement.contractagreement.service.ContractAgreementService;
import org.eclipse.dataspaceconnector.api.query.QuerySpecDto;
import org.eclipse.dataspaceconnector.api.transformer.DtoTransformerRegistry;
import org.eclipse.dataspaceconnector.spi.exception.InvalidRequestException;
import org.eclipse.dataspaceconnector.spi.exception.ObjectNotFoundException;
import org.eclipse.dataspaceconnector.spi.monitor.Monitor;
import org.eclipse.dataspaceconnector.spi.query.QuerySpec;
Expand Down Expand Up @@ -58,8 +59,7 @@ public ContractAgreementApiController(Monitor monitor, ContractAgreementService
public List<ContractAgreementDto> getAllAgreements(@Valid @BeanParam QuerySpecDto querySpecDto) {
var result = transformerRegistry.transform(querySpecDto, QuerySpec.class);
if (result.failed()) {
monitor.warning("Error transforming QuerySpec: " + String.join(", ", result.getFailureMessages()));
throw new IllegalArgumentException("Cannot transform QuerySpecDto object");
throw new InvalidRequestException(result.getFailureMessages());
}

var spec = result.getContent();
Expand Down
Loading