[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: |
|
|||||||||||||||||||||||||
| Sub-Tasks: |
|
|||||||||||||||||||||||||
| 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: | ||||||||||||||||||||||||||
| 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 ---------------------------------- |
| 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. 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: |