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

Bugno34600318 #630

Merged
merged 8 commits into from
Oct 18, 2022
Merged

Bugno34600318 #630

merged 8 commits into from
Oct 18, 2022

Conversation

vavishal
Copy link
Contributor

@vavishal vavishal commented Oct 4, 2022

The faultstring in the SOAPFault contains the exception message if any which is the default current behavior

The customer has a requirement where they do not want any exception messages to be displayed in the faultstring for security concerns of exposing class

Therefore, we are adding a new system property -Dcom.sun.xml.ws.fault.doNotPrintExpMsg=true
to implement the behavior requested by the customer

Added testcase at
./jaxws-ri/runtime/rt/src/test/java/com/sun/xml/ws/fault/SOAPFaultBuilderTest.java

Signed-off-by: Vaibhav Vishal <[email protected]>
Signed-off-by: Vaibhav Vishal <[email protected]>
Signed-off-by: Vaibhav Vishal <[email protected]>
Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

the property needs to follow the pattern used on other places, so it should be sth like fqClassName.propertyName - I'd suggest ie com.sun.xml.ws.fault.SOAPFaultBuilder.captureExceptionMessage

the initialization of the property value should follow the pattern used by the captureStackTrace property (the field should probably be private)

the newly added property also has to be documented:

  • add a note to the jaxws-ri/docs/release-documentation/src/main/docbook/changelog.xml
  • check jaxws-ri/docs/release-documentation/src/main/docbook/jax-ws-ri-extensions.xml to see if it makes sense to document the property there as well

@vavishal
Copy link
Contributor Author

vavishal commented Oct 6, 2022

I have tried to create the private variable as follows in SOAPFaultBuilder.java

private static final boolean captureExceptionMessage = getBooleanSystemProperty();

private static boolean getBooleanSystemProperty() {
final String propertyString = System.getProperty("com.sun.xml.ws.fault.SOAPFaultBuilder.captureExceptionMessage");
if (propertyString == null) {
//default value
return true;
} else {
return Boolean.getBoolean(propertyString);
}
}

But the above change is causing the testcase at SOAPFaultBuilderTest.java to fail always since while executing the ExceptionBeanTest.java it calls the SOAPFaultBuilder which initialized the property captureExceptionMessage with default value and also its value is not getting changed in SOAPFaultBuilderTest.java as captureExceptionMessage is a static variable

@lukasj
Copy link
Member

lukasj commented Oct 6, 2022

would non-final field with package private/public access or having public getter/setter for the field to toggle the value help the test?

@vavishal
Copy link
Contributor Author

vavishal commented Oct 6, 2022

The following piece of code and then calling SOAPFaultBuilder.setCaptureExceptionMessage(); from the testcase helped

private static boolean captureExceptionMessage = getBooleanSystemProperty();

private static boolean getBooleanSystemProperty() {
final String propertyString = System.getProperty("com.sun.xml.ws.fault.SOAPFaultBuilder.captureExceptionMessage");
if (propertyString == null) {
default value
return true;
} else {
return Boolean.getBoolean(propertyString);
}
}
public static boolean getCaptureExceptionMessage() {
return captureExceptionMessage;
}
public static void setCaptureExceptionMessage() {
captureExceptionMessage = getBooleanSystemProperty();
}

@vavishal vavishal requested a review from lukasj October 10, 2022 05:05
@vavishal
Copy link
Contributor Author

Gentle reminder to review this change

return captureExceptionMessage;
}

public static void setCaptureExceptionMessage() {
Copy link
Member

Choose a reason for hiding this comment

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

setter needs an argument, so one can call it and change the value from a test (for example); take a look at the dump and similar fields in HttpAdapter, the way they are being initialized and set and follow that pattern. Also note that if the method is to return boolean value, then its name should not start with get but rather with is, should or something implying yes/no answer

@lukasj lukasj self-requested a review October 18, 2022 07:58
Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@lukasj lukasj merged commit 5d35361 into eclipse-ee4j:master Oct 18, 2022
lukasj pushed a commit to lukasj/metro-jax-ws that referenced this pull request Feb 2, 2023
Signed-off-by: Vaibhav Vishal <[email protected]>
(cherry picked from commit 5d35361)
lukasj pushed a commit to lukasj/metro-jax-ws that referenced this pull request Feb 2, 2023
Signed-off-by: Vaibhav Vishal <[email protected]>
(cherry picked from commit 5d35361)
lukasj pushed a commit that referenced this pull request Feb 2, 2023
Signed-off-by: Vaibhav Vishal <[email protected]>
(cherry picked from commit 5d35361)
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