[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: |
|
||||||||
| Issue Links: |
|
||||||||
| 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:
1. Floods the logs with the thrown exceptions of trying to fetch the content. 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: - -
[#-- 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. |
| 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. 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). Tomas had this problems with it, him self and in teaching front end devs. All the arguments I read from the Architecture board and in this ticket is: |
| 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
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). If not working every day based with Jira & projects, its almost impossible to keep track of all the tickets that might be relevant:
What I'm trying to say is: One creates a ticket, takes the time to create patches etc, but is not informed when his ticket is discussed. |
| Comment by Jan Haderka [ 12/Jan/17 ] |
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. 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: Sorry, I think this whole argument / facts from you side are very weak. It is supper bound to the way the PD team works. If you guys manage to change, that tickets are dead sometimes for months, then it could work. |