Skip to content

Feature/2399-share-assets#2528

Open
kacperkruger wants to merge 11 commits intoAccenture:developfrom
kacperkruger:feature/2399-share-assets
Open

Feature/2399-share-assets#2528
kacperkruger wants to merge 11 commits intoAccenture:developfrom
kacperkruger:feature/2399-share-assets

Conversation

@kacperkruger
Copy link

@kacperkruger kacperkruger commented Mar 3, 2026

PR details

What changes did you make? (Give an overview)

  • Added option to automaticlly share assets during deployment.

  • Added --shareAsset option for building templates and deploying components

  • Changed stringification fields logic before templating to support nested paths with wildcards

  • closes [FEATURE] Automatic Asset Sharing After Deployment #2399

Further details (optional)

...

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • test scripts updated
  • Wiki updated (if applicable)

if (this.definition.stringifyFieldsBeforeTemplate) {
// numeric fields are returned as numbers by the SDK/API. If we try to replace them in buildTemplate it would break the JSON format - but not if we stringify them first because then the {{{var}}} is in quotes
const metadataTemp = JSON.parse(metadataStr);
for (const field of this.definition.stringifyFieldsBeforeTemplate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why this change was necessary?

Copy link
Author

@kacperkruger kacperkruger Mar 5, 2026

Choose a reason for hiding this comment

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

Fields containing business unit id as Integer (businessUnitAvailability.%.memberId, sharingProperties.sharedWith) have to be stringified before templating. Neded to add support for nested paths and wildcards paths

Copy link
Author

@kacperkruger kacperkruger Mar 11, 2026

Choose a reason for hiding this comment

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

Let me know if you think it should be refactored or done differently

Copy link
Contributor

@JoernBerkefeld JoernBerkefeld Mar 13, 2026

Choose a reason for hiding this comment

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

Actually like it. Wondering if we don't already have that same logic as part of how type dependencies are tracked for build --dependencies. If I am right I'd like to move the logic somewhere central and reuse it. Will investigate that

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an opt-in --shareAsset workflow to include asset sharing metadata during templating and deployment, and extends the pre-templating stringification logic to support nested field paths (including wildcards) so numeric IDs can be safely templated.

Changes:

  • Added --shareAsset CLI option across deploy/build/template-related commands and wired it into global option handling.
  • Enabled (opt-in) inclusion/templating of asset sharing fields (businessUnitAvailability, sharingProperties) and added tests for templating output.
  • Enhanced MetadataType.buildTemplate() numeric-to-string preprocessing to support nested paths with % wildcards.

Reviewed changes

Copilot reviewed 6 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
lib/metadataTypes/MetadataType.js Reworked stringify-before-template logic to support nested paths/wildcards.
lib/metadataTypes/definitions/Asset.definition.js Added nested stringify paths needed for templating share-related numeric IDs.
lib/metadataTypes/Asset.js Opt-in enabling of share-related fields + changed create behavior when shareAsset is set.
lib/cli.js Exposed --shareAsset flag on multiple commands.
lib/index.js Added shareAsset to recognized options.
test/type.asset.test.js Added templating test asserting share fields are preserved and numeric IDs are stringified.
@types/lib/metadataTypes/Asset.d.ts Updated typings to include newly referenced type and new helper methods.
@types/**/*.d.ts.map Regenerated sourcemaps reflecting the above changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +3003 to +3035
static _enableShareAsset() {
const sharingProperties = [
'businessUnitAvailability',
'businessUnitAvailability.%',
'businessUnitAvailability.%.view',
'businessUnitAvailability.%.update',
'businessUnitAvailability.%.delete',
'businessUnitAvailability.%.memberId',
'businessUnitAvailability.%.transferOwnership',
'sharingProperties',
'sharingProperties.sharedWith',
'sharingProperties.sharingType',
];

for (const property of sharingProperties) {
this.definition.fields[property] = {
isCreateable: true,
isUpdateable: true,
retrieving: true,
template: true,
};
}
}

/**
* enables share-asset metadata fields at runtime if option is active
*
* @returns {void}
*/
static _enableShareAssetIfNeeded() {
if (Util.OPTIONS.shareAsset) {
this._enableShareAsset();
}
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

_enableShareAsset() mutates the static this.definition.fields object at runtime and never restores the previous values. If Util.OPTIONS.shareAsset is toggled within the same process (tests, programmatic usage, multiple commands), the “enabled” template/retrieve behavior will leak into later operations even when shareAsset is false. Consider making the change reversible (store/restore original field configs) or apply the extra template fields without mutating the global definition.

Copilot uses AI. Check for mistakes.
delete metadata.businessUnitAvailability;
this._enableShareAssetIfNeeded();
if (!Util.OPTIONS.shareAsset) {
delete metadata.businessUnitAvailability;
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

create() conditionally keeps businessUnitAvailability when --shareAsset is set, but it does not similarly gate sharingProperties. As a result, a deploy package that contains sharingProperties can still trigger sharing behavior even when shareAsset is not enabled, making the flag ineffective as a safety switch. To make the option behavior explicit, strip sharingProperties (and any related share fields) when Util.OPTIONS.shareAsset is false, or otherwise ensure those fields cannot be deployed unless the flag is set.

Suggested change
delete metadata.businessUnitAvailability;
delete metadata.businessUnitAvailability;
delete metadata.sharingProperties;
delete metadata.sharingType;
delete metadata.sharedWith;

Copilot uses AI. Check for mistakes.
Comment on lines 309 to +313
static create(metadata) {
delete metadata.businessUnitAvailability;
this._enableShareAssetIfNeeded();
if (!Util.OPTIONS.shareAsset) {
delete metadata.businessUnitAvailability;
}
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

The --shareAsset option changes deploy behavior (Asset.create() keeps businessUnitAvailability, and share fields are enabled), but the test suite added here only validates templating/stringification. Please add deploy-focused coverage (create + update) that asserts: (1) share fields are omitted when shareAsset is false, and (2) they are sent when shareAsset is true, to prevent regressions in the deploy path.

Copilot uses AI. Check for mistakes.
Comment on lines +413 to +423
const stringifyValue = (value) => {
if (Array.isArray(value)) {
for (let i = 0; i < value.length; i++) {
value[i] = stringifyValue(value[i]);
}
return value;
} else if ('object' === typeof value) {
for (const key in value) {
value[key] = stringifyValue(value[key]);
}
return value;
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

In stringifyValue(), the typeof value === 'object' branch also matches null. If a targeted field/path ever contains null, the for (const key in value) loop will throw. Add an explicit value === null guard (and consider value === undefined) before treating it as an object/array.

Copilot uses AI. Check for mistakes.
Comment on lines +427 to +451
const stringifyFieldByPath = (target, fieldPathParts) => {
if (!target) {
return;
}
const [currentPart, ...rest] = fieldPathParts;
if (currentPart === '%') {
if (Array.isArray(target)) {
for (const item of target) {
stringifyFieldByPath(item, rest);
}
} else if ('object' === typeof metadataTemp[field]) {
for (const subField in metadataTemp[field]) {
metadataTemp[field][subField] =
metadataTemp[field][subField].toString();
} else if ('object' === typeof target) {
for (const key in target) {
stringifyFieldByPath(target[key], rest);
}
} else {
metadataTemp[field] = metadataTemp[field].toString();
}
return;
}

if (rest.length) {
stringifyFieldByPath(target[currentPart], rest);
} else {
if (target[currentPart]) {
target[currentPart] = stringifyValue(target[currentPart]);
}
}
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

stringifyFieldByPath() currently uses truthy checks (if (!target) and if (target[currentPart])). This skips valid falsy values like 0 (and also prevents walking into empty objects/arrays), which defeats the goal of reliably stringifying numeric fields. Use target == null checks and test for property existence (e.g., Object.prototype.hasOwnProperty.call(target, currentPart) / target[currentPart] !== undefined) instead of truthiness.

Copilot uses AI. Check for mistakes.
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