Feat: extend custom ORD content #63

zongqichen · 2024-09-26T16:30:10Z

No description provided.

swennemers · 2024-09-30T13:40:16Z

I was wondering, if we should have the custom ORD content be in sync with the ideas of a delta ORD document...
/open-resource-discovery-specification/pull/862/
@Fannon , what do you think?

Fannon · 2024-10-02T07:24:59Z

I was wondering, if we should have the custom ORD content be in sync with the ideas of a delta ORD document... /open-resource-discovery-specification/pull/862/ @Fannon , what do you think?

Do you mean that we should not just have a way to add / completely override, but use the same idea of the "JSON Patch" format (with explicit null semantics) from https://github.tools.sap/CentralEngineering/open-resource-discovery-specification/pull/862?

I'm not entirely sure. Merging sounds better first, but it would make it really difficult to completely override something that the CAP framework generated by default. If you just want to selectively override something, you would likely use the local annotations where they belong (Service level).
We would also have to explain the merge behavior, allthough it's not that complicated (JSON Merge Patch essentially).

So we need to pick between:

Merge Behavior

If custom ORD content doesn't exist yet, just add it
If custom ORD content has same ORD ID as existing content, merge it with the CAP generated content. The values in the override take precedence. If a value in override is set to null the property is to be removed entirely.

Override Behavior

If custom ORD content doesn't exist yet, just add it
If custom ORD content has same ORD ID as existing content, replace the generated content entirely with the override.

swennemers · 2024-10-02T08:05:42Z

I am not on the latest stage with the exact delta ORD syntax.

What we observe here is that we need deep merge logic (e.g. overwrite an APIs package assignment without the need for rewriting everything). Don't we need as similar logic also for delta ORD, like overwriting just the endpoint of an API?

I agree, that we need to explain the merge & overwrite logic. However, I thought, if we can sync these, we only need to describe it once

Fannon · 2024-10-02T08:35:54Z

I am not on the latest stage with the exact delta ORD syntax.

What we observe here is that we need deep merge logic (e.g. overwrite an APIs package assignment without the need for rewriting everything). Don't we need as similar logic also for delta ORD, like overwriting just the endpoint of an API?

I agree, that we need to explain the merge & overwrite logic. However, I thought, if we can sync these, we only need to describe it once

Ok, but overwriting a package assignment should not be done on the global ORD override level. You should do it on the CDS Service Level with @ORD.Extensions. Here we already have a merge logic, allthough not the best one (See issue #49)

swennemers · 2024-10-02T10:11:42Z

I am not on the latest stage with the exact delta ORD syntax.
What we observe here is that we need deep merge logic (e.g. overwrite an APIs package assignment without the need for rewriting everything). Don't we need as similar logic also for delta ORD, like overwriting just the endpoint of an API?
I agree, that we need to explain the merge & overwrite logic. However, I thought, if we can sync these, we only need to describe it once

Ok, but overwriting a package assignment should not be done on the global ORD override level. You should do it on the CDS Service Level with @ORD.Extensions. Here we already have a merge logic, although not the best one (See issue #49)

true, bad example with package assignment. but not every ORD Resource has the CDS entities, e.g. package. what should we do there.

lib/extendOrdWithCustom.js

swennemers · 2024-10-02T11:43:18Z

Let's handle the custom package assignment in #64

Co-authored-by: Sebastian Wennemers <sebastian.wennemers@sap.com>

zongqichen · 2024-10-04T16:02:20Z

I was wondering, if we should have the custom ORD content be in sync with the ideas of a delta ORD document... /open-resource-discovery-specification/pull/862/ @Fannon , what do you think?

Do you mean that we should not just have a way to add / completely override, but use the same idea of the "JSON Patch" format (with explicit null semantics) from https://github.tools.sap/CentralEngineering/open-resource-discovery-specification/pull/862?

I'm not entirely sure. Merging sounds better first, but it would make it really difficult to completely override something that the CAP framework generated by default. If you just want to selectively override something, you would likely use the local annotations where they belong (Service level). We would also have to explain the merge behavior, allthough it's not that complicated (JSON Merge Patch essentially).

So we need to pick between:

Merge Behavior

If custom ORD content doesn't exist yet, just add it

If custom ORD content has same ORD ID as existing content, merge it with the CAP generated content. The values in the override take precedence. If a value in override is set to null the property is to be removed entirely.

Override Behavior

If custom ORD content doesn't exist yet, just add it

If custom ORD content has same ORD ID as existing content, replace the generated content entirely with the override.

At the moment, I use lodash.merge as merge strategy: https://lodash.com/docs/4.17.15#merge. You can see the code. It will override existing item and merge new item if not existed.

swennemers · 2024-10-04T16:12:22Z

__tests__/bookshop/ord/custom.ord.json

+        {
+            "apiProtocol": "odata-v5",
+            "description": "new AdminService",
+            "ordId": "sap.test.cdsrc.sample:apiResource:AdminService:v1",
+            "entityTypeMappings": [
+                {
+                    "entityTypeTargets": [
+                        "new.entityType.target"
+                    ]
+                }
+            ]
+        }


Better have a testcase, where you add a new rest API, the other API resource overrides should be handled via @ORD.Extension mechanism, see linked issue.

Fannon · 2024-10-07T08:16:56Z

At the moment, I use lodash.merge as merge strategy: https://lodash.com/docs/4.17.15#merge. You can see the code. It will override existing item and merge new item if not existed.

By default the lodash merge will not have the JSON merge patch semantics with null. You need to make sure to also cover that (please also add tests for this, as it's easy to get this wrong).

Here is some JSFiddle that I wrote for showing how AsyncAPI trait inheritance could work: https://jsfiddle.net/FannonF/yLsonr57/latest/

function apply(origin, patch) {
  if (!isObject(patch)) {
    // If the patch is not an object, it replaces the origin.
    return patch;
  }

  const result = !isObject(origin) ? // Non objects are being replaced.
    {} : // Make sure we never modify the origin.
    Object.assign({}, origin);

  Object.keys(patch).forEach(key => {
    const patchVal = patch[key];
    if (patchVal === null) {
      delete result[key];
    } else {
      result[key] = apply(result[key], patchVal);
    }
  });
  return result;
}

zongqichen · 2024-10-07T11:57:42Z

At the moment, I use lodash.merge as merge strategy: https://lodash.com/docs/4.17.15#merge. You can see the code. It will override existing item and merge new item if not existed.

By default the lodash merge will not have the JSON merge patch semantics with null. You need to make sure to also cover that (please also add tests for this, as it's easy to get this wrong).

Here is some JSFiddle that I wrote for showing how AsyncAPI trait inheritance could work: https://jsfiddle.net/FannonF/yLsonr57/latest/
function apply(origin, patch) {
  if (!isObject(patch)) {
    // If the patch is not an object, it replaces the origin.
    return patch;
  }

  const result = !isObject(origin) ? // Non objects are being replaced.
    {} : // Make sure we never modify the origin.
    Object.assign({}, origin);

  Object.keys(patch).forEach(key => {
    const patchVal = patch[key];
    if (patchVal === null) {
      delete result[key];
    } else {
      result[key] = apply(result[key], patchVal);
    }
  });
  return result;
}

Hi @Fannon, I have some corner cases need to discuss:

if set null in array, should I remove the whole array property? e.g:

{
    "apiResources": [
        {
            "ordId": "sap.sm:apiResource:SupplierService:v1",
            "partOfGroups": [
                null,
                "sap.cds:service:sap.test.cdsrc.sample:AdminService"
            ],
            "partOfPackage": "sap.sm:package:smDataProducts:v1"
        }
    ]
}

I should remove the ´partOfGroups´?

if null set in default property array, should I leave it as empty or removed?

{
    "apiResources": [
        {null},
        {
            "ordId": "sap.sm:apiResource:SupplierService:v1",
            "partOfGroups": [
                "sap.cds:service:sap.test.cdsrc.sample:AdminService"
            ],
            "partOfPackage": "sap.sm:package:smDataProducts:v1"
        }
    ]
}

or this case:

{
    "apiResources": [
         null
    ]
}

Fannon · 2024-10-07T13:33:25Z

Nice summary of corner-cases, @zongqichen . I'll answer inline below:

Hi @Fannon, I have some corner cases need to discuss:

if set null in array, should I remove the whole array property? e.g:

{
    "apiResources": [
        {
            "ordId": "sap.sm:apiResource:SupplierService:v1",
            "partOfGroups": [
                null,
                "sap.cds:service:sap.test.cdsrc.sample:AdminService"
            ],
            "partOfPackage": "sap.sm:package:smDataProducts:v1"
        }
    ]
}

To keep it simple (and stick with JSON Merge patch), we should always override arrays completely. Everything else will get complicated very quickly when ordering changes. Setting null inside an array is therefore invalid, as it would result in having a null entry in an array, which is not allowed in ORD.

I should remove the ´partOfGroups´?

if null set in default property array, should I leave it as empty or removed?

{
    "apiResources": [
        {null},
        {
            "ordId": "sap.sm:apiResource:SupplierService:v1",
            "partOfGroups": [
                "sap.cds:service:sap.test.cdsrc.sample:AdminService"
            ],
            "partOfPackage": "sap.sm:package:smDataProducts:v1"
        }
    ]
}

This is not even valid JSON and therefore cannot be supported :)

or this case:

{
    "apiResources": [
         null
    ]
}

Same situation as with first edge-case. We'll not support it. If you want to remove the array, you need to set "apiResources": null.

zongqichen · 2024-10-07T13:53:09Z

Nice summary of corner-cases, @zongqichen . I'll answer inline below:
Hi @Fannon, I have some corner cases need to discuss:

if set null in array, should I remove the whole array property? e.g:
{
    "apiResources": [
        {
            "ordId": "sap.sm:apiResource:SupplierService:v1",
            "partOfGroups": [
                null,
                "sap.cds:service:sap.test.cdsrc.sample:AdminService"
            ],
            "partOfPackage": "sap.sm:package:smDataProducts:v1"
        }
    ]
}
To keep it simple (and stick with JSON Merge patch), we should always override arrays completely. Everything else will get complicated very quickly when ordering changes. Setting null inside an array is therefore invalid, as it would result in having a null entry in an array, which is not allowed in ORD.
I should remove the ´partOfGroups´?

if null set in default property array, should I leave it as empty or removed?
{
    "apiResources": [
        {null},
        {
            "ordId": "sap.sm:apiResource:SupplierService:v1",
            "partOfGroups": [
                "sap.cds:service:sap.test.cdsrc.sample:AdminService"
            ],
            "partOfPackage": "sap.sm:package:smDataProducts:v1"
        }
    ]
}
This is not even valid JSON and therefore cannot be supported :)
or this case:
{
    "apiResources": [
         null
    ]
}
Same situation as with first edge-case. We'll not support it. If you want to remove the array, you need to set "apiResources": null.

I read JSON merge patch rfc7396, I think it's slightly different with our use case. We should not always override arrays completely. If users want to update package localId for example, they need to define following content, package ordId and localId they need to update:

{
    "packages": [
        {
            "ordId": "sap.sm:package:smDataProducts:v1",
            "localId": "smDataProducts"
        }
    ]
}

If we override completely, there are only two properties left. Our strategy is based on ordId, if no ordId mateched or provided, then extend it.

__tests__/__snapshots__/ordCdsrc.test.js.snap

lib/extendOrdWithCustom.js

swennemers

looks quite good, let's check a few comments in the sync

__tests__/unittest/__snapshots__/extendOrdWithCustom.test.js.snap

lib/extendOrdWithCustom.js

aramovic79

@zongqichen @swennemers @Fannon
Please check the comment I left.

aramovic79 · 2024-10-09T15:30:28Z

lib/extendOrdWithCustom.js

+    const clonedOrdContent = structuredClone(ordContent);
+    for (const key in customORDContent) {
+        const propertyType = typeof customORDContent[key];
+        if (propertyType !== 'object' && propertyType !== 'array') {


My understanding of this check(and after @zongqichen explained me) is that we want to allow "patch" of the resulting ORD document by appending only the properties that are arrays/lists. That then can cause some confusion, because one can put property "something" of type array, and the plugin will generate a non-valid ORD Document.
Yes, Metadata Validator is the one validating it and it's main purpose, but on the other side we are this way generating "on-the-fly" a non-valid ORD Document which is to my opinion wrong.
I would rather keep the check to strictly allowed fields("dataProducts", "entityTypes") or at least add some validation on the very begging of the ORD doc generation if the custom *ord.json file contains properties with correct names(dataProducts, entityTypes...). Something like validation schema for the json object. Still, if it is already agreed among three of you to check only if the property is array, I will approved the PR.
cc: @Fannon @zongqichen @swennemers

@aramovic79 , these are good comments.
One of the use cases for this overwrite as well as also the @ORD.Extension is to allow a developer being ahead of the plugins Support for ORD. If we restrict the properties, we can't add things not yet available in the plug-in. But you are right, this comes with the risk of an broken ORD document. We can mitigate the risk with the validator and in case we want to experiment with new ORD features, we might even want the chance to do that.

Thanks, I link this comment to the #65, lets fix it in the following pr.

zongqichen added 5 commits September 25, 2024 12:19

add ovrride logic

0d58e3a

add todo list

0eb9736

udpate unittest

f7d1981

extract into new file

acd4a79

add unittest

84107fd

zongqichen linked an issue Sep 26, 2024 that may be closed by this pull request

Allow generic override / expand of ORD information on global level #49

Closed

swennemers reviewed Oct 2, 2024

View reviewed changes

lib/extendOrdWithCustom.js Outdated Show resolved Hide resolved

swennemers reviewed Oct 2, 2024

View reviewed changes

lib/extendOrdWithCustom.js Outdated Show resolved Hide resolved

extend conflict function

b2fc675

Fannon and others added 9 commits October 2, 2024 14:03

Update lib/extendOrdWithCustom.js

e408c81

Co-authored-by: Sebastian Wennemers <sebastian.wennemers@sap.com>

Update lib/extendOrdWithCustom.js

233ff11

Co-authored-by: Sebastian Wennemers <sebastian.wennemers@sap.com>

use lodash to merge two objects

89d4cf6

handle main branch conflict

83ccdf7

handle conflict

d3a5bcf

update snapshot

680f213

fix small issues

be7f45d

add one more test

d2340c8

update ord.custom.json

79c5d50

zongqichen requested review from swennemers, Fannon and aramovic79 October 4, 2024 15:57

clean up logic to be clear

c9feb8c

swennemers reviewed Oct 4, 2024

View reviewed changes

clean up unittest

c507b25

Fannon reviewed Oct 8, 2024

View reviewed changes

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

Fannon reviewed Oct 8, 2024

View reviewed changes

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

zongqichen added 5 commits October 8, 2024 13:07

clean up unittest

35cd873

change patch logic

389e664

fix mutation bug

2d95619

use override instead of merge

f68c43d

clean up

6cf94ce

zongqichen requested review from Fannon and swennemers October 8, 2024 15:33

zongqichen added the ready for review label Oct 8, 2024

Fannon reviewed Oct 8, 2024

View reviewed changes

lib/extendOrdWithCustom.js Outdated Show resolved Hide resolved

Fannon reviewed Oct 8, 2024

View reviewed changes

lib/extendOrdWithCustom.js Outdated Show resolved Hide resolved

swennemers reviewed Oct 9, 2024

View reviewed changes

__tests__/unittest/__snapshots__/extendOrdWithCustom.test.js.snap Show resolved Hide resolved

lib/extendOrdWithCustom.js Outdated Show resolved Hide resolved

zongqichen added 3 commits October 9, 2024 13:12

clean up dead code and add cds.log replacing console.log

5cc0335

give proper logger id

95d2058

refactor and fix

58bc670

swennemers approved these changes Oct 9, 2024

View reviewed changes

aramovic79 approved these changes Oct 9, 2024

View reviewed changes

zongqichen mentioned this pull request Oct 9, 2024

Feat: apply ord document validator in CICD pipeline #65

Open

zongqichen merged commit a159fee into main Oct 9, 2024
3 checks passed

zongqichen deleted the add_override_ord_information branch October 9, 2024 19:09

swennemers mentioned this pull request Oct 16, 2024

Feat: extend @ORD.Extensions #75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: extend custom ORD content #63

Feat: extend custom ORD content #63

zongqichen commented Sep 26, 2024

swennemers commented Sep 30, 2024

Fannon commented Oct 2, 2024 •

edited

Loading

swennemers commented Oct 2, 2024

Fannon commented Oct 2, 2024

swennemers commented Oct 2, 2024

swennemers commented Oct 2, 2024

zongqichen commented Oct 4, 2024

Merge Behavior

Override Behavior

swennemers Oct 4, 2024

Fannon commented Oct 7, 2024 •

edited

Loading

zongqichen commented Oct 7, 2024 •

edited

Loading

Fannon commented Oct 7, 2024

zongqichen commented Oct 7, 2024

swennemers left a comment

aramovic79 left a comment

aramovic79 Oct 9, 2024

swennemers Oct 9, 2024

zongqichen Oct 9, 2024

Feat: extend custom ORD content #63

Feat: extend custom ORD content #63

Conversation

zongqichen commented Sep 26, 2024

swennemers commented Sep 30, 2024

Fannon commented Oct 2, 2024 • edited Loading

Merge Behavior

Override Behavior

swennemers commented Oct 2, 2024

Fannon commented Oct 2, 2024

swennemers commented Oct 2, 2024

swennemers commented Oct 2, 2024

zongqichen commented Oct 4, 2024

Merge Behavior

Override Behavior

swennemers Oct 4, 2024

Choose a reason for hiding this comment

Fannon commented Oct 7, 2024 • edited Loading

zongqichen commented Oct 7, 2024 • edited Loading

Fannon commented Oct 7, 2024

zongqichen commented Oct 7, 2024

swennemers left a comment

Choose a reason for hiding this comment

aramovic79 left a comment

Choose a reason for hiding this comment

aramovic79 Oct 9, 2024

Choose a reason for hiding this comment

swennemers Oct 9, 2024

Choose a reason for hiding this comment

zongqichen Oct 9, 2024

Choose a reason for hiding this comment

Fannon commented Oct 2, 2024 •

edited

Loading

Fannon commented Oct 7, 2024 •

edited

Loading

zongqichen commented Oct 7, 2024 •

edited

Loading