[CDSADAPTERS-2187]: Added integration dependency for cds import for events #94

Dipto-at-sap · 2024-11-13T11:05:50Z

JIRA: https://jira.tools.sap/browse/CDSADAPTERS-2187
Github issue: #81

swennemers

this is not the right approach to build the integration dependencies, you have to read the package json/cds config for this, the information is not the same as the events

__tests__/__snapshots__/noOrdInCdsrc.test.js.snap

swennemers · 2024-11-25T10:01:06Z

lib/ord.js

@@ -30,8 +32,13 @@ const initializeAppConfig = (csn) => {
    const modelKeys = Object.keys(csn.definitions);
    const apiEndpoints = new Set();
    const events = [];
+    let externalServiceKeys = [];
+    if (packageJson.cds && packageJson.cds.requires) {


What will happen, when the information is provided through the cdsrc.json and not via package json?

@swennemers if you have configurations in both package.json and cdsrc.json then it is considering both the configurations and both the objects are there in the integrationDependencies array

So, which one should be omitted or given more priority?

Consider the following case:
custom.ord.json

package.json

This is what I'm getting:

@Dipto-at-sap , the comment was related to the issue @David-Kunz highlighted, you can have cds.requires in package.json and cdsrc.json the custom.ord.json is another aspect.

With regards to the custom.ord.json, it must be consistent with the behavior described in https://github.com/cap-js/ord/blob/main/docs/ord.md. @zongqichen can support you in case of questions on that. Please reach out directly to him or @aramovic79 in his absence.

This is related to David's comment. So, we'll discuss it in the call.

swennemers · 2024-11-25T10:07:40Z

lib/templates.js

+        })
+        .map(event => ({ eventType: serviceDefinition.model.definitions[event]["@topic"] }));
+
+    return {


please follow the example of event and API templates to handle ordExtensions and don't explicitly access properties of ordExtensions and test that aspects can be overwritten as well (missing test)

In the case of createIntegrationDependencyTemplate, in the ordExtensions, there is no attribute related to the event, so how will we get the eventType for all the consumed external events? In API or event templates, we had no requirement for events, so we can't refer to those examples.

lib/templates.js

lib/ord.js

aramovic79 · 2024-11-25T15:51:54Z

__tests__/bookshop/package.json

+  "cds": {
+    "requires": {
+      "sap.s4.beh.businesspartner.v1.BusinessPartner": {
+        "kind": "odata",
+        "model": "srv/external/CE_BUSINESSPARTNEREVENTS.asyncapi"
+      }
+    }


Shouldn't we have a similar structure/content in the ORD/xmpl folder?

@David-Kunz , is this a correct cds.requires statement? Kind odata sounds like API required, but model is an async API, so events...

Yes, that is weird. I've only seen *.cds or *.json files here.

But I don't know if *.asyncapi is also supported...

I think this should be a .json file. You could make it .asyncapi.json if you want to state it. But it definately is a JSON file, encoded in application/json

it's indeed a json file, the name is given like that to state it's an asyncapi file.

This statement doesn't match: https://pages.github.tools.sap/cap/docs/guides/messaging/s4#configure-cap
Therefore I have my strong doubts the code is sitting on the right funcationality. @David-Kunz , can you help @Dipto-at-sap on how event consumption must be configured for a CAP app?

This was the requirement:

And for that issue, let me know if we're missing out on anything.

__tests__/bookshop/srv/external/CE_BUSINESSPARTNEREVENTS.asyncapi.cds

lib/ord.js

lib/templates.js

RoshniNaveenaS · 2024-11-27T05:43:00Z

this is not the right approach to build the integration dependencies, you have to read the package json/cds config for this, the information is not the same as the events

Yeah we have corrected this

lib/templates.js

David-Kunz · 2024-11-28T10:30:37Z

I did a small example with

srv/external/myRemote.cds

service myRemote {
  event Foo { key ID: UUID };
}

package.json contains

{
  "cds": {
    "requires": {
      "myRemote": {
        "kind": "odata",
        "model": "srv/external/myRemote.cds",
        "credentials": {
          "url": "http://www.foo.com"
        }
      }
  }
}

in custom code during bootstrapping:

  const ext = await cds.connect.to('myRemote')

The problem is that the remote service definitions are not part of

  app.get("/open-resource-discovery/v1/documents/1", async (req, res) => {
      /* ... */
      const csn = await cds.load(cds.env.folders.srv);  // <-- doesn't include myRemote

I think you assume that the external model is loaded within the own model (using {...} from './external/myRemote.cds'), which is not always the case.

Note: cds.model would include it, shouldn't this be used, also for performance reasons? In mtx scenarios, it should be the customer's model.

Since csn doesn't include the remote definitions (incl. events), integrationDependencies is empty.

swennemers · 2024-12-03T08:17:28Z

lib/defaults.js

@@ -58,6 +58,7 @@ module.exports = {
            return [
                createPackage(name, "-api:v1"),
                createPackage(name, "-event:v1"),
+                createPackage(name,"-integration-dependency:v1")


please us the constants defined above

@swennemers let me know which constant you're referring to. Here, I'm creating another package for integration dependency and defined the constant in the lib/constants.js, just similar to what was there for event and api.

swennemers

I don't see my comments addressed.

Dipto-at-sap · 2024-12-03T08:43:04Z

I don't see my comments addressed.

@swennemers for David's comment, currently we're assuming that the services will be given in package.json only and we'll set up a call for further discussion to address that. For now trying to get the other comments here resolved.

Add integration depencdency cds import for events

f94a447

swennemers requested changes Nov 13, 2024

View reviewed changes

RoshniNaveenaS marked this pull request as draft November 14, 2024 08:50

Dipto-at-sap and others added 8 commits November 15, 2024 13:37

Update based on Suggestions

feb6537

Small Fix

1190849

Fix conflicting branch

2ec62d7

Merge branch 'main' into integration_dependency

383486d

Add validation for cds requires

2b5f7b7

Merge branch 'main' into integration_dependency

6b50416

Add Test case for integrationDependency and update snapshots

cbb0808

Add more test cases for integrationDependency

9268a91

Dipto-at-sap marked this pull request as ready for review November 19, 2024 09:04

Dipto-at-sap requested review from swennemers, SamuelBrucksch and RoshniNaveenaS November 19, 2024 09:04

RoshniNaveenaS requested review from aramovic79 and Fannon November 21, 2024 09:04

Fannon reviewed Nov 21, 2024

View reviewed changes

__tests__/__snapshots__/noOrdInCdsrc.test.js.snap Outdated Show resolved Hide resolved

zongqichen added the ready for review label Nov 21, 2024

Modify the integrationDependencies attribute name

b4f60c4

Dipto-at-sap requested a review from Fannon November 25, 2024 08:26

Fix branch conflicts

2b2e379

RoshniNaveenaS requested a review from David-Kunz November 25, 2024 09:28

swennemers requested changes Nov 25, 2024

View reviewed changes

zongqichen linked an issue Nov 25, 2024 that may be closed by this pull request

ORD and integration dependencies (consumed events) #81

Open

aramovic79 reviewed Nov 25, 2024

View reviewed changes

lib/ord.js Outdated Show resolved Hide resolved

lib/ord.js Outdated Show resolved Hide resolved

aramovic79 reviewed Nov 26, 2024

View reviewed changes

lib/templates.js Outdated Show resolved Hide resolved

aramovic79 reviewed Nov 26, 2024

View reviewed changes

lib/templates.js Outdated Show resolved Hide resolved

zongqichen reviewed Nov 27, 2024

View reviewed changes

lib/templates.js Show resolved Hide resolved

lib/templates.js Outdated Show resolved Hide resolved

Update the PR based on review comments

6d0f266

Dipto-at-sap requested review from zongqichen, swennemers and aramovic79 December 3, 2024 03:39

swennemers reviewed Dec 3, 2024

View reviewed changes

swennemers requested changes Dec 3, 2024

View reviewed changes

Dipto-at-sap requested a review from swennemers December 4, 2024 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDSADAPTERS-2187]: Added integration dependency for cds import for events #94

[CDSADAPTERS-2187]: Added integration dependency for cds import for events #94

Dipto-at-sap commented Nov 13, 2024

swennemers left a comment

swennemers Nov 25, 2024

Dipto-at-sap Nov 28, 2024 •

edited

Loading

swennemers Nov 29, 2024

Dipto-at-sap Dec 3, 2024

swennemers Nov 25, 2024

Dipto-at-sap Dec 3, 2024 •

edited

Loading

aramovic79 Nov 25, 2024

swennemers Nov 29, 2024

David-Kunz Nov 29, 2024

David-Kunz Nov 29, 2024

Fannon Nov 29, 2024

Dipto-at-sap Nov 29, 2024

swennemers Dec 3, 2024

Dipto-at-sap Dec 3, 2024 •

edited

Loading

RoshniNaveenaS commented Nov 27, 2024

David-Kunz commented Nov 28, 2024 •

edited

Loading

swennemers Dec 3, 2024

Dipto-at-sap Dec 3, 2024 •

edited

Loading

swennemers left a comment

Dipto-at-sap commented Dec 3, 2024 •

edited

Loading

[CDSADAPTERS-2187]: Added integration dependency for cds import for events #94

Are you sure you want to change the base?

[CDSADAPTERS-2187]: Added integration dependency for cds import for events #94

Conversation

Dipto-at-sap commented Nov 13, 2024

swennemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dipto-at-sap Nov 28, 2024 • edited Loading

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

Dipto-at-sap Dec 3, 2024 • edited Loading

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

Dipto-at-sap Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

RoshniNaveenaS commented Nov 27, 2024

David-Kunz commented Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Dipto-at-sap Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

swennemers left a comment

Choose a reason for hiding this comment

Dipto-at-sap commented Dec 3, 2024 • edited Loading

Dipto-at-sap Nov 28, 2024 •

edited

Loading

Dipto-at-sap Dec 3, 2024 •

edited

Loading

Dipto-at-sap Dec 3, 2024 •

edited

Loading

David-Kunz commented Nov 28, 2024 •

edited

Loading

Dipto-at-sap Dec 3, 2024 •

edited

Loading

Dipto-at-sap commented Dec 3, 2024 •

edited

Loading