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

build(deps): upgrade EDC to 0.1.2 #35

Merged
merged 10 commits into from
Jul 31, 2023
Merged

Conversation

ndr-brt
Copy link
Contributor

@ndr-brt ndr-brt commented May 23, 2023

what

Upgrade EDC to 0.1.2

why

keep dependencies updated

notes

updated gradle at 7.6.1 as well

@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch from 9798443 to 151409f Compare May 23, 2023 13:25
@ndr-brt
Copy link
Contributor Author

ndr-brt commented Jun 1, 2023

I'm working to switch all the calls to the new Json-LD endpoints, unfortunately seems that the json-ld library has an annoying bug: digitalbazaar/jsonld.js#523

will see if there are workaround to this

@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch from bf7cdbb to 0718110 Compare June 1, 2023 15:16
@fdionisi fdionisi mentioned this pull request Jun 1, 2023
2 tasks
OlfaBensoussia and others added 2 commits July 27, 2023 16:43
* 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]>
@OlfaBensoussia
Copy link
Contributor

@ndr-brt I took the liberty of removing the ternary operator in my latest commit because it's not really doing anything there. The initial value assigned to the query is {} and we are already using the spread operator for constructing the body. So even if it's empty, it won't affect the final result.
As another update, I am currently going through the same issue. All of the tests are passing locally but failing when I push. This is probably related to the receiver server
Screenshot 2023-07-27 at 5 33 27 PM

@ndr-brt
Copy link
Contributor Author

ndr-brt commented Jul 28, 2023

@ndr-brt I took the liberty of removing the ternary operator in my latest commit because it's not really doing anything there. The initial value assigned to the query is {} and we are already using the spread operator for constructing the body. So even if it's empty, it won't affect the final result. As another update, I am currently going through the same issue. All of the tests are passing locally but failing when I push. This is probably related to the receiver server Screenshot 2023-07-27 at 5 33 27 PM

seems like negotiations got TERMINATED instead of FINALIZED, now I improved the waitForNegotiationState and now that's visible, it remains to understand why they fail, will investigate further

@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch 4 times, most recently from 4ef0d6f to 3d9cb72 Compare July 28, 2023 07:40
@ndr-brt
Copy link
Contributor Author

ndr-brt commented Jul 28, 2023

I think it has something to do with the json-ld expansion, because this offer:

{
  "@id": "ZGVmaW5pdGlvbi0zZjgzNTFmMS0zOWQ0LTQ1ODMtYmI2YS0yOWZjZDg0NGJhOTg=:NDQ0MTlmMzEtNTFkMi00MDE4LWJiNDYtYWFjMjczMmM5MWFl:M2E5MDExNTQtM2U0Yy00N2FhLWFhZjYtNDQxMmMwOTY0MDQ0",
  "@type": "odrl:Set",
  "odrl:permission": [],
  "odrl:prohibition": [],
  "odrl:obligation": [],
  "odrl:target": "44419f31-51d2-4018-bb46-aac2732c91ae"
}

gets expanded as:

{
  "id": "NDQ0MTlmMzEtNTFkMi00MDE4LWJiNDYtYWFjMjczMmM5MWFl:M2E5MDExNTQtM2U0Yy00N2FhLWFhZjYtNDQxMmMwOTY0MDQ0",
  "type": [
    "http://www.w3.org/ns/odrl/2/Set"
  ],
  "http://www.w3.org/ns/odrl/2/obligation": [],
  "http://www.w3.org/ns/odrl/2/permission": [],
  "http://www.w3.org/ns/odrl/2/prohibition": [],
  "http://www.w3.org/ns/odrl/2/target": [
    {
      "@value": "44419f31-51d2-4018-bb46-aac2732c91ae"
    }
  ]
}

so it loses the first part of the ID, could be related to an issue that I already opened on:
digitalbazaar/jsonld.js#523

No clue why this is not happening locally...

@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch 6 times, most recently from f41236c to 29269e1 Compare July 28, 2023 09:35
@ndr-brt
Copy link
Contributor Author

ndr-brt commented Jul 28, 2023

@OlfaBensoussia the ternary operator is necessary, because an empty json-ld cannot be expanded and it returns error, so the body should be either null or a valid json-ld (if you remove that now the related test will fail).
Added only to the queryTransferProcesses but it will be needed on the other endpoints as well.

Now we're green with 0.1.2, for 0.1.3 we'd need to discover why the expansion of the @id changes the id itself.

ready for merge and release? @fdionisi

@ndr-brt ndr-brt changed the title build(deps): upgrade EDC to milestone-9 build(deps): upgrade EDC to 0.1.2 Jul 28, 2023
@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch from 29269e1 to 4e6da0d Compare July 28, 2023 09:41
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.

Stunning job, @OlfaBensoussia and @ndr-brt. I have just a few nits, but most of them could be handled as follow-ups. Let me know your thoughts.

tests/test-utils.ts Outdated Show resolved Hide resolved
.github/workflows/pull-request-testing.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
jest.config.ts Show resolved Hide resolved
src/entities/context.ts Outdated Show resolved Hide resolved
src/entities/contract-agreement.ts Outdated Show resolved Hide resolved
src/entities/contract-negotiation.ts Show resolved Hide resolved
src/entities/data-address.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
@ndr-brt ndr-brt requested a review from fdionisi July 28, 2023 12:36
@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch 2 times, most recently from 20220b6 to 57c83f7 Compare July 28, 2023 13:03
@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch from 57c83f7 to a684f77 Compare July 28, 2023 13:09
providerAgentId?: string;

get assetId(): string {
return this['https://w3id.org/edc/v0.0.1/ns/assetId']
Copy link
Contributor

Choose a reason for hiding this comment

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

@ndr-brt do you think it will make sense if we tried to use ${EDC_CONTEXT}assetId instead of always hardcoding the value? it seems like there is a pattern in getting those values in general, it's usually 'https://w3id.org/edc/v0.0.1/ns/entityName'

Copy link
Contributor Author

@ndr-brt ndr-brt Jul 28, 2023

Choose a reason for hiding this comment

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

hey, I'm working at this, let's check out the latest commit 🌋

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to keep the objects not typed and defining properties through get methods that take info out of the json-ld properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmmh... despite this is a really clean approach, typescript lacks this feature: microsoft/TypeScript#14417
and it's pretty fundamental for what I was doing here... maybe something that can be used in the meantime for "output" objects, instead for creating "input" objects better go with the current approach.
The intersection between the two is the Policy class, that's also a crucial one.

@ndr-brt ndr-brt requested a review from OlfaBensoussia July 28, 2023 14:37
@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch from adc823d to 1ddbe01 Compare July 28, 2023 14:40
@ndr-brt ndr-brt force-pushed the feat/34-upgrade-milestone-9 branch from 6ab7e6d to 71925b6 Compare July 28, 2023 15:01
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.

LGTM! 🚀

@ndr-brt ndr-brt merged commit 6a9a329 into main Jul 31, 2023
@ndr-brt ndr-brt deleted the feat/34-upgrade-milestone-9 branch July 31, 2023 13:45
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