Skip to content

fix: use correct DM delivery URLs per asset MIME type #832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auniverseaway merged 4 commits into from
Mar 19, 2026
Merged

fix: use correct DM delivery URLs per asset MIME type #832

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f87462f
fix: use correct DM delivery URLs per asset MIME type
actinium15 Mar 9, 2026
1ff7d2b
fix: correct DM delivery URL formats per OpenAPI spec
actinium15 Mar 9, 2026
20a9959
fix: encode asset name in DM delivery URLs
actinium15 Mar 9, 2026
740f434
Merge branch 'main' into fix/da-assets-dm-delivery-url-routing
auniverseaway Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion blocks/edit/da-assets/da-assets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ const loadScript = (await import(`${getNx()}/utils/script.js`)).default;

const ASSET_SELECTOR_URL = 'https://experience.adobe.com/solutions/CQ-assets-selectors/static-assets/resources/assets-selectors.js';

// MIME types that use /original rendition instead of web-optimized or /play delivery.
// Configurable per-project via 'aem.asset.document.mimetypes' (comma-separated).
Copy link
Copy Markdown

@sagarsane sagarsane Mar 9, 2026

Choose a reason for hiding this comment

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

@auniverseaway @chrischrischris I'm thinking we probably don't need to configure this per project .. and by default only support whatever Dynamic Media OpenAPI supports (which is the list below). WDYT?

Copy link
Copy Markdown
Contributor Author

@actinium15 actinium15 Mar 9, 2026

Choose a reason for hiding this comment

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

I think we should have that config. two reasons:

  • the list isn't comprehensive since assets beyond these four may also require original download (pptx, doc, txt, ...) and DM with OpenAPI does support them
  • videos are interesting, because while /play is ideal, some customers may want to use mediabus to deliver mp4s and may require original there (and this can serve as an override)

const USE_ORIGINAL_DELIVERY_FOR_MIMETYPES = [
'application/pdf',
'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
'text/csv',
'application/zip',
];

const fullConfJsons = {};
const CONFS = {};

Expand Down Expand Up @@ -121,11 +130,25 @@ export async function openAssets() {

const getBaseDmUrl = (asset) => `https://${prodOrigin}${prodOrigin.includes('/') ? '' : '/adobe/assets/'}${asset['repo:id']}`;

const useOriginalMimetypesConfig = await getConfKey(owner, repo, 'aem.asset.document.mimetypes');
const useOriginalMimetypes = useOriginalMimetypesConfig
? useOriginalMimetypesConfig.split(/\s*,\s*/).map((t) => t.toLowerCase().trim())
: USE_ORIGINAL_DELIVERY_FOR_MIMETYPES;

const getAssetUrl = (asset, name = asset.name) => {
if (!dmDeliveryEnabled) {
return `https://${prodOrigin}${asset.path}`;
}
return `${getBaseDmUrl(asset)}${asset.mimetype?.startsWith('video/') ? '/original' : ''}/as/${name}`;
const mimeType = asset.mimetype?.toLowerCase();
const encodedName = encodeURIComponent(name);
if (useOriginalMimetypes.includes(mimeType)) {
return `${getBaseDmUrl(asset)}/original/as/${encodedName}`;
}
if (mimeType?.startsWith('video/')) {
return `${getBaseDmUrl(asset)}/play`;
Copy link
Copy Markdown
Contributor

@ravuthu ravuthu Mar 11, 2026

Choose a reason for hiding this comment

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

When the author repository is configured for the asset selector, it's not guaranteed that the DM OpenAPI is enabled for the project, and /play endpoint may not be available, as I understand. @actinium15 thoughts?

Copy link
Copy Markdown
Contributor Author

@actinium15 actinium15 Mar 11, 2026

Choose a reason for hiding this comment

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

author repository is configured for the asset selector, it's not guaranteed that the DM OpenAPI is enabled for the project, and /play endpoint may not be available, as I understand

your understanding is correct.
afaict, author-repo flow ends a L140 with a return in if (!dmDeliveryEnabled) {} block. the .play endpoint changes are thus only applicable when dmDeliveryEnabled=true.

apologies if I'm going blind and missing something obvious here, @ravuthu

Copy link
Copy Markdown
Contributor

@ravuthu ravuthu Mar 11, 2026

Choose a reason for hiding this comment

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

@actinium15 The use case is that some customers choose the author Repo to have the folder structure even though the DM delivery is enabled.

Copy link
Copy Markdown
Contributor Author

@actinium15 actinium15 Mar 11, 2026

Choose a reason for hiding this comment

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

some customers choose the author Repo to have the folder structure even though the DM delivery is enabled

as long as the code's generating urls that start with getBaseDmUrl (which the original code was doing as well here), the author/delivery/... repo won't matter.

Copy link
Copy Markdown

@pablomoreno61 pablomoreno61 Mar 12, 2026

Choose a reason for hiding this comment

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

If I understood the code correctly, dmDeliveryEnabled == true does not really mean that DM is enabled, but it means that the customer said they want to use the delivery tier.

To be 100% sure DM is enabled, is it possible to call the Discovery Service /index endpoint? In such a case, the Discovery Service returns "dmopenapi" as a value for "aem:solutions" property when Dynamic Media is enabled.

Copy link
Copy Markdown
Contributor Author

@actinium15 actinium15 Mar 12, 2026

Choose a reason for hiding this comment

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

dmDeliveryEnabled == true does not really mean that DM is enabled

the current code (outside the context of this PR) switches completely to generating DM Urls when dmDeliveryEnabled==true.

is it possible to call the Discovery Service /index endpoint?

@pablomoreno61, are you suggesting that the assumption made in the code that's on mainline today is wrong?

Copy link
Copy Markdown

@pablomoreno61 pablomoreno61 Mar 12, 2026

Choose a reason for hiding this comment

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

My understanding is that dmDeliveryEnabled is set in
const dmDeliveryEnabled = smartCropSelectEnabled || (await getConfKey(owner, repo, 'aem.asset.dm.delivery')) === 'on' || prodOrigin?.startsWith('delivery-');

I'm not knowledgeable about this repo, so take my opinion with a grain of salt. In that line of code, we are reading the properties set by the customer in their DA config file, right? If there is no previous validation, we are just "trusting" the input from the customer, but in the end, Dynamic Media could still be disabled for the AEM repository they targeted if they did not manually enable DM within AEM Experience Manager Cloud.

Copy link
Copy Markdown
Contributor Author

@actinium15 actinium15 Mar 12, 2026

Choose a reason for hiding this comment

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

@ravuthu, @sagarsane, the behaviour @pablomoreno61 notes (if valid) predates this changeset.

would you agree? if so, perhaps a candidate to address in a followup PR and move ahead with this changeset?

wdyt?

}
const baseName = encodedName.replace(/\.[^.]+$/, '');
return `${getBaseDmUrl(asset)}/as/${baseName}.avif`;
};

// Determine if images should be links
Expand Down
Loading