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

WW-5270 Fix forwarding from Struts excluded URL #648

Conversation

kusalk
Copy link
Member

@kusalk kusalk commented Dec 19, 2022

Implements WW-5270

@kusalk kusalk marked this pull request as draft December 20, 2022 01:20
@kusalk kusalk force-pushed the WW-5270-forwarding-from-excluded-url branch from 803833d to 7c61800 Compare December 20, 2022 11:06
@kusalk
Copy link
Member Author

kusalk commented Dec 20, 2022

My initial ThreadLocal based approach to reworking the request cleanup process was not functioning as intended. I've reverted to tracking filter recursion as a request attribute but have still retained the reworked logic.

This means:

  • ActionContext is not cleared until the first StrutsPrepareFilter execution for that request has completed - this is irrespective of whether the ActionContext was generated there or not. This can occur when a Struts excluded URL forwards to a non-excluded one. Waiting until the ActionContext is cleared is required by SiteMesh.
  • The MultiPart component of a request is only cleaned up in the filter execution in which it was initially wrapped. This is retaining the original behaviour as before. I believe if we try to cleanup at any other stage, it will fail as the instanceof check will fail - although I haven't investigated this closely.

The implementation is a little confusing at first glance (and was also why I favoured the ThreadLocal based approach) but it's the best I've got right now - open to suggestions.

@kusalk kusalk marked this pull request as ready for review December 20, 2022 11:19
Copy link
Member

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

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

Sorry, I totally missed this PR, LGTM 👍

@lukaszlenart
Copy link
Member

@kusalk could you resolve conflicts? I see nothing but GH blocks merging this PR :\

@kusalk kusalk force-pushed the WW-5270-forwarding-from-excluded-url branch from 7c61800 to f524dc8 Compare January 30, 2023 11:42
@kusalk kusalk force-pushed the WW-5270-forwarding-from-excluded-url branch from f524dc8 to 0ce254d Compare January 30, 2023 11:55
@kusalk
Copy link
Member Author

kusalk commented Jan 30, 2023

@lukaszlenart All good, should be sorted now. We shipped this change in Confluence 8.0.2 on Dec 22nd and there have been no reported issues.

@lukaszlenart lukaszlenart merged commit 7d40a81 into apache:master Jan 30, 2023
@kusalk kusalk deleted the WW-5270-forwarding-from-excluded-url branch January 30, 2023 15:17
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.

2 participants