fix: event resources generation #55

ductaily · 2024-09-02T15:21:52Z

Address issues from #38

eventResources

The entries are service-level, not event-level
IMPORTANT: test with more than one event declared per service → should end up in one entry, not many

ductaily · 2024-09-02T15:27:36Z

I extended service definitions of bookshop in the test:

using { sap.capire.bookshop as my } from '../db/schema';
service AdminService @(requires:'authenticated-user') { 
  entity Books as projection on my.Books;
  entity Authors as projection on my.Authors;

  event BookCreated : {
    ID    : Integer;
    title : String @title: 'Title';
  };

  event BookDeleted : {
    ID    : Integer;
  };

  event BookUpdated : {
    ID    : Integer;
    title : String @title: 'Title';
  }
}

using { sap.capire.bookshop as my } from '../db/schema';
service CatalogService @(path:'/browse') { 

  @readonly entity Books as select from my.Books {*,
    author.name as author
  } excluding { createdBy, modifiedBy };

  @requires: 'authenticated-user'
  action submitOrder (book: Books:ID, quantity: Integer);
  
  event BookCreated : {
      ID    : Integer;
    title : String @title: 'Title';
  };

  event BookDeleted : {
    ID    : Integer;
  };

  event BookUpdated : {
    ID    : Integer;
    title : String @title: 'Title';
  }
}

Resulting ORD Document:

{
  "$schema": "https://sap.github.io/open-resource-discovery/spec-v1/interfaces/Document.schema.json",
  "apiResources": [
     //...
  ],
  "description": "this is an application description",
  "eventResources": [
    {
      "description": "This is an example event catalog that contains only a partial ODM cap-js-ord V1 event",
      "extensible": {
        "supported": "no",
      },
      "ordId": "capjs.ord:eventResource:undefined.AdminService:v1",
      "partOfGroups": [
        "sap.cds:service:capjs.ord:undefined.AdminService",
      ],
      "partOfPackage": "capjsord:package:undefined:v1",
      "releaseStatus": "beta",
      "resourceDefinitions": [
        {
          "accessStrategies": [
            {
              "type": "open",
            },
          ],
          "mediaType": "application/json",
          "type": "asyncapi-v2",
          "url": "/.well-known/open-resource-discovery/v1/api-metadata/AdminService.asyncapi2.json",
        },
      ],
      "shortDescription": "Example ODM Event",
      "title": "ODM capjsord Events",
      "version": "1.0.0",
      "visibility": "public",
    },
    {
      "description": "This is an example event catalog that contains only a partial ODM cap-js-ord V1 event",
      "extensible": {
        "supported": "no",
      },
      "ordId": "capjs.ord:eventResource:undefined.CatalogService:v1",
      "partOfGroups": [
        "sap.cds:service:capjs.ord:undefined.CatalogService",
      ],
      "partOfPackage": "capjsord:package:undefined:v1",
      "releaseStatus": "beta",
      "resourceDefinitions": [
        {
          "accessStrategies": [
            {
              "type": "open",
            },
          ],
          "mediaType": "application/json",
          "type": "asyncapi-v2",
          "url": "/.well-known/open-resource-discovery/v1/api-metadata/CatalogService.asyncapi2.json",
        },
      ],
      "shortDescription": "Example ODM Event",
      "title": "ODM capjsord Events",
      "version": "1.0.0",
      "visibility": "public",
    },
  ],
  "groups": [
    {
      "groupId": "sap.cds:service:capjs.ord:undefined.AdminService",
      "groupTypeId": "sap.cds:service",
      "title": "Admin Service Title",
    },
    {
      "groupId": "sap.cds:service:capjs.ord:undefined.CatalogService",
      "groupTypeId": "sap.cds:service",
      "title": "Catalog Service Title",
    },
  ],
  "openResourceDiscovery": "1.9",
  "packages": [
    {
      "description": "Description for capjs ord",
      "ordId": "capjsord:package:undefined:v1",
      "partOfProducts": [
        "customer:product:capjs.ord:",
      ],
      "shortDescription": "Short description for capjs ord",
      "title": "capjs ord",
      "vendor": "customer:vendor:Customer:",
      "version": "1.0.0",
    },
  ],
  "policyLevel": "none",
  "products": [
    {
      "ordId": "customer:product:cap.js.ord:",
      "shortDescription": "Description for cap js ord",
      "title": "cap js ord",
      "vendor": "customer:vendor:customer:",
    },
  ],
}

@Fannon

ductaily · 2024-09-02T15:42:09Z

Result with our example:
document.json

lib/ord.js

Co-authored-by: Zongqi Chen <137198879+zongqichen@users.noreply.github.com>

zongqichen · 2024-09-03T11:46:19Z

lib/ord.js

@@ -113,14 +112,9 @@ const fGetGroups = (csn, global) => {
    // storing the group ids in a set to avoid duplicates
    let groupIds = new Set();

-    let serviceGroups = global.aServices
+    return global.aServices


I know it's not your problem, but this function implementation is really wired:

We should avoid using global to store data.

If we have to use global, we should not pass it as argument, which against the usage of global.

The name 'global.aServices' is really confused. I would suggest that, later we use oop to refactor it.

The function name and return are also confused. The name is fGetGroups, and I would expect to get the list of group. But the return value is indirectly indication. I would suggest, either we define a variable: 'const groupList' and return this variable, or remove line "let groupIds = new Set();", e.g

const fGetGroups = (csn, global) => return global.aServices .map((serviceName) => fCreateGroupsTemplateForService(serviceName, csn.definitions[serviceName], new Set())) .filter((resource) => resource !== null && resource !== undefined);

A few cents from my side:

We should avoid using global to store data.

If we have to use global, we should not pass it as argument, which against the usage of global.

This probably just my preference. I don't really want to mock a global variable in tests and rather pass a mocked object into the function. Not sure what the others think about it.

The name 'global.aServices' is really confused. I would suggest that, later we use oop to refactor it.

Could you please elaborate what exactly is confusing you?

The whole project uses a functional programming approach. Not sure how much sense it makes to switch to a totally different concept.

The function name and return are also confused. The name is fGetGroups, and I would expect to get the list of group. But the return value is indirectly indication. I would suggest, either we define a variable: 'const groupList' and return this variable, or remove line "let groupIds = new Set();", e.g ...

Good point. It would even make sense to remove the groupIds from the function signature.

Thx @ductaily, I see https://github.com/cap-js/ord/blob/main/lib/ord.js#L163, the global is the name of object. This is what I want, but the name is conflict with javascript keyword. We should avoid it. And good point, the naming pattern for example aServices and fGetGroups, we should have agreement in discussion.

Just to document something that @zongqichen and I discussed via call:

I don't think that global is a global variable. It's very likely a variable that the CAP Framework passes into the plugins, to pass a global context, where the CAP framework keeps the overall information on what is part of the current application. This is where you find a list of all services, entities etc.

This was also my understanding what the global variable here is. I misinterpreted the statements above.

lib/ord.js

zongqichen · 2024-09-03T12:13:20Z

__tests__/ord.test.js

+          let service = match.groups?.service
+          if (service.startsWith("undefined")) service = service.replace("undefined.", "")


Could you explain these two lines? And service seems to imply to serviceName?

Yes, service imply serviceName, you can see it in the group id regex as a named capturing group.

The implementation here is not finished but my goal was to find the corresponding service in the CSN definition using the information I have in the ORD Document. In this case, I want to find that the eventResource partOfGroups is assigned to a groupId that is actually a service in the CSN.

Since they are attaching the namespace in the groupId, I want to remove the namespace part here to find the corresponding CSN definition. In this case, undefined.

I will improve this to check for the namespace and remove the namespace instead of undefined.

I hope this answered the question.

Co-authored-by: Zongqi Chen <137198879+zongqichen@users.noreply.github.com>

…nto fix/event-resources

ductaily · 2024-09-03T13:12:18Z

The tests are failing and I can't reproduce the error on @zongqichen and my machine.

@RoshniNaveenaS Do you have an idea what could be the cause and how we could resolve it?

ductaily · 2024-09-04T14:33:28Z

The tests are failing and I can't reproduce the error on @zongqichen and my machine.

@RoshniNaveenaS Do you have an idea what could be the cause and how we could resolve it?

I see now that the failure is caused by the new CDS Version: https://github.tools.sap/cap/cds/releases/tag/rel%2F8.2.0
The LinkedDefinitions were changed and do not contain map, forEach, ... anymore.

zongqichen · 2024-09-04T17:18:21Z

The tests are failing and I can't reproduce the error on @zongqichen and my machine.
@RoshniNaveenaS Do you have an idea what could be the cause and how we could resolve it?

I see now that the failure is caused by the new CDS Version: https://github.tools.sap/cap/cds/releases/tag/rel%2F8.2.0 The LinkedDefinitions were changed and do not contain map, forEach, ... anymore.

Cool, your commit has fixed it. I have one more question, it seems github action will always install the latest version of cds, and brings the issue like it. We should control peerDepdencies upper limits as well?

zongqichen · 2024-09-04T17:31:04Z

lib/templates.js

@@ -289,7 +289,7 @@ const fCreateEventResourceTemplate = (srv, srvDefinition, global,packageIds) =>
 */
 function checkEntityFunctionAction(srvDefinition, global) {
  if (srvDefinition.entities) {
-    return srvDefinition.entities.map((entity) => {
+    return [...srvDefinition.entities].map((entity) => {


I think there are 5 more places need to be adjust, for example srvDefinition.actions.map. But we don't have corresponding test, lets ignore it in this pr.

Fannon · 2024-09-05T07:46:22Z

__tests__/ord.test.js

+
+
+  describe("eventResources", () => {
+    const GROUP_ID_REGEX = /^([a-z0-9-]+(?:[.][a-z0-9-]+)*):([a-zA-Z0-9._\-/]+):([a-z0-9-]+(?:[.][a-z0-9-]+)*):(?<service>[a-zA-Z0-9._\-/]+)$/


I like this idea to have named groups in a regexp!

ductaily · 2024-09-09T07:12:43Z

The tests are failing and I can't reproduce the error on @zongqichen and my machine.
@RoshniNaveenaS Do you have an idea what could be the cause and how we could resolve it?

I see now that the failure is caused by the new CDS Version: https://github.tools.sap/cap/cds/releases/tag/rel%2F8.2.0 The LinkedDefinitions were changed and do not contain map, forEach, ... anymore.

Cool, your commit has fixed it. I have one more question, it seems github action will always install the latest version of cds, and brings the issue like it. We should control peerDepdencies upper limits as well?

@zongqichen Yes, we should specify the exact version.

Fannon · 2024-09-09T08:23:49Z

__tests__/__snapshots__/ord.test.js.snap

+      "extensible": {
+        "supported": "no",
+      },
+      "ordId": "capjs.ord:eventResource:undefined.AdminService:v1",


I still feel bad about all those undefined here. But this is planned to be fixed with a follow up PR?

Yeah, I will fix it in next PR

The undefined issues are all related and coming from the global.capNamespace. That will also fix this.

aramovic79 · 2024-09-09T15:12:23Z

@ductaily: There are a few merge conflicts to be resolved...

@zongqichen @Fannon

ductaily · 2024-09-09T15:27:34Z

@ductaily: There are a few merge conflicts to be resolved...

@zongqichen @Fannon

Done.

aramovic79 · 2024-09-11T09:59:29Z

@RoshniNaveenaS @RamIndia : Can we merge this PR?
cc: @ductaily @zongqichen @Fannon

aramovic79 · 2024-09-11T10:02:52Z

@ductaily : I guess the following one is solved as well, correct?
The groups for events shall be services, not the individual events → but then the last two entries in the bookstore sample are duplicate

ductaily · 2024-09-11T10:15:34Z

@ductaily : I guess the following one is solved as well, correct? The groups for events shall be services, not the individual events → but then the last two entries in the bookstore sample are duplicate

Correct. This PR would also solve that issue.

RamIndia

Checked the generated documents for incidents app, looks fine as well

fix event resources

60e4329

Update ord.test.js

508e3a7

ductaily marked this pull request as ready for review September 2, 2024 15:38

ductaily requested review from Fannon, aramovic79 and zongqichen September 2, 2024 15:38

zongqichen reviewed Sep 3, 2024

View reviewed changes

lib/ord.js Outdated Show resolved Hide resolved

Update lib/ord.js

b0206f3

Co-authored-by: Zongqi Chen <137198879+zongqichen@users.noreply.github.com>

zongqichen reviewed Sep 3, 2024

View reviewed changes

lib/ord.js Outdated Show resolved Hide resolved

zongqichen reviewed Sep 3, 2024

View reviewed changes

ductaily and others added 3 commits September 3, 2024 14:17

Update lib/ord.js

63a3a3b

Co-authored-by: Zongqi Chen <137198879+zongqichen@users.noreply.github.com>

Check srvDefinition before passing it

11d916b

Merge branch 'fix/event-resources' of https://github.com/cap-js/ord i…

c851316

…nto fix/event-resources

ductaily requested a review from RoshniNaveenaS September 3, 2024 13:11

fix missing array methods

30ca734

zongqichen approved these changes Sep 4, 2024

View reviewed changes

zongqichen reviewed Sep 4, 2024

View reviewed changes

Fannon reviewed Sep 5, 2024

View reviewed changes

ductaily requested a review from Fannon September 9, 2024 08:20

Fannon reviewed Sep 9, 2024

View reviewed changes

Duc Tai Ly added 2 commits September 9, 2024 13:39

Merge branch 'main' into fix/event-resources

76289ba

fix asyncapi url and remove obselete test case

ba96369

Duc Tai Ly added 2 commits September 9, 2024 17:21

Merge remote-tracking branch 'origin' into fix/event-resources

cab2eea

Update snapshot

e8d96ee

RamIndia approved these changes Sep 12, 2024

View reviewed changes

aramovic79 merged commit e9d6538 into main Sep 12, 2024
3 checks passed

aramovic79 deleted the fix/event-resources branch September 12, 2024 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: event resources generation #55

fix: event resources generation #55

ductaily commented Sep 2, 2024 •

edited

Loading

ductaily commented Sep 2, 2024

ductaily commented Sep 2, 2024

zongqichen Sep 3, 2024

ductaily Sep 3, 2024

zongqichen Sep 3, 2024

Fannon Sep 5, 2024

ductaily Sep 9, 2024

zongqichen Sep 3, 2024

ductaily Sep 3, 2024

ductaily commented Sep 3, 2024

ductaily commented Sep 4, 2024

zongqichen commented Sep 4, 2024

zongqichen Sep 4, 2024

Fannon Sep 5, 2024

ductaily commented Sep 9, 2024

Fannon Sep 9, 2024 •

edited

Loading

zongqichen Sep 9, 2024

ductaily Sep 9, 2024

aramovic79 commented Sep 9, 2024

ductaily commented Sep 9, 2024

aramovic79 commented Sep 11, 2024

aramovic79 commented Sep 11, 2024

ductaily commented Sep 11, 2024

RamIndia left a comment

		let service = match.groups?.service
		if (service.startsWith("undefined")) service = service.replace("undefined.", "")



		describe("eventResources", () => {
		const GROUP_ID_REGEX = /^([a-z0-9-]+(?:[.][a-z0-9-]+)):([a-zA-Z0-9._\-/]+):([a-z0-9-]+(?:[.][a-z0-9-]+)):(?<service>[a-zA-Z0-9._\-/]+)$/

fix: event resources generation #55

fix: event resources generation #55

Conversation

ductaily commented Sep 2, 2024 • edited Loading

ductaily commented Sep 2, 2024

ductaily commented Sep 2, 2024

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

ductaily commented Sep 3, 2024

ductaily commented Sep 4, 2024

zongqichen commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ductaily commented Sep 9, 2024

Fannon Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramovic79 commented Sep 9, 2024

ductaily commented Sep 9, 2024

aramovic79 commented Sep 11, 2024

aramovic79 commented Sep 11, 2024

ductaily commented Sep 11, 2024

RamIndia left a comment

Choose a reason for hiding this comment

ductaily commented Sep 2, 2024 •

edited

Loading

Fannon Sep 9, 2024 •

edited

Loading