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-5406 Ensure Action excluded patterns are reinjected #910

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

kusalk
Copy link
Member

@kusalk kusalk commented Apr 8, 2024

@kusalk kusalk force-pushed the WW-5406-excluded-patterns branch from b27a28d to 6f6b136 Compare April 9, 2024 00:34
@kusalk kusalk force-pushed the WW-5406-excluded-patterns branch from 6f6b136 to efa2042 Compare April 9, 2024 00:42
@kusalk
Copy link
Member Author

kusalk commented Apr 9, 2024

Ignore SonarCloud results, they are incorrect again

@kusalk kusalk marked this pull request as ready for review April 9, 2024 01:43
@lukaszlenart
Copy link
Member

Ignore SonarCloud results, they are incorrect again

Probably because the last scan was for Struts 7 branch, I will fix that

@kusalk kusalk requested a review from lukaszlenart April 9, 2024 05:06
Comment on lines +351 to +370
@Inject(value = StrutsConstants.STRUTS_ACTION_EXCLUDE_PATTERN_SEPARATOR, required = false)
public void setActionExcludedPatternsSeparator(String separator) {
this.actionExcludedPatternsSeparator = separator;
}

@Inject(value = StrutsConstants.STRUTS_ACTION_EXCLUDE_PATTERN, required = false)
public void setActionExcludedPatterns(String excludedPatterns) {
this.actionExcludedPatterns = buildExcludedPatternsList(excludedPatterns, actionExcludedPatternsSeparator);
}

private static List<Pattern> buildExcludedPatternsList(String patterns, String separator) {
if (patterns == null || patterns.trim().isEmpty()) {
return emptyList();
}
return unmodifiableList(Arrays.stream(patterns.split(separator)).map(String::trim).map(Pattern::compile).collect(toList()));
}

public List<Pattern> getActionExcludedPatterns() {
return actionExcludedPatterns;
}
Copy link
Member

Choose a reason for hiding this comment

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

It's getting crowd'y here, what about moving all the injectable options into DispatcherOptions? It doesn't have to happen now, but I can create a ticket to address that later

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep I think that's a fair call, I'll create an issue for it

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -43,6 +43,8 @@ public class StrutsPrepareFilter implements StrutsStatics, Filter {
protected static final String REQUEST_EXCLUDED_FROM_ACTION_MAPPING = StrutsPrepareFilter.class.getName() + ".REQUEST_EXCLUDED_FROM_ACTION_MAPPING";

protected PrepareOperations prepare;

@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Please document why it's deprecated and what to use instead. Also since should be added, and feel free to create a ticket in JIRA to remove this deprecated element in Struts 7. An example deprecation

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep let me rectify
Regarding a ticket for the deletion - my assumption would be that all deprecated elements are deleted in the next major version. Do we still require individual follow-up issues in the case of a deprecation?

Copy link
Member

Choose a reason for hiding this comment

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

We can have one general ticket to remove all the deprecated options, just to be sure we won't miss this one at some point (so since would be needed)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed and created this general issue - WW-5411

@kusalk
Copy link
Member Author

kusalk commented Apr 10, 2024

Ah wait, just realised I made a breaking change in PrepareOperations.java, let me fix that

@kusalk
Copy link
Member Author

kusalk commented Apr 10, 2024

Done - looks like the quality gate is failing due to the deprecations

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@kusalk kusalk merged commit 929a601 into master Apr 11, 2024
8 of 9 checks passed
@kusalk kusalk deleted the WW-5406-excluded-patterns branch April 11, 2024 04:27
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