adobe / asset-share-commons

A modern, open-source asset share reference implementation built on Adobe Experience Manager (AEM)
https://opensource.adobe.com/asset-share-commons/
Apache License 2.0
88 stars 107 forks source link

MetadataProperties OSGi Service/Datasource should support multi-tenancy #58

Open davidjgonzalez opened 6 years ago

davidjgonzalez commented 6 years ago

The MetadataProperties collection should support multi-tenancy. I can see this being achieved in 2 ways (or a mix).

https://github.com/Adobe-Marketing-Cloud/asset-share-commons/blob/develop/core/src/main/java/com/adobe/aem/commons/assetshare/content/impl/MetadataSchemaPropertiesImpl.java#L45

kaushalmall commented 6 years ago

@davidjgonzalez I can pick this up if you want. Based on our conversation, I think it might be better if we make this a page level configuration that can be authored.

The above two approaches have some drawbacks

  1. Assumes that the tenants are setup following the correct context aware structure, which might or might not be true

  2. If we specify this on the datasource node, it becomes a developer job and can't really be scalable every time a new "tenant" gets added.

Thinking that in [0], we can something like

Config config = request.adaptTo(Config.class)
config.getMetadataSchemaRoot()
metadataProperties.getMetadataProperties(request, metadataFieldTypes, metadataSchemaRoot);

then do the necessary changes in [1] and [2].

WDYT?

[0]https://github.com/Adobe-Marketing-Cloud/asset-share-commons/blob/develop/core/src/main/java/com/adobe/aem/commons/assetshare/content/impl/datasources/MetadataSchemaPropertiesDataSource.java [1]https://github.com/Adobe-Marketing-Cloud/asset-share-commons/blob/develop/core/src/main/java/com/adobe/aem/commons/assetshare/content/impl/MetadataSchemaPropertiesImpl.java [2]https://github.com/Adobe-Marketing-Cloud/asset-share-commons/blob/develop/core/src/main/java/com/adobe/aem/commons/assetshare/configuration/impl/ConfigImpl.java

davidjgonzalez commented 6 years ago

@kaushalmall that would be great - I actually tried going down this path assuming cq:conf was applied to the page structure (actually the new sling prop I can’t think of off the top of my head) and ran into issues.

On the surface it seems like using the “native” context aware config toolset makes a lot of sense since I assume that’s how the metadata schemas would be broken out naturally.

Building it into the ASC ConfigImpl seems duplicative — initially we wanted to make ConfigImpl a /conf config however there isn’t tooling to allow authors to create bespoke configs, so we couldn’t) (thus the similar naming)

How are you envisioning the break out of the schemas themselves? And at what granularity would the schema selection be (the asset share site level, asset details page level, etc) Would the site tenant and the metadata schema tenant ever clash?

kaushalmall commented 6 years ago

@davidjgonzalez agreed that the ConfigImpl seems duplicative, can we expose the cq:conf property in the dialog, then use something like DamUtil.getTenantAssetsRoot to get the root for the AEM Sites page. From there we can get the metadata configs.

Would be nice if you can tell me what issues you ran into when you tried it. I can implement as long as we can come to an agreement on how to do it. Maybe @justinedelson has ideas.

@justinedelson is cq:conf still the property to use for should we use sling:configRef as described -- > https://sling.apache.org/documentation/bundles/context-aware-configuration/context-aware-configuration.html

kaushalmall commented 6 years ago
How are you envisioning the break out of the schemas themselves? And at what granularity would the schema selection be (the asset share site level, asset details page level, etc)
Would the site tenant and the metadata schema tenant ever clash?

The way I see it is that each tenant has their own schema under /conf/my-tenant and a corresponding Site under /content/my-tenant-site

davidjgonzalez commented 6 years ago

Note that it appears AEM stores Sling CAConfig paths in cq:conf rather than sling:configRef, so we can follow suite.

kaushalmall commented 6 years ago

@davidjgonzalez so, I was able to resolve

 Resource confResource = configResolver.getResource(resource, "settings", "dam/adminui-extension/metadataschema" );

to the appropriate conf bucket as pointed to by the cq:conf property on the /content/asset-share-commons/en/light/jcr:content node. For this discussion, lets assume I set it to /conf/asset-share-commons

But, it would still wouldn't work :-(

The SchemaFormHelper.getSchemaFormsIterator method has a getBaseFormPaths method call that sets the base forms to be tenant specific, aka, it looks at the metadataschema.home property on the /etc/tenants/mytenant node. If this property is missing, it will set the schemaExtHome variable to /conf/global/settings/dam/adminui-extension/metadataschema which doesn't match /conf/asset-share-commons/settings/dam/adminui-extension/metadataschema and the SchemaFormHelper.getSchemaFormsIterator method doesn't work.

AFAICS, for this to work, you have to setup a sling tenant by creating the /etc/tenant/asset-share-commons node with the metadataschema.home property to something like /conf/asset-share-commons/settings/dam/adminui-extension/metadataschema and have the cq:conf property set to /conf/asset-share-commons and configure the Sling Tenant Provider to make sure the request maps to the asset-share-commons.

It might be better to provide a separate implementation of MetadataProperties interface. Something like public class ContextAwareMetadataSchemaPropertiesImpl implements MetadataProperties. Then have an accepts method on the MetadataProperties interface where ContextAwareMetadataSchemaPropertiesImpl accepts if Tenant != null and MetadataSchemaPropertiesImpl accepts if Tenant = null.

WDYT?