Skip to content

Commit

Permalink
feat: Remove duplicate information in the initiate negotiation api re…
Browse files Browse the repository at this point in the history 
…quest (#3605)

* Remove duplicate information in the initiate negotiation api request without icreasing the api version.

* Fix unit test.

* Fix review findings.

* Move logs to validator.

* Move logs to validator.

* Adapt ContractRequestSchema

* Update extensions/control-plane/api/management-api/contract-negotiation-api/src/main/java/org/eclipse/edc/connector/api/management/contractnegotiation/ContractNegotiationApi.java

Co-authored-by: ndr_brt <[email protected]>

* Fix Checkstyle

* Solve conflict

---------

Co-authored-by: ndr_brt <[email protected]>
  • Loading branch information
@tuncaytunc-zf @ndr-brt
tuncaytunc-zf and ndr-brt authored Nov 16, 2023
1 parent dd99058 commit 8c3013a
Show file tree
Hide file tree
Showing 11 changed files with 170 additions and 51 deletions.
23 changes: 11 additions & 12 deletions .../org/eclipse/edc/connector/api/management/contractnegotiation/ContractNegotiationApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ record ContractRequestSchema(
String counterPartyAddress,
@Schema(requiredMode = REQUIRED)
String providerId,
@Deprecated(since = "0.3.2")
@Schema(deprecated = true, description = "please use policy instead of offer")
ContractOfferDescriptionSchema offer,
ManagementApiSchema.PolicySchema policy,
List<ManagementApiSchema.CallbackAddressSchema> callbackAddresses) {

// policy example took from https://w3c.github.io/odrl/bp/
Expand All @@ -145,18 +148,14 @@ record ContractRequestSchema(
"counterPartyAddress": "http://provider-address",
"protocol": "dataspace-protocol-http",
"providerId": "provider-id",
"offer": {
"offerId": "offer-id",
"assetId": "asset-id",
"policy": {
"@context": "http://www.w3.org/ns/odrl.jsonld",
"@type": "Set",
"@id": "offer-id",
"permission": [{
"target": "asset-id",
"action": "display"
}]
}
"policy": {
"@context": "http://www.w3.org/ns/odrl.jsonld",
"@type": "Set",
"@id": "policy-id",
"permission": [],
"prohibition": [],
"obligation": [],
"target": "assetId"
},
"callbackAddresses": [{
"transactional": false,
Expand Down
8 changes: 4 additions & 4 deletions ...pse/edc/connector/api/management/contractnegotiation/ContractNegotiationApiExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,17 @@ public String name() {
@Override
public void initialize(ServiceExtensionContext context) {
var factory = Json.createBuilderFactory(Map.of());
transformerRegistry.register(new JsonObjectToContractRequestTransformer());
var monitor = context.getMonitor();

transformerRegistry.register(new JsonObjectToContractRequestTransformer(monitor));
transformerRegistry.register(new JsonObjectToContractOfferDescriptionTransformer());
transformerRegistry.register(new JsonObjectToTerminateNegotiationCommandTransformer());
transformerRegistry.register(new JsonObjectFromContractNegotiationTransformer(factory));
transformerRegistry.register(new JsonObjectFromNegotiationStateTransformer(factory));

validatorRegistry.register(CONTRACT_REQUEST_TYPE, ContractRequestValidator.instance(context.getMonitor()));
validatorRegistry.register(CONTRACT_REQUEST_TYPE, ContractRequestValidator.instance(monitor));
validatorRegistry.register(TERMINATE_NEGOTIATION_TYPE, TerminateNegotiationValidator.instance());

var monitor = context.getMonitor();

var controller = new ContractNegotiationApiController(service, transformerRegistry, monitor, validatorRegistry);
webService.registerResource(config.getContextAlias(), controller);
}
Expand Down
1 change: 1 addition & 0 deletions ...ipse/edc/connector/api/management/contractnegotiation/model/ContractOfferDescription.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import static org.eclipse.edc.spi.CoreConstants.EDC_NAMESPACE;

@Deprecated(since = "0.3.2")
public class ContractOfferDescription {

public static final String CONTRACT_OFFER_DESCRIPTION_TYPE = EDC_NAMESPACE + "ContractOfferDescription";
Expand Down
2 changes: 1 addition & 1 deletion ...gement/contractnegotiation/transform/JsonObjectToContractOfferDescriptionTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import static org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription.OFFER_ID;
import static org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription.POLICY;


@Deprecated(since = "0.3.2")
public class JsonObjectToContractOfferDescriptionTransformer extends AbstractJsonLdTransformer<JsonObject, ContractOfferDescription> {

public JsonObjectToContractOfferDescriptionTransformer() {
Expand Down
31 changes: 23 additions & 8 deletions .../api/management/contractnegotiation/transform/JsonObjectToContractRequestTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription;
import org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest;
import org.eclipse.edc.jsonld.spi.transformer.AbstractJsonLdTransformer;
import org.eclipse.edc.policy.model.Policy;
import org.eclipse.edc.spi.monitor.Monitor;
import org.eclipse.edc.spi.types.domain.callback.CallbackAddress;
import org.eclipse.edc.spi.types.domain.offer.ContractOffer;
import org.eclipse.edc.transform.spi.TransformerContext;
Expand All @@ -30,13 +32,17 @@
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.CONNECTOR_ADDRESS;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.OFFER;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.POLICY;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.PROTOCOL;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.PROVIDER_ID;

public class JsonObjectToContractRequestTransformer extends AbstractJsonLdTransformer<JsonObject, ContractRequest> {

public JsonObjectToContractRequestTransformer() {
private final Monitor monitor;

public JsonObjectToContractRequestTransformer(Monitor monitor) {
super(JsonObject.class, ContractRequest.class);
this.monitor = monitor;
}

@Override
Expand All @@ -46,13 +52,22 @@ public JsonObjectToContractRequestTransformer() {
.counterPartyAddress(counterPartyAddressOrConnectorAddress(jsonObject, context))
.protocol(transformString(jsonObject.get(PROTOCOL), context));

var contractOfferDescription = transformObject(jsonObject.get(OFFER), ContractOfferDescription.class, context);
var contractOffer = ContractOffer.Builder.newInstance()
.id(contractOfferDescription.getOfferId())
.assetId(contractOfferDescription.getAssetId())
.policy(contractOfferDescription.getPolicy())
.build();
contractRequestBuilder.contractOffer(contractOffer);
var policyJson = jsonObject.get(POLICY);
if (policyJson != null) {
var policy = transformObject(jsonObject.get(POLICY), Policy.class, context);
contractRequestBuilder.policy(policy);
}

var offerJson = jsonObject.get(OFFER);
if (offerJson != null) {
var contractOfferDescription = transformObject(jsonObject.get(OFFER), ContractOfferDescription.class, context);
var contractOffer = ContractOffer.Builder.newInstance()
.id(contractOfferDescription.getOfferId())
.assetId(contractOfferDescription.getAssetId())
.policy(contractOfferDescription.getPolicy())
.build();
contractRequestBuilder.contractOffer(contractOffer);
}

var callbackAddress = jsonObject.get(CALLBACK_ADDRESSES);
if (callbackAddress != null) {
Expand Down
36 changes: 29 additions & 7 deletions ...edc/connector/api/management/contractnegotiation/validation/ContractRequestValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,38 +15,60 @@
package org.eclipse.edc.connector.api.management.contractnegotiation.validation;

import jakarta.json.JsonObject;
import org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription;
import org.eclipse.edc.spi.monitor.Monitor;
import org.eclipse.edc.validator.jsonobject.JsonLdPath;
import org.eclipse.edc.validator.jsonobject.JsonObjectValidator;
import org.eclipse.edc.validator.jsonobject.validators.MandatoryObject;
import org.eclipse.edc.validator.jsonobject.validators.MandatoryValue;
import org.eclipse.edc.validator.spi.ValidationResult;
import org.eclipse.edc.validator.spi.Validator;
import org.eclipse.edc.validator.spi.Violation;

import static java.lang.String.format;
import static org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription.ASSET_ID;
import static org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription.OFFER_ID;
import static org.eclipse.edc.connector.api.management.contractnegotiation.model.ContractOfferDescription.POLICY;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.CONNECTOR_ADDRESS;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.CONTRACT_REQUEST_TYPE;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.OFFER;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.POLICY;
import static org.eclipse.edc.connector.contract.spi.types.negotiation.ContractRequest.PROTOCOL;

public class ContractRequestValidator {

public static Validator<JsonObject> instance(Monitor monitor) {
return JsonObjectValidator.newValidator()
.verify(path -> new MandatoryCounterPartyAddressOrConnectorAddress(path, monitor))
.verify(PROTOCOL, MandatoryValue::new)
.verify(OFFER, MandatoryObject::new)
.verifyObject(OFFER, v -> v
.verify(OFFER_ID, MandatoryValue::new)
.verify(ASSET_ID, MandatoryValue::new)
.verify(POLICY, MandatoryObject::new)
)
.verify(path -> new MandatoryOfferOrPolicy(path, monitor))
.build();
}

private record MandatoryOfferOrPolicy(JsonLdPath path, Monitor monitor) implements Validator<JsonObject> {
@Override
public ValidationResult validate(JsonObject input) {
var offerValidity = new MandatoryObject(path.append(OFFER)).validate(input);
if (offerValidity.succeeded()) {
monitor.warning(format("The attribute %s has been deprecated in type %s, please use %s",
OFFER, CONTRACT_REQUEST_TYPE, POLICY));
return JsonObjectValidator.newValidator()
.verifyObject(OFFER, v -> v
.verify(OFFER_ID, MandatoryValue::new)
.verify(ASSET_ID, MandatoryValue::new)
.verify(ContractOfferDescription.POLICY, MandatoryObject::new)
).build().validate(input);
}

var policyValidity = new MandatoryObject(path.append(POLICY)).validate(input);
if (policyValidity.succeeded()) {
return ValidationResult.success();
}

return ValidationResult.failure(Violation.violation(format("'%s' or '%s' must not be empty", OFFER, POLICY), path.toString()));
}
}

/**
* This custom validator can be removed once `connectorAddress` is deleted and exists only for legacy reasons
*/
Expand Down
35 changes: 34 additions & 1 deletion ...dc/connector/api/management/contractnegotiation/ContractNegotiationApiControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ void getSingleContractNegotiationAgreement_whenNoneFound() {
}

@Test
void initiate() {
void initiate_with_contractOffer() {
when(validatorRegistry.validate(any(), any())).thenReturn(ValidationResult.success());
var contractNegotiation = createContractNegotiation("cn1");
var responseBody = createObjectBuilder().add(TYPE, ID_RESPONSE_TYPE).add(ID, contractNegotiation.getId()).build();
Expand Down Expand Up @@ -363,6 +363,39 @@ void initiate() {
verifyNoMoreInteractions(transformerRegistry, service);
}

@Test
void initiate_with_policy() {
when(validatorRegistry.validate(any(), any())).thenReturn(ValidationResult.success());
var contractNegotiation = createContractNegotiation("cn1");
var responseBody = createObjectBuilder().add(TYPE, ID_RESPONSE_TYPE).add(ID, contractNegotiation.getId()).build();

when(transformerRegistry.transform(any(JsonObject.class), eq(ContractRequest.class))).thenReturn(Result.success(
ContractRequest.Builder.newInstance()
.protocol("test-protocol")
.providerId("test-provider-id")
.counterPartyAddress("test-cb")
.policy(Policy.Builder.newInstance().build())
.build()));

when(transformerRegistry.transform(any(), eq(JsonObject.class))).thenReturn(Result.success(responseBody));
when(service.initiateNegotiation(any(ContractRequest.class))).thenReturn(contractNegotiation);

when(transformerRegistry.transform(any(IdResponse.class), eq(JsonObject.class))).thenReturn(Result.success(responseBody));

baseRequest()
.contentType(JSON)
.body(createObjectBuilder().build())
.post()
.then()
.statusCode(200)
.body(ID, is(contractNegotiation.getId()));

verify(service).initiateNegotiation(any());
verify(transformerRegistry).transform(any(JsonObject.class), eq(ContractRequest.class));
verify(transformerRegistry).transform(any(IdResponse.class), eq(JsonObject.class));
verifyNoMoreInteractions(transformerRegistry, service);
}

@Test
void initiate_shouldReturnBadRequest_whenValidationFails() {
when(validatorRegistry.validate(any(), any())).thenReturn(ValidationResult.failure(violation("error", "path")));
Expand Down
5 changes: 3 additions & 2 deletions .../eclipse/edc/connector/api/management/contractnegotiation/ContractNegotiationApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,11 @@ class ContractNegotiationApiTest {
private final ObjectMapper objectMapper = JacksonJsonLd.createObjectMapper();
private final JsonLd jsonLd = new JsonLdExtension().createJsonLdService(testServiceExtensionContext());
private final TypeTransformerRegistry transformer = new TypeTransformerRegistryImpl();
private final Monitor monitor = mock();

@BeforeEach
void setUp() {
transformer.register(new JsonObjectToContractRequestTransformer());
transformer.register(new JsonObjectToContractRequestTransformer(monitor));
transformer.register(new JsonObjectToContractOfferDescriptionTransformer());
transformer.register(new JsonObjectToCallbackAddressTransformer());
transformer.register(new JsonObjectToTerminateNegotiationCommandTransformer());
Expand All @@ -64,7 +65,7 @@ void setUp() {

@Test
void contractRequestExample() throws JsonProcessingException {
var validator = ContractRequestValidator.instance(mock(Monitor.class));
var validator = ContractRequestValidator.instance(monitor);

var jsonObject = objectMapper.readValue(CONTRACT_REQUEST_EXAMPLE, JsonObject.class);
assertThat(jsonObject).isNotNull();
Expand Down
3 changes: 2 additions & 1 deletion .../management/contractnegotiation/transform/JsonObjectToContractRequestTransformerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ class JsonObjectToContractRequestTransformerTest {
private final JsonLd jsonLd = new TitaniumJsonLd(mock(Monitor.class));
private final TransformerContext context = mock();
private JsonObjectToContractRequestTransformer transformer;
private final Monitor monitor = mock();

@BeforeEach
void setUp() {
transformer = new JsonObjectToContractRequestTransformer();
transformer = new JsonObjectToContractRequestTransformer(monitor);
}

@Test
Expand Down
57 changes: 43 additions & 14 deletions ...connector/api/management/contractnegotiation/validation/ContractRequestValidatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,47 +65,76 @@ void shouldSuccess_whenObjectIsValid() {
}

@Test
void shouldFail_whenMandatoryPropertiesAreMissing() {
var input = Json.createObjectBuilder().build();
void shouldFail_whenOfferMandatoryPropertiesAreMissing() {
var input = Json.createObjectBuilder()
.add(CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS, value("http://connector-address"))
.add(CONNECTOR_ADDRESS, value("http://connector-address"))
.add(PROTOCOL, value("protocol"))
.add(PROVIDER_ID, value("connector-id"))
.add(OFFER, createArrayBuilder().add(createObjectBuilder()))
.build();

var result = validator.validate(input);

assertThat(result).isFailed().extracting(ValidationFailure::getViolations).asInstanceOf(list(Violation.class))
.isNotEmpty()
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS))
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(PROTOCOL))
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(OFFER));
.anySatisfy(violation -> assertThat(violation.path()).contains(OFFER_ID))
.anySatisfy(violation -> assertThat(violation.path()).contains(ASSET_ID))
.anySatisfy(violation -> assertThat(violation.path()).contains(POLICY));
}

@Test
void shouldFail_whenOfferMandatoryPropertiesAreMissing() {
void shouldFail_whenOfferAndPolicyAreMissing() {
var input = Json.createObjectBuilder()
.add(CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS, value("http://connector-address"))
.add(PROTOCOL, value("protocol"))
.add(PROVIDER_ID, value("connector-id"))
.add(OFFER, createArrayBuilder().add(createObjectBuilder()))
.build();

var result = validator.validate(input);

assertThat(result).isFailed().extracting(ValidationFailure::getViolations).asInstanceOf(list(Violation.class))
.isNotEmpty()
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(OFFER + "/" + OFFER_ID))
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(OFFER + "/" + ASSET_ID))
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(OFFER + "/" + POLICY));
.anySatisfy(violation -> assertThat(violation.message()).contains(OFFER))
.anySatisfy(violation -> assertThat(violation.message()).contains(POLICY));
}

@Test
void shouldSucceed_whenDeprecatedConnectorAddressIsUsed() {
void shouldFail_whenMandatoryPropertiesAreMissing() {
var input = Json.createObjectBuilder().build();

var result = validator.validate(input);

assertThat(result).isFailed().extracting(ValidationFailure::getViolations).asInstanceOf(list(Violation.class))
.isNotEmpty()
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS))
.anySatisfy(violation -> assertThat(violation.path()).isEqualTo(PROTOCOL));
}

@Test
void shouldSucceed_whenDeprecatedOfferIsUsed() {
var input = Json.createObjectBuilder()
.add(CONNECTOR_ADDRESS, value("http://connector-address"))
.add(CONTRACT_REQUEST_COUNTER_PARTY_ADDRESS, value("http://connector-address"))
.add(PROTOCOL, value("protocol"))
.add(PROVIDER_ID, value("connector-id"))
.add(OFFER, createArrayBuilder().add(createObjectBuilder()
.add(OFFER_ID, value("offerId"))
.add(ASSET_ID, value("offerId"))
.add(POLICY, createArrayBuilder().add(createObjectBuilder()))
))
.add(POLICY, createArrayBuilder().add(createObjectBuilder()))))
.build();

var result = validator.validate(input);
assertThat(result).isSucceeded();
verify(monitor).warning(anyString());
}

@Test
void shouldSucceed_whenDeprecatedConnectorAddressIsUsed() {
var input = Json.createObjectBuilder()
.add(CONNECTOR_ADDRESS, value("http://connector-address"))
.add(PROTOCOL, value("protocol"))
.add(PROVIDER_ID, value("connector-id"))
.add(POLICY, createArrayBuilder().add(createObjectBuilder()))
.build();

var result = validator.validate(input);
Expand Down
Loading

0 comments on commit 8c3013a

Please sign in to comment.