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-5429 Log parameter annotation issues at ERROR level when in DevMode #969

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

kusalk
Copy link
Member

@kusalk kusalk commented Jun 18, 2024

@kusalk kusalk force-pushed the WW-5429-param-anno-log branch 3 times, most recently from f1ddc88 to 35ca03c Compare June 18, 2024 09:27
@kusalk kusalk force-pushed the WW-5429-param-anno-log branch from 35ca03c to b96cf2c Compare June 18, 2024 09:36
@@ -119,7 +119,9 @@ public interface ValidationAware {
*
* @return <code>(hasActionErrors() || hasFieldErrors())</code>
*/
boolean hasErrors();
default boolean hasErrors() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added default implementation which matches the JavaDoc, makes implementing this class simpler

@@ -42,7 +42,7 @@ public ErrorMessageBuilder errorSettingExpressionWithValue(String expr, Object v
return this;
}

private void appenExpression(String expr) {
private void appendExpression(String expr) {
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 typo

*/
public class DebugUtils {

public static void notifyDeveloperOfError(Logger log, Object action, String message) {
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 this method out of ParametersInterceptor for reuse

@@ -116,15 +116,17 @@ public void testInsecureParameters() throws Exception {
pi.setParameters(action, vs, HttpParameters.create(params).build());

// then
assertEquals(3, action.getActionMessages().size());
assertEquals(3, action.getActionErrors().size());
Copy link
Member Author

Choose a reason for hiding this comment

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

Using Action errors instead of Action messages to communicate developer errors (only impacts DevMode)

String msg2 = actionErrors.get(1);
String msg3 = actionErrors.get(2);

assertEquals("Unexpected Exception caught setting 'expression' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'expression' with value '#f=#_memberAccess.getClass().getDeclaredField('allowStaticMethodAccess'),#f.setAccessible(true),#f.set(#_memberAccess,true),#req=@org.apache.struts2.ServletActionContext@getRequest(),#resp=@org.apache.struts2.ServletActionContext@getResponse().getWriter(),#resp.println(#req.getRealPath('/')),#resp.close()'", msg1);
Copy link
Member Author

Choose a reason for hiding this comment

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

These messages now include both the context message as well as the exception message. Whilst they are very similar in this test example, it's not guaranteed to be the case

@kusalk kusalk requested a review from lukaszlenart June 18, 2024 09:42
@@ -26,7 +26,7 @@
* ValidationAware classes can accept Action (class level) or field level error messages. Action level messages are kept
* in a Collection. Field level error messages are kept in a Map from String field name to a List of field error msgs.
*
* @author plightbo
* @author plightbo
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the @author tag next time (recommended by ASF)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

/**
* @since 6.5.0
*/
public class DebugUtils {
Copy link
Member

Choose a reason for hiding this comment

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

can this be final or maybe make an interface which can be mixed in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Made it final, not sure it makes sense as a mix-in interface

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
65.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@kusalk kusalk merged commit 898a8d9 into master Jun 21, 2024
7 of 9 checks passed
@kusalk kusalk deleted the WW-5429-param-anno-log branch June 21, 2024 09:47
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