Skip to content

fix: allowing sorting Contract Definitions by creation date #5020

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.

Sign up for GitHub

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

fix: allowing sorting Contract Definitions by creation date #5020

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a135a72
allowing sorting Contract Definitions by creation date.
bmg13 May 30, 2025
86c621c
Merge branch 'main' of github.com:eclipse-edc/Connector into fix/allo…
bmg13 Jun 3, 2025
bf44068
Added ContractDefiniton createdAt tests in multiple flows.
bmg13 Jun 3, 2025
af5b9bd
Changes from PR.
bmg13 Jun 3, 2025
7172a21
Update sorting tests.
bmg13 Jun 6, 2025
8b49c1e
Update sorting tests.
bmg13 Jun 8, 2025
c5cee4e
Add api Contract Definition sort by create date.
bmg13 Jun 9, 2025
54d5d0a
Improve E2E test.
bmg13 Jun 9, 2025
4f5ec4d
Fix some tests.
bmg13 Jun 9, 2025
c957151
Changes from PR.
bmg13 Jun 9, 2025
72e1c59
Merge branch 'main' of github.com:eclipse-edc/Connector into fix/allo…
bmg13 Jun 9, 2025
c3f9e1c
Changes from PR.
bmg13 Jun 9, 2025
6456ff3
Small test update.
bmg13 Jun 9, 2025
6d8995b
Merge branch 'main' of github.com:eclipse-edc/Connector into fix/allo…
bmg13 Jun 12, 2025
52d6903
Improve Test Format.
bmg13 Jun 13, 2025
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
1 change: 1 addition & 0 deletions .../controlplane/store/sql/contractdefinition/schema/postgres/ContractDefinitionMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
public class ContractDefinitionMapping extends TranslationMapping {
public ContractDefinitionMapping(ContractDefinitionStatements statements) {
add("id", statements.getIdColumn());
add("createdAt", statements.getCreatedAtColumn());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated/Adde tests regarding ContractDefinition's createdAt, but no UT related with that specific block, since similar mappings do not seem to have them. Would you propose to create a ContractDefinitionMappingTest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests you provided do not represent the behavior that you described as failing in the attached issue. Please write a test that checks if the result of a contract definition api request is correctly ordered by creation date, when the request has a query spec contains such an ordering criteria.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added suggested test. Keep in mind that the createdAt field is not returned, so I enforced the result extraction as a list to keep the order and validate it with the return ids.

add("accessPolicyId", statements.getAccessPolicyIdColumn());
add("accessPolicy", statements.getAccessPolicyIdColumn());
add("contractPolicyId", statements.getContractPolicyIdColumn());
Expand Down
10 changes: 10 additions & 0 deletions ...r/controlplane/contract/spi/testfixtures/offer/store/ContractDefinitionStoreTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,16 @@ void verifySorting() {
assertThat(getContractDefinitionStore().findAll(QuerySpec.Builder.newInstance().sortField("id").sortOrder(SortOrder.DESC).build())).hasSize(10).isSortedAccordingTo((c1, c2) -> c2.getId().compareTo(c1.getId()));
}

@Test
void verifySortingCreatedAt() {
IntStream.range(0, 10).mapToObj(i -> createContractDefinition("id" + i)).forEach(getContractDefinitionStore()::save);

var querySpec = QuerySpec.Builder.newInstance().sortField("createdAt");

assertThat(getContractDefinitionStore().findAll(querySpec.sortOrder(SortOrder.ASC).build())).isSortedAccordingTo(Comparator.comparing(ContractDefinition::getCreatedAt));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format this so the method calls appear on subsequent lines - it makes it easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in here.

assertThat(getContractDefinitionStore().findAll(querySpec.sortOrder(SortOrder.DESC).build())).isSortedAccordingTo((c1, c2) -> Long.compare(c2.getCreatedAt(), c1.getCreatedAt()));
}

@Test
void verifySorting_invalidProperty() {
IntStream.range(0, 10).mapToObj(i -> createContractDefinition("id" + i)).forEach(getContractDefinitionStore()::save);
Expand Down
1 change: 1 addition & 0 deletions ...lipse/edc/connector/controlplane/contract/spi/testfixtures/offer/store/TestFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public static ContractDefinition createContractDefinition(String id, String acce
.id(id)
.accessPolicyId(accessPolicyId)
.contractPolicyId(contractPolicyId)
.createdAt(1234)
.build();
}

Expand Down
47 changes: 47 additions & 0 deletions ...c/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@

package org.eclipse.edc.test.e2e.managementapi;

import com.fasterxml.jackson.core.JsonProcessingException;
import jakarta.json.JsonArray;
import jakarta.json.JsonObject;
import jakarta.json.JsonObjectBuilder;
import jakarta.json.JsonValue;
import org.eclipse.edc.connector.controlplane.contract.spi.offer.store.ContractDefinitionStore;
import org.eclipse.edc.connector.controlplane.contract.spi.types.offer.ContractDefinition;
import org.eclipse.edc.jsonld.util.JacksonJsonLd;
import org.eclipse.edc.junit.annotations.EndToEndTest;
import org.eclipse.edc.junit.annotations.PostgresqlIntegrationTest;
import org.eclipse.edc.sql.testfixtures.PostgresqlEndToEndExtension;
Expand All @@ -28,7 +31,11 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.RegisterExtension;

import java.util.LinkedHashMap;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;

import static io.restassured.http.ContentType.JSON;
import static jakarta.json.Json.createArrayBuilder;
Expand Down Expand Up @@ -117,6 +124,46 @@ void queryPolicyDefinitionWithSimplePrivateProperties(ManagementEndToEndTestCont
.body("size()", is(0));
}

@Test
void queryContractDefinitions_sortByCreatedDate(ManagementEndToEndTestContext context, ContractDefinitionStore store) throws JsonProcessingException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you doing this in an end-to-end test? This should be verified by individual unit tests and an integration test for Postgres. End-to-end tests should only verify complete codepaths and should not be used to verify individual functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning is due to the raised issue was not having the ordered response and with this test that can be confirmed, being a PostgresqlIntegrationTest.
Similar to what found in here for TransferProcesss.
If the end result was just the existence of a field, it would not make much sense, but having the order on the final response I believe it adds value.
What if I update this test to be queryContractDefinitions_withQuerySpec since it is broader and is not currently covered?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see verifySortingCreatedAt covers the case for all store implementations. I don't understand the comment:

What if I update this test to be queryContractDefinitions_withQuerySpec since it is broader and is not currently covered?

Is this test intended to verify that ordering is preserved end-to-end, or does it do something else?

var id1 = UUID.randomUUID().toString();
var id2 = UUID.randomUUID().toString();
var id3 = UUID.randomUUID().toString();
var createdAtTime = 1000L;
var increment = new AtomicInteger(0);
Stream.of(id1, id2, id3).forEach(id -> store.save(createContractDefinition(id).createdAt(createdAtTime + increment.getAndIncrement()).build()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use an AtomicLong and have it as one operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, simplified in here.


var content = """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a JSON string created and parsed into a query object like this. The query object should be created in code directly.

{
"@context": {
"@vocab": "https://w3id.org/edc/v0.0.1/ns/"
},
"@type": "QuerySpec",
"sortField": "createdAt",
"sortOrder": "DESC",
"limit": 100,
"offset": 0
}
""";
var query = JacksonJsonLd.createObjectMapper()
.readValue(content, JsonObject.class);

var result = context.baseRequest()
.contentType(JSON)
.body(query)
.post("/v3/contractdefinitions/request")
.then()
.log().ifError()
.statusCode(200)
.body("size()", is(3))
.extract()
.as(List.class);

assertThat(result)
.extracting(cd -> ((LinkedHashMap<?, ?>) cd).get(ID))
.containsExactlyElementsOf(List.of(id3, id2, id1));
}

@Test
void shouldCreateAndRetrieve(ManagementEndToEndTestContext context, ContractDefinitionStore store) {
var id = UUID.randomUUID().toString();
Expand Down
Loading