[MAGNOLIA-8869] ResponseContentTypeVoter returns server default if request has no extension Created: 07/Mar/23  Updated: 11/Sep/23  Resolved: 10/May/23

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: None
Fix Version/s: 6.2.34

Type: Bug Priority: Major
Reporter: Teresa Miyar Assignee: Chuong Doan Huy
Resolution: Fixed Votes: 0
Labels: None
Σ Remaining Estimate: Not Specified Remaining Estimate: Not Specified
Σ Time Spent: 3d 3.5h Time Spent: 3d 0.5h
Σ Original Estimate: Not Specified Original Estimate: Not Specified

Issue Links:
Problem/Incident
Relates
relates to MGNLCACHE-188 Cache endpoint does not correctly set... Open
relates to MGNLCACHE-290 Replace deprecated ResponseContentTyp... Closed
relates to MAGNOLIA-8908 DOC : introduce "emptyExtensionVote" ... Closed
relates to MAGNOLIA-8882 Replace ResponseContentTypeVoter as i... Closed
Sub-Tasks:
Key
Summary
Type
Status
Assignee
MAGNOLIA-8870 Implement Sub-task Completed Chuong Doan Huy  
MAGNOLIA-8871 Review code Sub-task Completed Milan Divilek  
MAGNOLIA-8872 Pre-int QA Sub-task Completed Milan Divilek  
MAGNOLIA-8873 Final QA Sub-task Completed Milan Divilek  
Template:
Acceptance criteria:
Empty
Task DoD:
[X]* Doc/release notes changes? Comment present?
[X]* Downstream builds green?
[X]* Solution information and context easily available?
[X]* Tests
[X]* 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: Support
Sprint: DevX 35, DevX 36, DevX 37
Story Points: 2
Team: DeveloperX
Work Started:
Approved:
Yes

 Description   

ResponseContentTypeVoter uses the extension of request if none set, it uses the Magnolia server default extension, so when using a delivery endpoint, it is not cached because in falls into:

/modules/cache/config/contentCaching/defaultPageCache/browserCachePolicy/policies/dontCachePages

resolves as html and sets the no cache headers.

code here:

String defaultExtension = ServerConfiguration.getInstance().getDefaultExtension();

in class info.magnolia.cms.beans.config.MIMEMapping

I guess fixing this issue will make many installations work in a different way

To reproduce just open this link https://demopublic.magnolia-cms.com/.rest/delivery/tours/v1

 ----------------------------------
Development decision :
Because ResponseContentTypeVoter was deprecated so we need to replace with RequestExtensionVoter which is a replacement for it (MGNLCACHE-290). And in order to make the class more configurable, we decided to add new flag named "emptyExtensionVote" to control this voter behavior in case of empty extension. Default value if not set is true (for backward compatibility). So in case of rest endpoint, if users want to cache it, they just need to add the flag to the dontCachePages voter and set its value to false.



 Comments   
Comment by Viet Nguyen [ 30/Mar/23 ]

Most of customers do not use ".html" as page's extension for the modern world wide web. So changing default value of "/server@defaultExtension" from "html" to an empty string would help in this case and also other use cases. Documentation warning would help legacy customers being aware.

Comment by Chuong Doan Huy [ 11/Apr/23 ]

Currently "dontCachePage" config tried to detect if a url is a page based on its extension (.html for example). But in case url has no extension (most popular these day as viet.nguyen pointed out), this guess mechanism is not correct as reported in this ticket. Unfortunately, there are no ways to detect if a url is a page or a rest request or even an image...if they have no extension. With that said, "dontCachePage" config can not work properly as expected.

So i can only think of "Remove "dontCachePage" config completely as we can not detect if a url is a page or not if it has no extension"

What do you think tmiyar, miruela, czimmermann, viet.nguyen ? Do you have any other suggestions ? Thank.

Comment by Teresa Miyar [ 11/Apr/23 ]

chuong.doan I think it would be better, that, if not possible to get the content-type for it, do not fall back into the .html unless explicitly indicated, so I would extend the rule, add a flag usedefaultextension as true (for backward compatibility) that customers can disable, then it will be the customer the one adding extra voters for what they need.

Comment by Chuong Doan Huy [ 11/Apr/23 ]

That's very nice suggestion, totally agree, thanks tmiyar. Let's see if other people have any opinions.

Comment by Christopher Zimmermann [ 11/Apr/23 ]

Not my area of expertise. But sounds fine to me.

Comment by Mercedes Iruela [ 12/Apr/23 ]

Sounds good to me also!

Comment by Chuong Doan Huy [ 25/Apr/23 ]

Changing to RequestExtensionVoter works. Because RequestExtensionVoter return true for empty extension, i made an improvement adding a configurable flag that can control returning true/false for more dynamic using.

Comment by Milan Divilek [ 03/May/23 ]

After updating to cache module with this fix (5.9.6-SNAPSHOT) 12 ui test starts consistently fail on jenkins

it.info.magnolia.functionaltests.PagesCoreFunctionalTests.publishDeletionSinglePage(PageObjects,Navigator,MagnoliaHttpClient)
it.info.magnolia.functionaltests.PagesCoreFunctionalTests.restoreVersion(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaAngularFunctionalTests.addComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaAngularFunctionalTests.editComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaAngularFunctionalTests.moveComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaReactFunctionalTests.addComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaReactFunctionalTests.editComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaReactFunctionalTests.moveComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaVueFunctionalTests.addComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaVueFunctionalTests.editComponent(PageObjects,Navigator)
it.info.magnolia.functionaltests.PagesSpaVueFunctionalTests.moveComponent(PageObjects,Navigator)
it.info.magnolia.integrationtests.GzipFilterTest.pageShouldBeGZippedIfClientDoesSupportIt

Missing port to master - ResponseContentTypeVoter is still used there.

Comment by Milan Divilek [ 03/May/23 ]

I did manual test replicating behaviour of PagesCoreFunctionalTests.publishDeletionSinglePage.
I saw different behaviour with fix and without related to browser cache caused by different headers (Pragma and Cache-Control headers).

Headers without fix:

milan@CZN151:~/dev/workspace/master/ce [MAGNOLIA-8869]$ curl http://localhost:8080/magnoliaPublic/ccc.html -i
HTTP/1.1 200 
Set-Cookie: csrf=bhwL7J4SaLiGLh_NAGAryXn6wh7_TsUqR8tk8L4lOGo:AAABh-D2rcs:DHYODx5X9Wv3i8MqfxTepw; Path=/magnoliaPublic; SameSite=Lax
Pragma: no-cache
Cache-Control: no-cache, no-store, must-revalidate, max-age=0
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Last-Modified: Wed, 03 May 2023 09:34:35 GMT
Content-Type: text/html;charset=UTF-8
Content-Length: 1119
Date: Wed, 03 May 2023 09:34:35 GMT

Headers with fix:

milan@CZN151:~/dev/workspace/master/ce [MAGNOLIA-8869]$ curl http://localhost:8080/magnoliaPublic/ddd.html -i
HTTP/1.1 200
Set-Cookie: csrf=fmGHbzWFhrkEZ9mMWNxYRgY0ZKbdMvpvnPmeBjq-Q3o:AAABh-DqojU:ZZ8Sp5Z7oKi2ARKKPsqFTw; Path=/magnoliaPublic; SameSite=Lax
Cache-Control: max-age=600, public
Expires: Wed, 03 May 2023 09:31:26 GMT
Last-Modified: Wed, 03 May 2023 09:21:26 GMT
Content-Type: text/html;charset=UTF-8
Content-Length: 1123
Date: Wed, 03 May 2023 09:21:26 GMT

Use case replicating publishDeletionSinglePage test:
1. visit magnoliaPublic:8080/ccc.html
2. magnoliaAuthor - delete ccc page
3. magnoliaAuthor - publish deletion of ccc page
4. WITH FIX - visit magnoliaPublic:8080/ccc.html you will still see the page from browser cache - then refresh the page cmmnd+r - you get 404
4. WITHOUT FIX - visit magnoliaPublic:8080/ccc.html you will get 404

Generated at Mon Feb 12 04:36:31 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.