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-5352 Refactor ParametersInterceptor #831

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

kusalk
Copy link
Member

@kusalk kusalk commented Jan 3, 2024

WW-5352

General clean up in preparation for #832

@kusalk kusalk force-pushed the WW-5352-parameter-annotation-2 branch from 9afea18 to 5c7a6bc Compare January 3, 2024 10:22
@kusalk kusalk force-pushed the WW-5352-parameter-annotation-2 branch from 5c7a6bc to 8a245e8 Compare January 3, 2024 10:34
Base automatically changed from WW-5352-parameter-annotation to master January 3, 2024 12:04
@kusalk kusalk marked this pull request as ready for review January 3, 2024 12:08
@@ -32,37 +32,37 @@ public interface AcceptedPatternsChecker {
* @param value to check
* @return object containing result of matched pattern and pattern itself
*/
public IsAccepted isAccepted(String value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Redundant in interfaces

@@ -58,7 +58,7 @@ public String getName() {
@Override
public String getValue() {
String[] values = toStringArray();
return (values != null && values.length > 0) ? values[0] : null;
Copy link
Member Author

Choose a reason for hiding this comment

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

#toStringArray never returns a null value

@@ -132,138 +133,149 @@ static private int countOGNLCharacters(String s) {
@Override
public String doIntercept(ActionInvocation invocation) throws Exception {
Copy link
Member Author

Choose a reason for hiding this comment

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

Used early returns to reduce nesting and improve readability

* <p>
* In this class this is a no-op, since the parameters were fetched from the same location.
* In subclasses both retrieveParameters() and addParametersToContext() should be overridden.
* </p>
*/
protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) {
}

protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted a bunch of helper methods to make this method much more readable

boolean acceptableParamValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue()));
if (hasParamValuesToExclude() || hasParamValuesToAccept()) {
// Additional validations to process
acceptableParamValue &= acceptableValue(param.getName(), param.getValue());
Copy link
Member Author

Choose a reason for hiding this comment

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

&= doesn't short-circuit

@@ -338,18 +344,17 @@ protected boolean acceptableName(String name) {
return false;
}
boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) && isAccepted(name);
if (devMode && accepted) { // notify only when in devMode
Copy link
Member Author

Choose a reason for hiding this comment

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

Cleaned out comments for behaviours which are fairly obvious from code

@kusalk kusalk requested a review from lukaszlenart January 4, 2024 00:57
Copy link

sonarqubecloud bot commented Jan 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

10 Security Hotspots
28.7% Coverage on New Code (required ≥ 80%)
4.1% Duplication on New Code (required ≤ 3%)
E Security Rating on New Code (required ≥ A)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

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

@kusalk
Copy link
Member Author

kusalk commented Jan 5, 2024

@lukaszlenart Any idea how to configure a second base branch for SonarCloud dedicated to 7.x so we don't get wonky results?

@kusalk kusalk requested a review from lukaszlenart January 5, 2024 15:52
@kusalk kusalk merged commit ecd02de into master Jan 6, 2024
9 of 10 checks passed
@kusalk kusalk deleted the WW-5352-parameter-annotation-2 branch January 6, 2024 15:14
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