[MAGNOLIA-6868] TemplatingFunctions for fetching content in templating: Prevent flooding logs of non resolvable JCR items Created: 10/Nov/16  Updated: 07/Jun/18  Resolved: 05/Jan/17

Status: Closed
Project: Magnolia
Component/s: core, templating
Affects Version/s: None
Fix Version/s: 5.5.1

Type: Improvement Priority: Neutral
Reporter: Christian Ringele Assignee: Roman Kovařík
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File SessionUtil.patch     Text File TemplatingFunctions.patch    
Issue Links:
supersession
supersedes MAGNOLIA-4671 Bad error handling for method for inf... Closed
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Sprint: Kromeriz 75
Story Points: 3

 Description   

The cmsfn provides:

cmsfn.contentByPath(path, workspace)]
cmsfn.nodeByPath(path, workspace)]
cmsfn.contentById(id, workspace)]
cmsfn.nodeById(id, workspace)]

Two problems (1. & 2.) when the target node does not exists because of:

  • path outdated/invalid
  • id outdated/invalid

1. Floods the logs with the thrown exceptions of trying to fetch the content.
The templater ha no way to prevent the throwing of the exceptions, even he knows it is absolute valid an item does not exists.
There are methods in jcr session to actually know if it exists before requesting it:

jcrSession.nodeExists("pathToCheck");
jcrSession.itemExists("pathToCheck");
jcrSession.propertyExists("pathToCheck");

There is no jcr method for checking if an id exists -> needs to catch the ItemNotFoundException

2. Too many unnecessary if checks needed:
-It would be nice for the templater to be able to check if a path or an id exists within a workspace, BEFORE trying to fetch the content of it.- ?hasContent in an if statement might be used instead

-I added the needed Methods to info.magnolia.templating.functions.TemplatingFunctions and info.magnolia.jcr.util.SessionUtil as a patch.-

-Here an example of how easy the code becomes, and no unnescessary exception is logged:-

    [#-- By Path (will produce logs): From --]
    [#if eventPath?has_content]
        [#assign eventNode = cmsfn.contentByPath(eventPath, "events")]
        [#if !eventNode?has_content]
            [#-- Reacting on non resolvable path --]
        [/#if]
    [/#if]

    [#-- By Path: To --]
    [#if cmsfn.contentExists(eventPath, "events")]
        [#assign eventNode = cmsfn.contentByPath(eventPath, "events")]
    [#else]
        [#-- How ever to react on non existing path. But no need to check eventNode?has_content to get into the logical 'else' --]
    [/#if]
    
    
    [#-- By Id (will produce logs): From --]
    [#if eventId?has_content]
        [#assign eventNode = cmsfn.contentById(eventId, "events")]
        [#if !eventNode?has_content]
            [#-- Reacting on non resolvable path --]
        [/#if]
    [/#if]
    
    [#-- By if: To --]
    [#if cmsfn.idExists(eventId, "events")]
        [#assign eventNode = cmsfn.contentById(eventId, "events")]
    [#else]
        [#-- How ever to react on non existing id. But no need to check eventNode?has_content to get into the logical 'else' --]
    [/#if]


 Comments   
Comment by Tomáš Gregovský [ 10/Nov/16 ]

Comment by Roman Kovařík [ 14/Dec/16 ]

We don't see a benefit from these new methods. The only think we can improve is to not log the error in info.magnolia.jcr.util.SessionUtil#getNodeByIdentifier.
For the exist checks...why don't you do cmsfn.nodeByPath("xxx", "xxx")?has_content and you don't need the 20 new methods?

Comment by Roman Kovařík [ 15/Dec/16 ]

jsimak The same code without any new method:

[#-- By Path (will produce logs): From --]
[#if eventPath?has_content && cmsfn.contentByPath(eventPath, "events")?has_content]
    [#assign eventNode = cmsfn.contentByPath(eventPath, "events")]
[#else]
xxxx
[/#if]

[#-- By Path: To --]
[#if cmsfn.contentExists(eventPath, "events")]
    [#assign eventNode = cmsfn.contentByPath(eventPath, "events")]
[#else]
    [#-- How ever to react on non existing path. But no need to check eventNode?has_content to get into the logical 'else' --]
[/#if]
Comment by Roman Kovařík [ 15/Dec/16 ]

Architecture decision: https://wiki.magnolia-cms.com/display/ARCHI/2016-12-15+Meeting+notes

Comment by Christian Ringele [ 03/Jan/17 ]

There is a huge difference of getting twice the same content, just to find out if the value/path is valid. The code you say is equivalent:

[#if eventPath?has_content && cmsfn.contentByPath(eventPath, "events")?has_content]
    [#assign eventNode = cmsfn.contentByPath(eventPath, "events")]
[#else]

But its not.
1. You get the same content twice, first in the if itself, and then within the if.
This is just stupid and bad for performance.
This would be like in Java working with the try&catch instead of writing a proper if check.
And if you really want to check it properly over one assign, that you don't get the same content twice, then there is now way around the inner double if check.

2. Its not the same, as when the front end dev gets into the else he does not know if eventPath variable itself was null or the eventPath was not resolvable. There is not easy way to differentiate between the two possibilities.

3. Also be aware that if in assign eventNode = cmsfn.contentByPath(eventPath, "events") the eventPath is a null String, then the freemarker script will not work, as the null passing into the cmsfn.contentByPath( is not catche-able with an "Unable to render embedded object: File (" like this {{[#assign eventNode = cmsfn.contentByPath(eventPath, "events")) not found.]}} (exclamation mark).
These little differences make front end developers crazy!!
If he needs to know if a path exists, its cumbersome and the code of Roman will not work for the described situation.

Tomas had this problems with it, him self and in teaching front end devs.
I did also and many templates I spoke with.

All the arguments I read from the Architecture board and in this ticket is:
We don't see a benefit... how about you do once some proper templating in a project real world or listen to those who do. You guys only argue from the Java perspective.

Comment by Jan Haderka [ 05/Jan/17 ]

Sry, but the code changes described and agreed upon were already integrated, so in order to keep clean history, I'm closing the ticket again. It would have been great if you contributed your point already back in December just after link to discussion happening in architecture have been provided.

The solution implemented removes the cause of the issue (flooding of the log), what you describe is improvement in how to work w/ content in templates. I would suggest to create new ticket with info you describe if this is what you really want to push forward.

Comment by Christian Ringele [ 12/Jan/17 ]

had regarding

It would have been great if you contributed your point already back in December

I would have loved to, but it is out of my view impossible to know when you guys suddenly discuss/take care of a ticket (this is no critics).
Sometimes tickets are not touched over months, sometimes they are suddenly taken into account within days.

If not working every day based with Jira & projects, its almost impossible to keep track of all the tickets that might be relevant:

  • especially if ever helped out in support -> over-flood of mails (all cross tickets worked on etc.)
  • maybe gave a week of training or something alike (holidays) -> impossible to detect over hundreds of mails that the one ticket changed.
  • the more tickets created, the harder to track them -> I can show you tickets I created 3 years ago that are still untouched.
  • There is no good mail filter possibility to really filter your own created when being changed out of the mail floods.

What I'm trying to say is:
It would be nice if the (magnolia) person, that created an issue, is actively quickly informed "that a ticket is now in discussion".
That would cost 10 seconds for the one starting the process.
Where scanning them all all the time for the creator is very time consuming to do.
That's why I missed the whole debate about this ticket which is really a pitty.

One creates a ticket, takes the time to create patches etc, but is not informed when his ticket is discussed.
O know you guys have to manage it with Jira, but you do it on daily bases in a PD process with duties and scoped.
In other teams its much much harder to keep up track as one is not involved in all these processes.
So I really try to ask nicely:
It would be great, instead of the simple answer "just check your jira issues" (which I did but it was already to late), to get quickly actively informed when a ticket is discussed. Especially the once somebody takes time to write a bigger text and provide patches. Otherwise its just lost invested time.

Comment by Jan Haderka [ 12/Jan/17 ]

It would be nice if the (magnolia) person, that created an issue, is actively quickly informed "that a ticket is now in discussion".

That would cost 10 seconds for the one starting the process.

It is being done, and doesn't take even 10 seconds, as soon as ticket status is changed, jira notification is sent by mail to the creator of the ticket.
If you have missed that one, my guess would be that most likely, at some point, you were getting too many of those and decided to create mail filter to delete them automatically (or put in a place you don't really look anymore). So, if that is the case, I'd suggest to either reconfigure such filter or go to jira settings for your account and change there what notifications you want to be getting. It is hard, I'm not saying it is easy, but finding right pattern for yourself to filter out information that is sent your way it is only way to keep sanity when working with project as big as Magnolia is. Having someone else than jira to drop you mail just so you don't need to modify your overzealous mail filter is not.

BTW, the note about mentioning it back in December was not a complain, merely part of explanation why now it was too late to change description and scope of the ticket as we can't manipulate history in git and code for original scope have been already merged there. I have no issue following up on separate ticket.

Comment by Christian Ringele [ 27/Feb/17 ]

Yes, BUT:
What if somebody is giving a training, on vacation or otherwise not able to scan all the tickets mails in general?
In addition I receive about 150 mails a day from Jira, from all projects and so on.
I have about 50 mail filter set, helps only partially.
We create in various projects tickets, which sometimes are not touched for months, and then suddenly they are.
So you really suggest that I keeps an eye on all projects I ever created a ticket, just to fetch the mail then suddenly an issue is discussed by you guys.

Sorry, I think this whole argument / facts from you side are very weak. It is supper bound to the way the PD team works.
One is responsible for a project in jira, a module or something alike, they have a constrained scope.
OR their duty is scanning all the new and open ticket.
Ours is not, we see things, open a ticket.

If you guys manage to change, that tickets are dead sometimes for months, then it could work.
But as long this is not the case its just a cheap excuse....

Generated at Mon Feb 12 13:03:38 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.