Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIO-9006: Fixed cached grid responses #1104

Closed
wants to merge 1 commit into from

Conversation

roma-formio
Copy link
Contributor

@roma-formio roma-formio commented Oct 14, 2024

Link to Jira Ticket

https://formio.atlassian.net/browse/FIO-9006

Description

Issue: When the grid requests forms from the API server, the server generates an ETag header for the response. The browser uses this ETag with the If-None-Match header in subsequent requests so the server can validate if the content has changed. If not, it returns a 304 Not Modified status, indicating to the browser to use the cached version of the response.

Example: When the grid already has 1 page with the total items equal to the maximum items per page to show, and the user tries to add a new form that should be displayed on the second page of grid items, the following issue occurs:

  1. The user navigates back to the grid.
  2. The grid sends a request to get the list of items with the If-None-Match header.
  3. The server prepares a response with the same body but a different Content-Range header because a new form was added.
  4. Based on the implementation in the Express library, the ETag only considers the body of the response and not the headers.
  5. As a result, Express generates the same ETag, which matches the If-None-Match request header.
  6. Consequently, Express sends a 304 Not Modified status, and the browser uses the cached version of the response with the old Content-Range header.
  7. Grid pagination depends on the Content-Range header to display pagination correctly. In this case, the user does not see the new available page in the pagination.

Solution: Based on the fresh function, we can use the Cache-Control request header with the no-cache value to force the Express server not to return a 304 Not Modified status. This ensures the server sends the updated response with the correct Content-Range header, allowing the grid to display the correct pagination.

How has this PR been tested?

Manually

Checklist:

  • I have completed the above PR template
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • My changes include tests that prove my fix is effective (or that my feature works as intended)
  • New and existing unit/integration tests pass locally with my changes
  • Any dependent changes have corresponding PRs that are listed above

@@ -17,7 +17,10 @@ export class FormGridBodyComponent extends GridBodyComponent implements OnDestro

load(formio: FormioPromiseService, query?: any) {
query = query || {};
return formio.loadForms({ params: query })
return formio.loadForms(
Copy link
Contributor

@brendanbond brendanbond Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't the cache generally speaking a good thing except in this edge case? If I understand this correctly, this would disable browser caching for all requests of this kind. Is there a way to limit this change to only certain cases involving pagination?

I remember something about an If-Range header, but it's been awhile...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me that the etag headers are doing the right thing. the value is changing after you submit a new form, so I don't think it's directly related to the caching policy.

Also I was testing this in remote-next and couldn't reproduce. I was also seeing the no-cache value set. Let's make sure something didn't get merged or deployed by accident.

@roma-formio
Copy link
Contributor Author

Closing this because another solution is implemented on the server side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants