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

Feat: update asset endpoints #36

Merged

Conversation

OlfaBensoussia
Copy link
Contributor

@OlfaBensoussia OlfaBensoussia commented May 30, 2023

Description

With the EDC milestone 9 release, there were some changes when it comes to the asset related entities.
This PR addresses all of them but doesn't add the new asset endpoints.
To sum it up, we:

  • Added a new Context object
  • Updated the CreateResult object
  • Fixed the wrong endpoint that was used to get an asset's data address
  • Updated Asset objects and added new ones
  • Updated tests
  • Updated some utilities.

How to test it

  • Asset related tests should be passing successfully

Approach

  • To understand why those changes had to be made, we checked the management API and made use of the open source POSTMAN collection published by the Tractus-x team
  • Those are some screenshots from my local postman to give more context:
  1. Creating a new asset: POST {{ConnectorManagementURL}}/v2/assets
Screenshot 2023-05-30 at 6 17 42 PM
  1. Getting the list of assets: POST {{ConnectorManagementURL}}/v2/assets/request
Screenshot 2023-05-30 at 6 17 57 PM
  1. Getting an asset: GET {{ConnectorManagementURL}}/v2/assets/{{AssetId}}
Screenshot 2023-05-30 at 6 24 01 PM
  1. Getting an asset's data address: GET {{ConnectorManagementURL}}/v2/assets/{{AssetId}}/dataaddress
Screenshot 2023-05-30 at 6 24 35 PM

Open Questions and Pre-Merge TODOs

  • Update readme examples
  • Check if we have added all of the asset's primitive types or not. We already have description, name and contenttype. I remember we have more than that but I will need to go through the base code to get them.

@OlfaBensoussia OlfaBensoussia changed the base branch from milesone-9-migration to feat/34-upgrade-milestone-9 May 30, 2023 17:10
@OlfaBensoussia OlfaBensoussia requested a review from fdionisi May 30, 2023 17:28
@OlfaBensoussia
Copy link
Contributor Author

I tagged @fdionisi as the main reviewer to validate this PR from an approach and coding perspective.
I am tagging @NabilHouidi @AhmedGrati and @Nazdroth to help validate this from a functional POV considering that your team is working on the migration too.

src/entities/context.ts Outdated Show resolved Hide resolved
Copy link
Member

@fdionisi fdionisi left a comment

Choose a reason for hiding this comment

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

Is there something we should do to enable testing?
Have we considered already using some json-ld package to avoid working on the types ourselves?

@OlfaBensoussia
Copy link
Contributor Author

OlfaBensoussia commented Jun 1, 2023

Is there something we should do to enable testing? Have we considered already using some json-ld package to avoid working on the types ourselves?

@fdionisi thank you so much for taking the time to review this.

  • By enabling testing, do you mean fixing the failing ones or something else?
  • Before making those changes, I had a look at the jsonld package to see if it can be useful to use here. After consideration, it seemed to me that the package is only useful if you want to make some manipulations [compacting, flattening..] on the json objects. In our case, I didn't feel like there will an added value for using it, especially that we're not actively making changes on the objects.

@fdionisi
Copy link
Member

fdionisi commented Jun 1, 2023

@OlfaBensoussia, sorry for my poor communication. I meant that we should make tests pass, but now we are on the latest version of the connector. We should try land #35 first and then use the docker image to endure correct functionality on this PR. I don't think we'll be developing anymore for milestone-8 (except for #20, for now). Hence, it should be find to proceed

@fdionisi
Copy link
Member

fdionisi commented Jun 1, 2023

  • Before making those changes, I had a look at the jsonld package to see if it can be useful to use here. After consideration, it seemed to me that the package is only useful if you want to make some manipulations [compacting, flattening..] on the json objects. In our case, I didn't feel like there will an added value for using it, especially that we're not actively making changes on the objects.

I hear you. It makes sense to me for now, but I would like to explore the possibility of using a json-ld-aware library to perform input validation - if the format is not correct, then it's not worth bothering the backend in my opinion. Let's keep this exploration for later.

@OlfaBensoussia
Copy link
Contributor Author

@ndr-brt I tried to fix most of the failing tests, remove any hardcoded values and update most of the entities. As far as it concerns the management tests, they are all now passing except the ones that rely on the function waitForNegotiationState.
I have a gut feeling that there is an issue with the Negotiation states in general. As an example, canceling a contract negotiation doesn't automatically change the state to ERROR, it stays as TERMINATING for a while first.
I still need your help in understanding that part and fixing the remaining failing ones. Once we do that, we should be good to consider that we have the Lib compatible with v0.1+

@ndr-brt
Copy link
Contributor

ndr-brt commented Jul 27, 2023

@ndr-brt I tried to fix most of the failing tests, remove any hardcoded values and update most of the entities. As far as it concerns the management tests, they are all now passing except the ones that rely on the function waitForNegotiationState. I have a gut feeling that there is an issue with the Negotiation states in general. As an example, canceling a contract negotiation doesn't automatically change the state to ERROR, it stays as TERMINATING for a while first. I still need your help in understanding that part and fixing the remaining failing ones. Once we do that, we should be good to consider that we have the Lib compatible with v0.1+

TERMINATING it is correct, the state machine changed a lot since then. Please note that the /cancel endpoint has been deprecated as well, replaced by /terminate.

I will take a look at it, great work so far. 🙌

@ndr-brt ndr-brt self-requested a review July 27, 2023 08:14
@ndr-brt
Copy link
Contributor

ndr-brt commented Jul 27, 2023

@OlfaBensoussia I fixed stuff here and there, the CI is green. I'll propose to merge this into my PR and then I will fixup some cosmetics that needs to be in place before release, let me know.

EDIT: it works on my computer but not on github 😆

@ndr-brt ndr-brt force-pushed the feat/update-asset-endpoints branch 7 times, most recently from b5a85c5 to 280203b Compare July 27, 2023 12:33
@ndr-brt ndr-brt force-pushed the feat/update-asset-endpoints branch 17 times, most recently from 02207ee to 17d2b14 Compare July 27, 2023 15:09
@ndr-brt ndr-brt force-pushed the feat/update-asset-endpoints branch from 17d2b14 to f830445 Compare July 27, 2023 15:13
@OlfaBensoussia
Copy link
Contributor Author

@ndr-brt Thank you for the clarifications. I will merge this and we can fix the rest of the tests on the original branch

@OlfaBensoussia OlfaBensoussia merged commit 27b71eb into feat/34-upgrade-milestone-9 Jul 27, 2023
@OlfaBensoussia OlfaBensoussia deleted the feat/update-asset-endpoints branch July 27, 2023 15:43
ndr-brt added a commit that referenced this pull request Jul 31, 2023
* build(deps): upgrade EDC to milestone-9

* Upgrade connector to 0.1.0

* Fix public.test.ts

* Feat: update asset endpoints (#36)

* chore: update old asset entities specs & add new context

* chore: use new connector entities in management controllers and utils

* test: adjust asset tests

* chore: update entities

* fix: fix contract, policy and asset failing tests

* chore: update addresses props in connector config files

* refactor: Standardize request body objects and update entities

* fix: update catalog and contract negotiations tests

* Fix CI

---------

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

* refactor: remove ternary operator

* add missing providerId field

* Print logs

* add logs

* PR remarks

* Refactor

---------

Co-authored-by: Olfa <[email protected]>
Co-authored-by: OlfaBensoussia <[email protected]>
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.

3 participants