feat: Add SQL Agreement Retirement Store #1631

bmg13 · 2024-10-17T11:40:32Z

WHAT

Adds a SQL Agreement Retirement Store and edc_agreement_retirement table in migration script.

WHY

Provide a SQL store besides the InMemory default one added in previous PR.

FURTHER NOTES

List other areas of code that have changed but are not necessarily linked to the main feature. This could be method signature changes, package declarations, bugs that were encountered and were fixed inline, etc.

In accordance with DR: #1604
Related with: #1615
Addresses eclipse-tractusx/sig-release#777

…usx-edc into feat/add_sql_agreements_retirement_store

rafaelmag110 · 2024-10-17T12:19:48Z

settings.gradle.kts

@@ -56,6 +56,7 @@ include(":edc-extensions:agreements")
 include(":edc-extensions:agreements:retirement-evaluation-core")
 include(":edc-extensions:agreements:retirement-evaluation-api")
 include(":edc-extensions:agreements:retirement-evaluation-spi")
+include("edc-extensions:agreements:retirement-evaluation-store-sql")


Suggested change

include("edc-extensions:agreements:retirement-evaluation-store-sql")

include(":edc-extensions:agreements:retirement-evaluation-store-sql")

got a flashback, updated here.

rafaelmag110 · 2024-10-17T12:50:33Z

...a/org/eclipse/tractusx/edc/agreements/retirement/store/sql/SqlAgreementsRetirementStore.java

+        return AgreementsRetirementEntry.Builder.newInstance()
+                .withAgreementId(resultSet.getString(statements.getIdColumn()))
+                .withReason(resultSet.getString(statements.getReasonColumn()))
+                .withAgreementRetirementDate(Long.parseLong(resultSet.getString(statements.getRetirementDateColumn())))


maybe a result.getLong() directly?

yeah, good point, changed here.

rafaelmag110 · 2024-10-17T12:55:50Z

.../eclipse/tractusx/edc/agreements/retirement/store/SqlAgreementsRetirementStoreExtension.java

+    private static final String NAME = "SQL Agreement Retirement Store.";
+
+    @Setting(value = "Datasource name for the SQL AgreementsRetirement store")
+    private static final String DATASOURCE_SETTING_NAME = "edc.datasource.agreementretirement.name";


This setting was deprecated in favor of edc.sql.store.<name>.datasource

good point, updated to "tx.edc.sql.store.agreementretirement.datasource" since this is TX. updated here.

Just use an edc. prefix instead of the tx. one. I sense this could lead to confusion.

rafaelmag110 · 2024-10-17T13:16:19Z

...x/edc/postgresql/migration/contractnegotiation/V0_0_10__Init_ContractAgreementRetirement.sql

+        CONSTRAINT contract_negotiation_lease_lease_id_fk REFERENCES edc_lease ON DELETE SET NULL
+);
+
+CREATE TABLE IF NOT EXISTS edc_agreement_retirement


Having this here rings a bells for me. Couldn't we have just a file with the creation of the edc_agreement_retirement table?

Removed not needed additions I had here, but the create not exists of the edc_contract_agreement maintains because it is referenced. Maybe it can be referenced from other file, but based on what I saw here it should be added as well.

this is already created in the respective context

Removed in here.

rafaelmag110

Just some details but generally looks good.

rafaelmag110 · 2024-10-17T13:21:46Z

...a/org/eclipse/tractusx/edc/agreements/retirement/store/sql/SqlAgreementsRetirementStore.java

+                if (existsById(entry.getAgreementId(), connection)) {
+                    return StoreResult.alreadyExists(ALREADY_EXISTS_TEMPLATE.formatted(entry.getAgreementId()));
+                }
+                insert(connection, entry);


Because of the constraint on the edc_contract_agreement table, this might throw what I believe will be an uncaught exception.

that is valid. looking into SqlQueryExecutor.execute() we can confirm the already handled and throw of EdcPersistenceException to the client, so here catching EdcPersistenceException to throw it once more seems redundant.

we shouldn't rely on fk constraint, consistency should be maintained at the service layer

I assumed that since it depends on the existence of the fk then it could be controlled here to keep consistency.
Removed in here.

...x/edc/postgresql/migration/contractnegotiation/V0_0_10__Init_ContractAgreementRetirement.sql

ndr-brt · 2024-10-18T09:59:28Z

...x/edc/postgresql/migration/contractnegotiation/V0_0_10__Init_ContractAgreementRetirement.sql

+        CONSTRAINT contract_negotiation_lease_lease_id_fk REFERENCES edc_lease ON DELETE SET NULL
+);
+
+CREATE TABLE IF NOT EXISTS edc_agreement_retirement


this is already created in the respective context

...x/edc/postgresql/migration/contractnegotiation/V0_0_10__Init_ContractAgreementRetirement.sql

edc-extensions/agreements/retirement-evaluation-store-sql/docs/schema.sql

ndr-brt · 2024-10-18T10:02:06Z

...a/org/eclipse/tractusx/edc/agreements/retirement/store/sql/SqlAgreementsRetirementStore.java

+                if (existsById(entry.getAgreementId(), connection)) {
+                    return StoreResult.alreadyExists(ALREADY_EXISTS_TEMPLATE.formatted(entry.getAgreementId()));
+                }
+                insert(connection, entry);


we shouldn't rely on fk constraint, consistency should be maintained at the service layer

… into feat/add_sql_agreements_retirement_store

.../org/eclipse/tractusx/edc/agreements/retirement/service/AgreementsRetirementServiceImpl.java

edc-extensions/agreements/retirement-evaluation-store-sql/build.gradle.kts

edc-extensions/agreements/retirement-evaluation-store-sql/docs/schema.sql

.../eclipse/tractusx/edc/agreements/retirement/store/SqlAgreementsRetirementStoreExtension.java

...g/eclipse/tractusx/edc/agreements/retirement/store/sql/SqlAgreementsRetirementStoreTest.java

...sx/edc/postgresql/migration/agreementretirement/V0_0_1__Init_ContractAgreementRetirement.sql

ndr-brt · 2024-10-22T12:39:03Z

...rc/testFixtures/java/org/eclipse/tractusx/edc/tests/participant/TractusxParticipantBase.java

@@ -168,7 +168,7 @@ public void storeBusinessPartner(String bpn, String... groups) {
                .statusCode(204);
    }

-    public void retireProviderAgreement(String agreementId) {
+    public void retireProviderAgreement(String agreementId, int expectedStatusCode) {


I'm not a big fan of this, because it will make the test less explicit, please return directly the status code or, better, the ValidatableResponse object, on which the test can eventually add expectations or extracts body/headers.

I see, my goal was to reuse this logic and the test would have the responsability in its scope on what to expect/assert, but I see your point, reverted here.

ndr-brt · 2024-10-22T12:42:00Z

...s/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java

@@ -131,6 +131,12 @@ void retireAgreement_shouldCloseTransferProcesses() {

        }

+        @Test
+        @DisplayName("Verify non existing Contract Agreement is not stored in retired table.")


nit: the term "table" is really related only to the PG run of the tests, I would avoid to be that specific and only use hi level domain language, like:
void shouldFail_whenAgreementDoesNotExist()

note:

no need to specify retireAgreement, because this is the RetireAgreementTest class. Context is implicit

no need to use the @DisplayName, test name does already describe the purpose nicely.

thank you, that makes sense. changed it here, removing the @DisplayName.

ndr-brt · 2024-10-23T07:42:01Z

.../org/eclipse/tractusx/edc/agreements/retirement/service/AgreementsRetirementServiceImpl.java

@@ -75,4 +83,8 @@ private QuerySpec createFilterQueryByAgreementId(String agreementId) {
                                .build()
                ).build();
    }
+
+    private boolean contractAgreementExists(String agreementId) {


single liner method used once: can be inlined

changed in here.

ndr-brt · 2024-10-23T07:44:02Z

.../eclipse/tractusx/edc/agreements/retirement/store/SqlAgreementsRetirementStoreExtension.java

+public class SqlAgreementsRetirementStoreExtension implements ServiceExtension {
+
+    protected static final String NAME = "SQL Agreement Retirement Store.";
+    private static final String DEFAULT_DATASOURCE_NAME = "agreementretirement";


the default value is, according to line 63, DataSourceRegistry.DEFAULT_DATASOUCE

line 63 was kept here since is the same approach seen in the other datasources.

ndr-brt · 2024-10-23T08:24:16Z

...rc/testFixtures/java/org/eclipse/tractusx/edc/tests/participant/TractusxParticipantBase.java

@@ -183,6 +183,21 @@ public void retireProviderAgreement(String agreementId) {
                .statusCode(204);
    }

+    public void retireProviderAgreementBadRequest(String agreementId) {


please just have a single retireProviderAgreement method that returns either the state or the ValidatableResponse and let test explicitly add their expectations, as I pointed in the previous review

note: the name says "bad request" but the status code verified is 404

misunderstood, sorry, changed in here.
regarding the "note" you're right, should've been a notfound.

ndr-brt · 2024-10-23T08:27:39Z

...s/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java

@@ -131,6 +131,12 @@ void retireAgreement_shouldCloseTransferProcesses() {

        }

+        @Test
+        @DisplayName("Verify non existing Contract Agreement is not stored in retired table.")


please remove this @DisplayName as it's not providing any additional interesting information to the test name

was already removed in previous commit as indicated here.

ndr-brt · 2024-10-23T08:28:21Z

...s/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java

@@ -131,6 +131,12 @@ void retireAgreement_shouldCloseTransferProcesses() {

        }

+        @Test
+        @DisplayName("Verify non existing Contract Agreement is not stored in retired table.")
+        void retireAgreement_shouldFailIfTryingToRetireNonExistingId() {


the correct format is eventually:
retireAgreement_shouldFail_whenAgreementDoesNotExist

thank you, you're right, is more explicit. changed in here.

sonarqubecloud · 2024-10-23T08:57:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

rafaelmag110 and others added 30 commits October 8, 2024 10:25

Add agreement retirement function core

229f07f

Add RetirementApi and EndToEnd tests

5350b64

Improve EndToEnd tests

5486f53

Added AgreementsRetirementEntry and refactored code

24799aa

Adds API V3 interface and transformers from to and JsonObject

9f7fc5c

adds controller implementation and tests

cef2426

Refactored EndToEnd tests to use api

1eb9d38

refactored evaluation function logic to work with postValidators instead

d4cbdec

Headers

658a8a8

Checkstyle

01c04b3

Refactor pgruntime

ec17f40

Added docs

e0eb42f

adjusted namespaces

bb1e9b9

Feat: Add SQL Agreement Retirement Store.

0e670dd

Merge branch 'eclipse-tractusx:main' into agreements_retirement

9c94bee

Merge branch 'agreements_retirement' of github.com:rafaelmag110/tract…

a85ea85

…usx-edc into feat/add_sql_agreements_retirement_store

Add search retiredAgreements.

aa96043

Updated reading from sql store.

1b3e902

Run successfully with agreementId.

f285519

Fix create query with query spec.

810c2e3

Update sql schema.

dd68417

Add agreement retirement function core

c48171c

Add RetirementApi and EndToEnd tests

410a546

Improve EndToEnd tests

1d6649d

Added AgreementsRetirementEntry and refactored code

e6ec6b0

Adds API V3 interface and transformers from to and JsonObject

ec95637

adds controller implementation and tests

f7a3bcc

Refactored EndToEnd tests to use api

d7f399a

refactored evaluation function logic to work with postValidators instead

899402b

Headers

8fc2240

rafaelmag110 reviewed Oct 17, 2024

View reviewed changes

rafaelmag110 requested changes Oct 17, 2024

View reviewed changes

bmg13 marked this pull request as ready for review October 17, 2024 14:00

bmg13 added 2 commits October 17, 2024 15:33

Changes from PR.

e94343c

Changes from PR.

828f68c

bmg13 requested a review from rafaelmag110 October 17, 2024 15:00

ndr-brt reviewed Oct 18, 2024

View reviewed changes

bmg13 added 6 commits October 18, 2024 18:40

Changes from PR.

223e47d

Merge branch 'main' of https://github.com/eclipse-tractusx/tractusx-edc…

1c6f96e

… into feat/add_sql_agreements_retirement_store

Inject the ContractAgreementService.

7cf4919

Add ContractNegotiationService to AgreementsRetirementServiceImpl.

e7c3419

Fix unit test.

76a27c1

Change agreement_retirement script dir.

ff11487

bmg13 requested review from wolf4ood and ndr-brt October 22, 2024 10:00

ndr-brt requested changes Oct 22, 2024

View reviewed changes

Changes from PR.

9e9301a

bmg13 requested a review from ndr-brt October 22, 2024 16:08

ndr-brt requested changes Oct 23, 2024

View reviewed changes

Changes from PR.

b54f8c3

bmg13 requested a review from ndr-brt October 23, 2024 08:59

ndr-brt approved these changes Oct 23, 2024

View reviewed changes

rafaelmag110 approved these changes Oct 23, 2024

View reviewed changes

wolf4ood approved these changes Oct 23, 2024

View reviewed changes

wolf4ood merged commit 2662c42 into eclipse-tractusx:main Oct 23, 2024
33 checks passed

rafaelmag110 added the Feature label Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SQL Agreement Retirement Store #1631

feat: Add SQL Agreement Retirement Store #1631

bmg13 commented Oct 17, 2024

rafaelmag110 Oct 17, 2024

bmg13 Oct 17, 2024

rafaelmag110 Oct 17, 2024

bmg13 Oct 17, 2024

rafaelmag110 Oct 17, 2024

bmg13 Oct 17, 2024

rafaelmag110 Oct 18, 2024

rafaelmag110 Oct 17, 2024

bmg13 Oct 17, 2024

ndr-brt Oct 18, 2024

bmg13 Oct 18, 2024

rafaelmag110 left a comment

rafaelmag110 Oct 17, 2024

bmg13 Oct 17, 2024

ndr-brt Oct 18, 2024

bmg13 Oct 18, 2024

ndr-brt Oct 18, 2024

ndr-brt Oct 18, 2024

ndr-brt Oct 22, 2024

bmg13 Oct 22, 2024

ndr-brt Oct 22, 2024

bmg13 Oct 22, 2024

ndr-brt Oct 23, 2024

bmg13 Oct 23, 2024

ndr-brt Oct 23, 2024

bmg13 Oct 23, 2024

ndr-brt Oct 23, 2024

bmg13 Oct 23, 2024

ndr-brt Oct 23, 2024

bmg13 Oct 23, 2024

ndr-brt Oct 23, 2024

bmg13 Oct 23, 2024

sonarqubecloud bot commented Oct 23, 2024

	include("edc-extensions:agreements:retirement-evaluation-store-sql")
	include(":edc-extensions:agreements:retirement-evaluation-store-sql")

feat: Add SQL Agreement Retirement Store #1631

feat: Add SQL Agreement Retirement Store #1631

Conversation

bmg13 commented Oct 17, 2024

WHAT

WHY

FURTHER NOTES

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelmag110 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 23, 2024

Quality Gate passed