DataLayer Extension Pattern

[kyle giovannetti]

Hello,

With respect to the Data Layer;

I currently find that it is difficult for custom components (non core-components) to implement the data layer.

The data layer objects, and most of the methods that do some heavy lifting (example: generating an ID) are not exported and thus cannot be accessed outside of the project.

There are two issues:
1) It is challenging to implement the data layer for components that are not part of core components

2) it is challenging to extend/modify the data layer for custom component models that are overlays/wrappers for a core components model

The current implementation requires that a client who wishes to provide data layer for their own components must either re-implement the data layer interfaces and all associated logic;

or, if their component has a super type that is a core component, then in their model they may adapt the request to the super type model and get the data layer, and then wrap that data layer to add/remove/modify anything they want; keeping in mind that the data layer object returned from the super model does not take into account any of the custom logic they may have overridden in their implementation. 

Proposed solution:

I propose that a `DataLayerBuilder` be made available to provide an easy API for people to deliver data layer integration from their custom components.

I already have the workings of such a builder, and will contribute/collaborate on it within the CC project if there is interest in providing this functionality.

We wouldn't have to actually do it this way internally to the project, but for the AbstractComponentImpl the data layer could be built using something like this:

ComponentData data = DataLayerBuilder.forComponent()

    .withId(this::getId)

    .withTitle(this::getDataLayerTitle)

    .withDescription(this::getDataLayerDescription)

    .withText(this::getDataLayerText)

    .withLastModifiedDate(this::getDataLayerLastModifiedDate)

    .withLinkUrl(this::getDataLayerLinkUrl)

    .withType(resource::getResourceType)

    .build();

Is there any interest in something like this?

Kyle

[Stefan Seifert]

+1 for having such a feature in core components – I’ve encountered the same problem.

 

the method to generate a unique ID for the current resource path (including all the special handlings) should also be put into a separate class available via the core components API to be re-used in custom components.

 

stefan

 

--
You received this message because you are subscribed to the Google Groups "AEM Core Components Developer Community" group.
To unsubscribe from this group and stop receiving emails from it, send an email to aem-core-componen...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/aem-core-components-dev/134760af-063e-4433-b16d-08eb3590e798o%40googlegroups.com.

[Rajeev Yadav]

I would love it too.

[kyle giovannetti]

Ok, great!

I'll clean up and finalize what i've been working on, which included making the ID easy to get, and then submit a PR for review/discussion. 

I did have one question that someone here might be able to elaborate on.

The Component#getId() method is annotated as @Nullable, but is used in the ComponentData (for all core components that implement AbstractComponent).

The way ComponentData#getJson() works leads me to believe that ComponentData#getId() should actually be @NotNull (i.e. the Data Layer requires an ID).

This isn't an issue inside of core-components because AbstractComponent#getId() can safely be annotated as @NotNull even if the interface says @Nullable.

However, it would impact the design of the DataLayerBuilder if i am correct and the ComponentData#getId() should not be null.

Can someone please confirm that this statement is true (or not):
"Component data layer requires a non-null ID"

- Kyle

[Stefan Seifert]

>Can someone please confirm that this statement is true (or not):
>"Component data layer requires a non-null ID"

i think the statement is true, but i suppose getId() is marked as @Nullable for backward compatibility, because in previous versions of the core components most components did not generate an id by default.

stefan

[Rajesh Dwivedi]

+1

This is a very practical issue and we also face this. Certainly very much interested in this. Thank you for taking this into consideration.

Regards,
Rajesh Dwivedi

--

[kyle giovannetti]

Hello,

Another quick question:

I see that the PageDataImpl class extends the ContainerDataImpl class;

however, the PageData interface does not extend the ContainerData interface.

Does it make sense that PageData is a ContainerData?

The effect of the difference right now is that the PageDataImpl has the method "getShownItems()" whereas the public interface for PageData does not make that method available.

One of these two options is probably the solution:
1) PageDataImpl#getShownItems() should not be defined; or,

2) PageData#getShownItems() should be defined

Can someone weigh in on if the intent is for PageData to have 'shownItems'?

Thanks,

Kyle

On Monday, June 15, 2020 at 4:33:33 PM UTC-3, kyle giovannetti wrote:

[kyle giovannetti]

For any who are interested, i would appreciate feedback on the current state of the implementation.
It didn't turn out as straight forwards as I had hoped, and I will continue to try to find ways to simplify the implementations.

My main objectives were to provide an easy to use fluent-style API that by following your IDEs autocomplete/suggestions you could build a compliant data layer model without the risk of going wrong;

while, at the same time, not changing the existing DataLayer APIS.

Not changing the existing DataLayer models made the implementation a little more tricky/messy than expected; however, most of that complexity is not visible outside of the CC project.

You can check it out here: https://github.com/ky940819/aem-core-wcm-components/tree/data_layer_builder

The last four commits in this branch replace the existing DataLayer implementations with the builder to give real-world examples
https://github.com/ky940819/aem-core-wcm-components/commit/76f638a3e1cb00cec58668e025f1dfad7e461081

https://github.com/ky940819/aem-core-wcm-components/commit/b871ee0e58b7b8c5692462522a1ada3c2b693666

https://github.com/ky940819/aem-core-wcm-components/commit/85578d4d50ce1c42ed6af48ab3ac8a9062c76a56

https://github.com/ky940819/aem-core-wcm-components/commit/078f11ffe22cbdfa584b959a04ef1edc29379fe3

The tests are a good place to look for examples as well:
https://github.com/ky940819/aem-core-wcm-components/blob/0c3cd19cfaba8b96b0ba9d5fcd9deebdcb888dcf/bundles/core/src/test/java/com/adobe/cq/wcm/core/components/internal/models/v1/datalayer/builder/DataLayerBuilderTest.java


Please let me know if you have any feedback/thoughs.

Kyle

On Monday, June 15, 2020 at 4:33:33 PM UTC-3, kyle giovannetti wrote:

[Stefan Seifert]

hello kyle.

i've had a brief look, in general the patterns used to define the data layer in each models implementation looks very clean to me, and far better than the pattern with the set of internal abstract methods used before.

without looking into the details there are really a lot of classes involved for getting it to work (e.g. all the "fieldwrapper classes"). but the API looks quite clean [1].

do we need the caching support here? not sure if this is really required, has a big performance gain compared to the complexity of implementation it comes with.

can we remove the default implementations in the interfaces throwing UOE? in my pov this is really an anti-pattern (although used by the model interfaces, there was a discussion on this list some time ago, but these interfaces are no model interfaces).

overall, this looks the right way to go for me. it would be good to get some feedback here from the core components team as well!

stefan

[1] https://github.com/ky940819/aem-core-wcm-components/tree/data_layer_builder/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/models/datalayer/builder

>--
>You received this message because you are subscribed to the Google Groups
>"AEM Core Components Developer Community" group.
>To unsubscribe from this group and stop receiving emails from it, send an

>email to mailto:aem-core-componen...@googlegroups.com.

>To view this discussion on the web visit

>https://groups.google.com/d/msgid/aem-core-components-dev/d3827e5b-2190-
>4958-b4fd-
>e484502951d5o%40googlegroups.com?utm_medium=email&utm_source=footer.

[kyle giovannetti]

Hello,

Thanks for the feedback.

Personally I really don't like the default methods in the interfaces, it makes me think that we are saying that adherence to the interface is entirely optional. I understand why it's done, but, in general, it just makes it more likely that I'll forget to implement something that's required. A topic for another day...

I only included the defaults because there are tests that ensure that they are there.

I'll look at the field wrappers, yes it's a lot of classes and could probably be cleaned up. 

The caching was simply because I don't know how expensive a field supplier might be, it might just be returning a constant value, or could be a hugely expensive operation, for example, the ID generating isn't trivial. 

So for me, the lazy caching strategy makes the most sense, and is what most sling models do: do the work only if needed, but only do it at most once.

The eager caching I can't see a good use case for, I guess if you wanted to build the datalayer model twice and compare them, it was mostly just implemented because it was trivial to do so. 

The only use case I can think of for lazy non-caching is if the datalayer model is generated by an abstract class for which the concrete implementations are required to extend the data layer model. It's probably a trivial gain in this situation, especially if it's lazy and the original suppliers aren't going to be called anyway. 

Kyle

 

To unsubscribe from this group and stop receiving emails from it, send an email to aem-core-componen...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/aem-core-components-dev/774c2cf1ec6f47008fa9d2eecddad3f1%40mailx01.intern.pro-vision.de.

[Stefan Seifert]

>I only included the defaults because there are tests that ensure that they
>are there.

maybe an exclusion should then be defined in the test for this package, as it does not contain model interfaces.

>The caching was simply because I don't know how expensive a field supplier
>might be, it might just be returning a constant value, or could be a hugely
>expensive operation, for example, the ID generating isn't trivial.

my personal view is: sling models getters (especially for general purpose models like the core component ones) should be designed anyway to be as less expensive as possible, because you never know often they are called from the HTL code (and you do not want to force HTL code to cache values in the HTL code itself). so it's somewhat the same case for the data layer. and the data layer is only generated anyways when it's enabled via caconfig.

stefan

[kyle giovannetti]

Here's a slightly different version that does away with the field wrapper classes and the "extender" classes:
https://github.com/ky940819/aem-core-wcm-components/tree/eliminate_field_wrappers
diff: https://github.com/ky940819/aem-core-wcm-components/compare/data_layer_builder...ky940819:eliminate_field_wrappers?expand=1

Kyle 

On Monday, June 15, 2020 at 4:33:33 PM UTC-3, kyle giovannetti wrote:

[Vlad Băilescu]

Hi Kyle,

Thanks a lot for all your recent contributions, they are appreciated by our entire team!

I really like the fluent builder and it seems it would help people developing custom components to easily add DataLayer support to them. I'm also wondering, just like Stefan, if it makes sense to separate implementations from the builder interfaces as replacing the default implementation would not be that straightforward (or easy).

Best,

Vlad

--

You received this message because you are subscribed to the Google Groups "AEM Core Components Developer Community" group.
To unsubscribe from this group and stop receiving emails from it, send an email to aem-core-componen...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/aem-core-components-dev/bedea2e5-8179-4519-bbf2-2457aac9d7e4o%40googlegroups.com.

[kyle giovannetti]

Hello,

Here's another tweaked version that eliminates the interface/implementation separation.
I agree that as a utility there probably isn't a significant benefit to the separation of the interfaces and implementation.

branch: https://github.com/ky940819/aem-core-wcm-components/tree/remove_interfaces

diff: https://github.com/ky940819/aem-core-wcm-components/compare/eliminate_field_wrappers...ky940819:remove_interfaces?expand=1

Kyle

On Monday, June 15, 2020 at 4:33:33 PM UTC-3, kyle giovannetti wrote:

[Vlad Băilescu]

Hi,

That's nice.

Do you think the DataLayerSupplier and caching strategies are needed though? Seems overly engineered TBH. I share Stefan's opinion that expensive operations are best cached in the Sling Models getters.

Best,

Vlad

[kyle giovannetti]

Hello,

This is the builder without the caching:
https://github.com/ky940819/aem-core-wcm-components/tree/remove_cache_strategy

My only thought on that is that I didn't want to make the assumption that the supplied values for the data layer would always be method references to a sling model getter.
A couple areas where it would need to be reworked within the CC project is the asset data on image model, and the last modified date on the abstract component implementation.
These values are supplied to the builder without any caching internal to the model because they aren't used anywhere else in the model. 

For example, this is from the image model.

    @Override

    @NotNull

    protected ImageData getComponentData() {

        return DataLayerBuilder.extending(super.getComponentData()).asImageComponent()

            .withTitle(this::getTitle)

            .withLinkUrl(this::getLink)

            .withAssetData(() ->

                Optional.ofNullable(this.fileReference)

                    .map(reference -> this.request.getResourceResolver().getResource(reference))

                    .map(assetResource -> assetResource.adaptTo(Asset.class))

                    .map(DataLayerBuilder::forAsset)

                    .map(AssetDataBuilder::build)

                    .orElse(null))

            .build();

    }

Without any caching, the following calls

componentData.getAssetData().getLastModifiedDate()
componentData.getAssetData().getLastModifiedDate()

would execute the following block twice:

 Optional.ofNullable(this.fileReference)

    .map(reference -> this.request.getResourceResolver().getResource(reference))

    .map(assetResource -> assetResource.adaptTo(Asset.class))

    .map(DataLayerBuilder::forAsset)

    .map(AssetDataBuilder::build)

    .orElse(null)

It would also execute the following block twice:

() -> new Date(

    Optional.of(asset.getLastModified())

        .filter(lastMod -> lastMod > 0)

        .orElseGet(() -> Optional.ofNullable(asset.adaptTo(ValueMap.class))

            .map(vm -> vm.get(JcrConstants.JCR_CREATED, Calendar.class))

            .map(Calendar::getTimeInMillis)

            .orElse(0L)))

I do understand that there is some added complexity for wrapping the data in a cache, but i'm not certain that everyone would understand the implications of not doing the caching within their own model.
At least, that was my thought when writing it. I'm fine with whatever makes most sense to the rest of the project.

Kyle

[kyl...@gmail.com]

Any other thoughts on this?
I'd like to clean this up and submit a PR before it stagnates.

Would really appreciate more feedback.

Thanks,

Kyle

[Vlad Băilescu]

Hi Kyle,

Indeed, it was not immediately obvious for me that calling multiple methods from the AssedData would trigger the code for building the AssetData and getting it's value map multiple times. Those are indeed potentially expensive. Still trying to figure out how we can avoid that (except for using the previous caching).

BTW, we're preparing a 2.11.0 release of the Core Components. I guess this refactoring will not make it into this release, but we can follow up with a new release after that.

Best,

Vlad

--

You received this message because you are subscribed to the Google Groups "AEM Core Components Developer Community" group.
To unsubscribe from this group and stop receiving emails from it, send an email to aem-core-componen...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/aem-core-components-dev/8510fc55-04bf-4575-8dd0-9c4e95946e06n%40googlegroups.com.

[kyl...@gmail.com]

Hi Vlad,

I think it would be safer to spend a little bit more time reviewing it with a few more eyes on it to ensure nothing is released that would cause breaking changes to fix.
I have submitted the simplified non-caching variant as a PR. 

I will look at it and see if i can come up with a clean way of resolving the caching inefficiency; however, I think that the most important thing from a review perspective is making sure that its fit for purpose and that we are satisfied with the public utilities such that any further performance improvements are minor non-breaking changes. 

Kyle