Feat: extend custom ORD content

cap-js / ord

Open Resource Discovery (ORD) is a protocol that allows applications and services to self-describe their exposed resources and capabilities. This plugin enables generation of ORD document for CAP based applications.

Apache License 2.0

3 stars 4 forks source link

Feat: extend custom ORD content #63

Closed zongqichen closed 1 month ago

swennemers commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

swennemers commented 1 month ago

Let's handle the custom package assignment in https://github.com/cap-js/ord/issues/64

zongqichen commented 1 month ago

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.

Fannon commented 1 month ago

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 commented 1 month ago

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 nullset 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 commented 1 month ago

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 nullset 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 commented 1 month ago

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 nullset 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.