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

MXD Backend Service API #172

Merged

Conversation

ravinderkumarsap
Copy link
Contributor

@ravinderkumarsap ravinderkumarsap commented Dec 3, 2023

Description

Please describe your PR:
Created a http backend service to validate the transfer.
Link for issue : #128

  • What does this PR introduce?

Here's a step-by-step Explanation of created a REST API application to complete and validate a successful asset transfer:

Create a Content Storage API:

POST API /api/v1/contents:
{{backend-service-url}}/api/v1/content/

Endpoint to store assets.
Accepts asset data in the request body.
Persists the asset data and returns a URL or ID in the response.

GET API /api/v1/contents/{id}:
{{backend-service-url}}/api/v1/content/1

Endpoint to fetch content by ID.
Takes the content ID as a path parameter.
Returns the stored content associated with the given ID.
Create a Transfer API:

POST API /api/v1/transfers:
{{backend-service-url}}/api/v1/transfer/
Endpoint to accept transfer data from the connector.
Expects a JSON payload similar to:
{
"id": "",
"endpoint": "http://alice-tractusx-connector-dataplane:8081/api/public",
"authKey": "Authorization",
"authCode": "",
"properties": {}
}

Persists the transfer data with the transfer ID.
GET API /api/v1/transfers/{id}:
{{backend-service-url}}/api/v1/transfer/52ba4b2f-ae3a-46ae-96f2-6781aa819386/

Endpoint to retrieve transfer data by ID.
Takes the transfer ID as a path parameter.
Returns the JSON data pushed by the connector for the specified transfer.

GET API /api/v1/transfers/{id}/contents:
{{backend-service-url}}/api/v1/transfer/52ba4b2f-ae3a-46ae-96f2-6781aa819386/contents/

Endpoint to retrieve the actual assets content.
Uses the stored endpoint URL from the transfer data to fetch content.
Returns the content fetched from the specified endpoint.

GET API /api/v1/content/random
{{backend-service-url}}/api/v1/content/random
Return the Sample Random Generated JSON

Does it add a new feature? yes
Is it enhancing documentation : yes

It has the postman collections inside postman folder inside backend-service folder.

Closes #128

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

Copy link
Contributor

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

@ravinderkumarsap
Copy link
Contributor Author

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

I have updated the package declarations as discussed and updated the copyright headers as suggested.

@paullatzelsperger
Copy link
Contributor

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

I have updated the package declarations as discussed and updated the copyright headers as suggested.

you are still using Jakarta EE, for which there is no reason and which I asked you to replace earlier. Please use EDC modules and components, a lightweight REST API can easily be built with it, checkout this.

@ravinderkumarsap
Copy link
Contributor Author

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

I have updated the package declarations as discussed and updated the copyright headers as suggested.

you are still using Jakarta EE, for which there is no reason and which I asked you to replace earlier. Please use EDC modules and components, a lightweight REST API can easily be built with it, checkout this.

We have started with converting to EDC modules. We will update you once completed .

mxd/backend-service/Dockerfile Outdated Show resolved Hide resolved
mxd/backend-service.tf Outdated Show resolved Hide resolved
mxd/backend-service.tf Outdated Show resolved Hide resolved
mxd/backend-service/Dockerfile Outdated Show resolved Hide resolved
mxd/backend-service/build.gradle.kts Outdated Show resolved Hide resolved
mxd/backend-service/build.gradle.kts Outdated Show resolved Hide resolved
@ravinderkumarsap
Copy link
Contributor Author

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

Package declarations are wrong, and many copyright headers are likely wrong. Further, there is no reason to introduce J2EE when EDC modules are perfectly capable of implementing a REST API.

I have updated the package declarations as discussed and updated the copyright headers as suggested.

you are still using Jakarta EE, for which there is no reason and which I asked you to replace earlier. Please use EDC modules and components, a lightweight REST API can easily be built with it, checkout this.

We have started with converting to EDC modules. We will update you once completed.

I have completed the backend service using EDC modules. Let me know the feedback.

Copy link
Contributor

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

@ravinderkumarsap as I said before, please add E2E test coverage that demonstrates that the integration is working, after that I'll review the PR properly.

@ndr-brt I have completed the review comments and updated on PR. I am working on the E2E test coverage.

when the tests will be ready please re-request my review

@paullatzelsperger paullatzelsperger removed their request for review March 15, 2024 11:30
@hemantxpatel hemantxpatel requested a review from ndr-brt May 17, 2024 08:15
Copy link
Contributor

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

please ensure that tests are running as part of the CI

Comment on lines 38 to 57
implementation(libs.restAssured)
implementation(libs.edc.configuration.filesystem)
implementation(libs.edc.boot)
implementation(libs.edc.json.ld)
implementation(libs.edc.web.spi)
implementation(libs.edc.api.core)
implementation(libs.edc.core)
implementation(libs.edc.http)
implementation(libs.edc.http.lib)
implementation(libs.edc.http.spi)
implementation(libs.edc.jersey.core)
implementation(libs.swagger.core)
implementation(libs.edc.transaction.spi)
implementation(libs.edc.connector.core)
implementation(libs.edc.sql.core)
implementation(libs.apache.commons)
implementation(libs.postgresql)
implementation(libs.edc.transform)
implementation(libs.edc.transaction)
implementation(libs.edc.util)
Copy link
Contributor

Choose a reason for hiding this comment

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

there are some dependencies that are not needed, e.g.:

libs.edc.api.core
libs.edc.jersey.core
libs.swagger.core
libs.edc.transform
libs.edc.transaction
libs.edc.util

please clean them up

Copy link
Member

Choose a reason for hiding this comment

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

Removed unused libraries

format.version = "1.1"
[versions]
assertj = "3.25.3"
edc = "0.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a particular reason to not use the latest EDC version? (0.7.0)

Copy link
Member

Choose a reason for hiding this comment

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

Updated EDC version to 0.7.0

testImplementation(libs.assertj)
testImplementation(libs.edc.junit)
implementation(libs.edc.junit.base)
testImplementation(libs.testng)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we are using testng, please cleanup test dependencies as well

Copy link
Member

Choose a reason for hiding this comment

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

Removed unused libraries.

testImplementation(libs.postgres.containers)

testImplementation(testFixtures(libs.edc.sql.core))
testImplementation(libs.edc.core)
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed

Copy link
Member

Choose a reason for hiding this comment

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

Removed unused libraries.

Comment on lines 66 to 67
@Inject(required = false)
private EventListener okHttpEventListener;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused

Copy link
Member

Choose a reason for hiding this comment

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

Removed.

@Override
public SqlQueryStatement createQuery(QuerySpec querySpec) {
var select = format("SELECT * FROM %s", getTransferTable());
//return new SqlQueryStatement(select, querySpec, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

Removed commented codes.

Comment on lines 131 to 139
@Override
public StoreResult<Void> update(Content definition) {
return null;
}

@Override
public StoreResult<Content> deleteById(String id) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do define these methods if the implementation is not provided? please delete them from both interface and implementation

Copy link
Member

Choose a reason for hiding this comment

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

Removed unused codes.

monitor.warning(exception.getMessage());
throw new EdcPersistenceException(exception.getMessage(), exception);
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point of returning null here? I guess if affectedRow is 0 then it should throw an exception/return a failed result

Copy link
Member

Choose a reason for hiding this comment

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

Updated to throw exception.

Comment on lines 96 to 104
@Override
public StoreResult<Void> update(Transfer transfer) {
return null;
}

@Override
public StoreResult<Transfer> deleteById(String id) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the other class, please remove if not needed

Copy link
Member

Choose a reason for hiding this comment

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

Removed unused codes.


@RegisterExtension
public static final EdcRuntimeExtension RUNTIME = new EdcRuntimeExtension(
"",
Copy link
Contributor

Choose a reason for hiding this comment

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

this empty string parameter can be removed

Copy link
Member

Choose a reason for hiding this comment

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

This parameter is needed by EdcRuntimeExtension because of constructor overloading.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ieuna nope, there's a different constructor where the modules are passed as varargs as last argument, so you could use that one instead (way cleaner: an empty string is confusing to read)

Copy link
Member

Choose a reason for hiding this comment

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

not passing any argument to the varargs wasn't working, so "new String()" is passed to the third argument(varargs modules) now.

@RegisterExtension
EdcRuntimeExtension RUNTIME = new EdcRuntimeExtension(
        "backend",
        Map.of(....),
        new String()
);

Copy link
Contributor

Choose a reason for hiding this comment

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

@ieuna you don't need the third argument, just two: runtime name and configuration

Copy link
Member

Choose a reason for hiding this comment

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

Updated.

@ieuna ieuna requested a review from ndr-brt June 18, 2024 12:34
Copy link
Contributor

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

LGTM

@ndr-brt
Copy link
Contributor

ndr-brt commented Jun 19, 2024

please fix the conflicts and I will merge this

@ieuna
Copy link
Member

ieuna commented Jun 24, 2024

please fix the conflicts and I will merge this

resolved

@ieuna ieuna requested a review from ndr-brt June 24, 2024 05:21
@ndr-brt
Copy link
Contributor

ndr-brt commented Jun 24, 2024

@ieuna there's a validate-terraform-format check failing

@ieuna
Copy link
Member

ieuna commented Jun 24, 2024

@ieuna there's a validate-terraform-format check failing
@ndr-brt
fixed terraform format. validate terraform check is passing now

@ndr-brt ndr-brt merged commit 4d7bf0b into eclipse-tractusx:main Jun 25, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a http backend service to validate the transfer
6 participants