fix: allowing sorting Contract Definitions by creation date #5020

bmg13 · 2025-05-30T13:18:36Z

What this PR changes/adds

Include fix to enable retrieving Contract Definitions sorted by their creation date. Simply updated the mapping for this entity.

Why it does that

We should be able to sort Contract Definitions by their creation date, similar to what is seen with other entities (like assets and policies).

Linked Issue(s)

Closes #4921

paullatzelsperger · 2025-06-03T05:51:58Z

...tor/controlplane/store/sql/contractdefinition/schema/postgres/ContractDefinitionMapping.java

@@ -26,6 +26,7 @@
 public class ContractDefinitionMapping extends TranslationMapping {
    public ContractDefinitionMapping(ContractDefinitionStatements statements) {
        add("id", statements.getIdColumn());
+        add("createdAt", statements.getCreatedAtColumn());


there should be a test for this.

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?

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.

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.

…w_sort_contract_definition_with_created_time

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java

jimmarino · 2025-06-09T14:45:27Z

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java

+            var id1 = UUID.randomUUID().toString();
+            var id2 = UUID.randomUUID().toString();
+            var id3 = UUID.randomUUID().toString();
+            Stream.of(id1, id2, id3).forEach(id -> store.save(createContractDefinition(id).createdAt(System.nanoTime()).build()));


Please don't do this. Start with a time and then increment using an offset so it is deterministic.

followed suggestion, changed in here.

…w_sort_contract_definition_with_created_time

jimmarino · 2025-06-09T15:53:21Z

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java

-            Stream.of(id1, id2, id3).forEach(id -> store.save(createContractDefinition(id).createdAt(System.nanoTime()).build()));
+            var createdAtTime = 1000L;
+            var increment = new AtomicInteger(0);
+            Stream.of(id1, id2, id3).forEach(id -> store.save(createContractDefinition(id).createdAt(createdAtTime + increment.getAndIncrement()).build()));


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

You're right, simplified in here.

jimmarino · 2025-06-11T17:01:40Z

...ctor/controlplane/contract/spi/testfixtures/offer/store/ContractDefinitionStoreTestBase.java

+
+            var querySpec = QuerySpec.Builder.newInstance().sortField("createdAt");
+
+            assertThat(getContractDefinitionStore().findAll(querySpec.sortOrder(SortOrder.ASC).build())).isSortedAccordingTo(Comparator.comparing(ContractDefinition::getCreatedAt));


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

Done in here.

jimmarino · 2025-06-11T17:05:36Z

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java

@@ -117,6 +124,45 @@ void queryPolicyDefinitionWithSimplePrivateProperties(ManagementEndToEndTestCont
                    .body("size()", is(0));
        }

+        @Test
+        void queryContractDefinitions_sortByCreatedDate(ManagementEndToEndTestContext context, ContractDefinitionStore store) throws JsonProcessingException {


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.

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?

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?

…w_sort_contract_definition_with_created_time

ndr-brt · 2025-06-20T08:47:43Z

@jimmarino could you review it again? I'd be ready to merge this

jimmarino · 2025-06-20T09:57:36Z

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java

@@ -117,6 +124,45 @@ void queryPolicyDefinitionWithSimplePrivateProperties(ManagementEndToEndTestCont
                    .body("size()", is(0));
        }

+        @Test
+        void queryContractDefinitions_sortByCreatedDate(ManagementEndToEndTestContext context, ContractDefinitionStore store) throws JsonProcessingException {


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?

jimmarino · 2025-06-20T09:57:48Z

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java

+            var createdAtTime = new AtomicLong(1000L);
+            Stream.of(id1, id2, id3).forEach(id -> store.save(createContractDefinition(id).createdAt(createdAtTime.getAndIncrement()).build()));
+
+            var content = """


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

bmg13 requested a review from paullatzelsperger as a code owner May 30, 2025 13:18

bmg13 self-assigned this May 30, 2025

bmg13 added the bug Something isn't working label May 30, 2025

allowing sorting Contract Definitions by creation date.

a135a72

bmg13 force-pushed the fix/allow_sort_contract_definition_with_created_time branch from 1ef851a to a135a72 Compare May 30, 2025 13:34

paullatzelsperger reviewed Jun 3, 2025

View reviewed changes

bmg13 added 3 commits June 3, 2025 10:06

Merge branch 'main' of github.com:eclipse-edc/Connector into fix/allo…

86c621c

…w_sort_contract_definition_with_created_time

Added ContractDefiniton createdAt tests in multiple flows.

bf44068

Changes from PR.

af5b9bd

bmg13 requested a review from paullatzelsperger June 3, 2025 15:10

Update sorting tests.

7172a21

bmg13 requested a review from ndr-brt as a code owner June 8, 2025 08:40

bmg13 added 4 commits June 8, 2025 09:57

Update sorting tests.

8b49c1e

Add api Contract Definition sort by create date.

c5cee4e

Improve E2E test.

54d5d0a

Fix some tests.

4f5ec4d

bmg13 requested a review from rafaelmag110 June 9, 2025 13:23

ndr-brt approved these changes Jun 9, 2025

View reviewed changes

.../src/test/java/org/eclipse/edc/test/e2e/managementapi/ContractDefinitionApiEndToEndTest.java Outdated Show resolved Hide resolved

Changes from PR.

c957151

jimmarino reviewed Jun 9, 2025

View reviewed changes

bmg13 added 2 commits June 9, 2025 16:27

Merge branch 'main' of github.com:eclipse-edc/Connector into fix/allo…

72e1c59

…w_sort_contract_definition_with_created_time

Changes from PR.

c3f9e1c

jimmarino reviewed Jun 9, 2025

View reviewed changes

Small test update.

6456ff3

bmg13 requested a review from jimmarino June 11, 2025 16:58

jimmarino requested changes Jun 11, 2025

View reviewed changes

Merge branch 'main' of github.com:eclipse-edc/Connector into fix/allo…

6d8995b

…w_sort_contract_definition_with_created_time

bmg13 requested a review from jimmarino June 12, 2025 15:17

Improve Test Format.

52d6903

jimmarino requested changes Jun 20, 2025

View reviewed changes


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

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

fix: allowing sorting Contract Definitions by creation date #5020

Are you sure you want to change the base?

fix: allowing sorting Contract Definitions by creation date #5020

Uh oh!

Conversation

bmg13 commented May 30, 2025

What this PR changes/adds

Why it does that

Linked Issue(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimmarino Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndr-brt commented Jun 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimmarino Jun 9, 2025 •

edited

Loading