[MGNLREST-574] LegacyPageWriter in pages returns empty areas if the endpoint definition not including system properties Created: 15/Nov/22  Updated: 16/May/23  Resolved: 12/Dec/22

Status: Closed
Project: Magnolia REST Framework
Component/s: None
Affects Version/s: None
Fix Version/s: 3.0.0

Type: Bug Priority: Neutral
Reporter: Canh Nguyen Assignee: Oanh Thai Hoang
Resolution: Won't Fix Votes: 0
Labels: None
Σ Remaining Estimate: Not Specified Remaining Estimate: Not Specified
Σ Time Spent: 1d 1.5h Time Spent: 1d 1.5h
Σ Original Estimate: Not Specified Original Estimate: Not Specified

Attachments: PNG File hide-nested-content.png     PNG File json-repsonse.png     PNG File without-page.png    
Issue Links:
Relates
relates to MGNLFE-370 Hello SaaS - Issue with navigation in... Closed
Sub-Tasks:
Key
Summary
Type
Status
Assignee
MGNLREST-575 Implementation Sub-task To Do  
MGNLREST-576 Review Sub-task To Do  
MGNLREST-577 Pre-Integration QA Sub-task To Do  
MGNLREST-578 QA Sub-task To Do  
Template:
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)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Epic Link: Norsu delivery endpoint (phase 1)
Team: DeveloperX
Work Started:

 Description   

Page API from Norsu pages needs "mgnl:type" get areas. If the definition is configured false for includeSystemProperties, "mgnl:type" cannot be returned.

 

Workaround: set includeSystemProperties to true and systemProperties should be empty or at least has "mgnl:type"

 

Dev note: "mgnl:type" is a special property, we should treat it like @id, @path, @name. And it will always be bypassed by the property filter.



 Comments   
Comment by Christopher Zimmermann [ 16/Nov/22 ]

I cannot remember - was this different on JCR delivery endpoint?

Comment by Canh Nguyen [ 17/Nov/22 ]

There are a lot differences between them. Norsu has nested content while JCR has sub nodes for areas and components for pages. Norsu delivery endpoint has multiple MessageBodyWriter implementations so we implemented property filtering in a wrapper node so that we don't have to re-implement this feature on every MessageBodyWriter.

JCR has a complex NodeWrapper system, it unwraps the node to get the type. We didn't implement this pattern to Norsu because it's very complex and difficult to debug.

Comment by Oanh Thai Hoang [ 08/Dec/22 ]

Discovery output:

mgnl:type with previous code in REST module is considered is system property (since it start with mgnl). So set includeSystemProperties is false will not write nested content (area and component). See code in info.magnolia.pages.domain.AreaContainer#areas

 

Propose: We always write response for mgnl:type (whether includeSystemProperties is false or true)

 

Consequence: always write response for nested content in LegacyPageWriter

 

With following endpoint:

$type: deliveryEndpoint_v2
includeSystemProperties: false
depth: 5
nodeTypeMappings:
  pages:
    $type: pagesResolver_v2 

In case we wanna hide nested content. We can configure additional configuration includeNestedContent is false. See

$type: deliveryEndpoint_v2
includeSystemProperties: false
includeNestedContent: false
depth: 5
nodeTypeMappings:
  pages:
    $type: pagesResolver_v2 

Comment by Christopher Zimmermann [ 08/Dec/22 ]

Is includeNestedContent just about pages? Does it have relevance anywhere else?

What does "depth:5" relate to then. Is depth related to the nested content.... or to sub pages... or is it not used anymore?

Comment by Oanh Thai Hoang [ 08/Dec/22 ]

Is includeNestedContent just about pages? Does it have relevance anywhere else?

It can be used without page legacy. Ex: if we configure includeNestedContent is false.

$type: deliveryEndpoint_v2
includeSystemProperties: false
includeNestedContent: false 

What does "depth:5" relate to then. Is depth related to the nested content.... or to sub pages... or is it not used anymore?

No related to nested content. Sorry for confuse. I clone from my yaml

Comment by Christopher Zimmermann [ 09/Dec/22 ]

I find it difficult to make a decision. In general a goal is to make the Norsu endpoint work similar to JCR endpoint so that developers have an easy time working with SaaS or DXCORE. This also applies to our sample projects and documentation. Life will be a lot easier for everyone the more similar a SaaS to a DXCORE project is.

Re: mgnl:template:
So, I like the intention to make it easier to configure - but in endpoint on JCR for pages one must also configure the "mgnl:template", so I dont find it bad that one needs to configure in on Norsu as well.
If we make change to add "mgnl:template" is included by default - then I would want to make the change on Norsu and JCR endpoints. But to me it seems that we should do that later, at least after TPR. (Maybe when introducing v3 of endpoints.)

Re: includeNestedContent
Can you help me understand what this is for? Without this new property, is it possible to get just the "top" level content of a page, or do you alwaays get the entire page returned? Is the usecase for this building a navigation? Is there no other way to get just the "top" content?
For consistancy, I think the "depth" parameter should still work when getting a page and its content. Then I think we don't need to introduce a new property.

Comment by Oanh Thai Hoang [ 12/Dec/22 ]

Re: mgnl:template:

mgnl:type it same meaning like nodetype and do not relate to mgnl:template.
 

 

Re: includeNestedContent
{quote)

 

Without change any thing. includeSystemProperties false is possible to get just "top" level content of a page. As ticket description "LegacyPageWriter in pages returns empty areas if the endpoint definition not including system properties".

There has a use case for return a list of "top" level of pages without nested content is searching page name in find bar. Or using rest end point query full text search (ex: http://localhost:8080/container/.rest/delivery/newnorsu/v1/?q=react-mi)

In conclusion. As result of new property is unacceptable, I think includeSystemProperties is enough for now, since childNodetype do not support to filter nested area and component

  • includeSystemProperties false is workaround of use case return "top" level pages.
  • includeSystemProperties true can return whole entire page

And So I close ticket as won't fix at the moment. How does that sound?

Generated at Mon Feb 12 07:01:11 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.