DXP:1549: Add support for CFs - #31
Conversation
5f02f4c to
dc973d5
Compare
| private final ResourceResolverFactory rrFactory; | ||
| private final String nodePath; | ||
|
|
||
| AssetKey(ResourceResolverFactory rrFactory, String nodePath) { |
There was a problem hiding this comment.
why do you use rrFactory here ? Can you just have Resource in constructor ?
There was a problem hiding this comment.
Resource and ResourceResolver hold inside an open JCR Session. That Session is usually created inside a try-with-resources block and closed as soon as the execution of that block is finished. What is important, as soon as the Session is closed, the related Resource and ResourceResolver become unusable. Therefore, if we pass Resource or ResourceResolver as constructor parameters to some object, we are creating a tricky coupling. Namely, the behavior and usefulness of such object will be dependent on the execution flow of some other code where the Session was created. As soon as some other code closes that Session, the object that holds the Resource or ResourceResolver will become unusable. For that reason, it is better to pass ResourceResolverFactory, which is detached from any ongoing Session. Thanks to that the object becomes independent and has truly encapsulated behavior instead of being dependent on the execution flow of the code where the Session was initiated.
Yes, opening additional Session requires additional resources, but this performance cost is extremely low and negligible compared to the cost of fixing error-prone code with spread open Sessions here and there.
There was a problem hiding this comment.
After an online discussion: decided that the Resource should be passed. Therefor, the code should be changed.
| class JSONableNode { | ||
|
|
||
| private final String pathToJCRNode; | ||
| private final ResourceResolverFactory rrFactory; |
There was a problem hiding this comment.
I dont get why we need to pass rrFactory all over the place. Once we have resource we have what we need. If you need a resolver why dont you get it from resource itself ?
There was a problem hiding this comment.
Resource and ResourceResolver hold inside an open JCR Session. That Session is usually created inside a try-with-resources block and closed as soon as the execution of that block is finished. What is important, as soon as the Session is closed, the related Resource and ResourceResolver become unusable. Therefore, if we pass Resource or ResourceResolver as constructor parameters to some object, we are creating a tricky coupling. Namely, the behavior and usefulness of such object will be dependent on the execution flow of some other code where the Session was created. As soon as some other code closes that Session, the object that holds the Resource or ResourceResolver will become unusable. For that reason, it is better to pass ResourceResolverFactory, which is detached from any ongoing Session. Thanks to that the object becomes independent and has truly encapsulated behavior instead of being dependent on the execution flow of the code where the Session was initiated.
Yes, opening additional Session requires additional resources, but this performance cost is extremely low and negligible compared to the cost of fixing error-prone code with spread open Sessions here and there.
There was a problem hiding this comment.
After an online discussion: decided that the Resource should be passed. Therefore, the code should be changed.
| resourceResolver, slingRequestProcessor, resourcePath | ||
| ) | ||
| ).map(slingInternalRequest -> slingInternalRequest.withExtension("json")) | ||
| .map(slingInternalRequest -> slingInternalRequest.withSelectors("infinity")) |
There was a problem hiding this comment.
why infinity selector that gives you the whole structure ? Have you considered using .model selector ?
There was a problem hiding this comment.
Yes, I considered using model selector and came to the conclusion that it can't be used here. Exporter Framework for Sling Models is supposed to work for Sling Resources that have a specific resourceType (https://sling.apache.org/documentation/bundles/models.html#exporter-framework-1). This is usually a case for Sling Resources that represent components on a page. Here, in turn, we are not dealing with a Sling Resource that has a specific resourceType. We are dealing with a usual dam:Asset node from /content/dam/puresight (more specifically: Content Fragment), which is pure data, without a specific resourceType set. Therefore, the model selector won't and doesn't work here.
I also considered extracting the data from CF via GraphQL, but came to the conclusion that this solution won't be effective, since setting it up will take substantially more time than the current solution without a real necessity.
There was a problem hiding this comment.
After an online discussion: probably we will need a different approach than "infinity" + manual cleanup of the output data. To be resolved later.
3 participants
📋 Type of the Changes
🛠 Changes being made
Add support for Content Fragments.
✅ Checklist