Deepthi M|Updated to show details of error message only when a global property is enabled,Fixed failing test cases and added new tests for the updated method. #591

deepthi-mantena · 2023-09-21T05:22:06Z

As part of this PR we have added a global property so that enabling the property to true will show the stackTrace details in error response and making it false will show empty string in details of error response
Modified the test class to fix the failing tests after this change and also added relevant tests for the change introduced.
Related Talk Thread:
https://talk.openmrs.org/t/how-to-stop-displaying-stacktrace-and-http-error-reports-generated-by-openmrs-and-display-instead-custom-error-response/35282/3

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/RESTWS-921

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

…l property is enabled,Fixed failing test cases and added new tests for the updated method.

gsluthra · 2023-09-21T05:52:43Z

A lot of formatting / spacing changes in the file. It will spoil the "history" of the file. Can u undo those and re-submit?

deepthi-mantena · 2023-09-21T07:28:48Z

@gsluthra I have updated the PR removing whitespaces please review

gsluthra · 2023-09-21T08:36:33Z

omod/src/main/resources/config.xml

+	<globalProperty>
+		<property>@[email protected]</property>
+		<defaultValue>true</defaultValue>
+		<description>If the value of this setting is "true",then details of stackTrace would be shown in the error response</description>


Add the following in the description: Recommend to keep it as "false" from Security perspective, to avoid leaking implementation details.

@gsluthra sure added it.Please verify

gsluthra · 2023-09-21T08:37:50Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/RestUtil.java

@@ -831,13 +831,18 @@ public static SimpleObject wrapErrorResponse(Exception ex, String reason) {
 		StackTraceElement[] steElements = ex.getStackTrace();
 		if (steElements.length > 0) {
 			StackTraceElement ste = ex.getStackTrace()[0];
+			String stackTraceDetailsenabled_gp = Context.getAdministrationService()
+					.getGlobalProperty(RestConstants.ENABLE_STACK_TRACE_DETAILS_GLOBAL_PROPERTY_NAME);


If this property is NOT defined, what happen? Do you get NULL? If so, you might get a NullPointerException. I think there is an alternative method called getGlobalPropertyValue( name, "defaultValue") which handles the NPE scenario. Please check.

@gsluthra yes modified to have default as false .Now if we give some value to it it would consider that else it would always have a default as false to avoid NPE.Please verify

…fault value to global property as false when no value is given to it to avoid NPE

dkayiwa · 2023-09-23T19:53:41Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/RestUtil.java

@@ -831,13 +831,18 @@ public static SimpleObject wrapErrorResponse(Exception ex, String reason) {
 		StackTraceElement[] steElements = ex.getStackTrace();
 		if (steElements.length > 0) {
 			StackTraceElement ste = ex.getStackTrace()[0];
+			String stackTraceDetailsenabled_gp = Context.getAdministrationService()


Can you take a look at our variable naming conventions? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-NamingConventions

dkayiwa · 2023-09-23T19:54:14Z

omod-common/src/main/java/org/openmrs/module/webservices/rest/web/RestConstants.java

@@ -206,4 +206,8 @@ public class RestConstants {
 	 * Constants used for the Server Log REST Service privilege checking
 	 */
 	public static final String PRIV_GET_SERVER_LOGS = "Get Server Logs";
+	/**
+	 * Constants used for the StackTrace Details in error response


Constants or Constant?

I would also be more specific when describing the use for stack trace details? How does this constant affect the stack trace details? Is it this constant which contains the stack trace details in the error response?

dkayiwa · 2023-09-23T19:57:55Z

omod/src/main/resources/config.xml

@@ -82,6 +82,11 @@
 		<defaultValue>true</defaultValue>
 		<description>If the value of this setting is "true", then nothing is logged while the Swagger specification is being generated.</description>
 	</globalProperty>
+	<globalProperty>
+		<property>@[email protected]</property>
+		<defaultValue>true</defaultValue>


We usually put space after commas.

dkayiwa · 2023-09-23T19:58:26Z

omod-common/src/test/java/org/openmrs/module/webservices/rest/web/RestUtilTest.java

+		SimpleObject returnObject = RestUtil.wrapErrorResponse(ex, "wraperrorresponsemessage");
+
+		LinkedHashMap errorResponseMap = (LinkedHashMap) returnObject.get("error");
+		Assert.assertNotEquals("",errorResponseMap.get("detail"));


We usually put space after commas.

dkayiwa · 2023-09-23T19:58:36Z

omod-common/src/test/java/org/openmrs/module/webservices/rest/web/RestUtilTest.java

+		SimpleObject returnObject = RestUtil.wrapErrorResponse(ex, "wraperrorresponsemessage");
+
+		LinkedHashMap errorResponseMap = (LinkedHashMap) returnObject.get("error");
+		Assert.assertEquals("",errorResponseMap.get("detail"));


We usually put space after commas.

dkayiwa · 2023-09-23T20:01:42Z

omod/src/main/resources/config.xml

+	<globalProperty>
+		<property>@[email protected]</property>
+		<defaultValue>true</defaultValue>
+		<description>If the value of this setting is "true",then details of stackTrace would be shown in the error response,However recommendation is to keep it as "false" from Security perspective, to avoid leaking implementation details.</description>


If the value of this setting is "true", then the details of the stackTrace would be shown in the error response. However, the recommendation is to keep it as "false", from the Security perspective, to avoid leaking implementation details.

@dkayiwa Updated as per the review comments can you please review now

dkayiwa · 2023-09-25T09:46:50Z

According to this https://wiki.openmrs.org/display/docs/Pull+Request+Tips, we also recommend including the ticket id in the commit message

…ables,updated description in config.xml and added spaces after comma.

deepthi-mantena · 2023-09-25T10:00:33Z

@dkayiwa sure I have included ticket number in commit message now.Please review

dkayiwa · 2023-09-25T10:01:08Z

Do you mind also fixing the JIRA ticket link in the pull request description?

deepthi-mantena · 2023-09-25T10:05:15Z

Sure Updated the PR description with the ticket link.

Deepthi M|Updated to show details of error message only when a globa…

00066f2

…l property is enabled,Fixed failing test cases and added new tests for the updated method.

deepthi-mantena added 3 commits September 21, 2023 12:15

Deepthi M|Fixed global property

eea5ab5

Deepthi M|removing white spaces

4c89db7

Merge branch 'Trello-332' into removewhite_spaces

401ea75

gsluthra reviewed Sep 21, 2023

View reviewed changes

Deepthi M|Updated description of global property and also added a de…

ef7ff26

…fault value to global property as false when no value is given to it to avoid NPE

deepthi-mantena requested a review from gsluthra September 21, 2023 12:19

dkayiwa reviewed Sep 23, 2023

View reviewed changes

Deepthi M|RESTWS-921|Updated as per the review comments-Renamed vari…

41ba9ba

…ables,updated description in config.xml and added spaces after comma.

deepthi-mantena force-pushed the Trello-332 branch from 35dd26c to 41ba9ba Compare September 25, 2023 09:58

dkayiwa merged commit 7ee4c56 into openmrs:master Sep 25, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deepthi M|Updated to show details of error message only when a global property is enabled,Fixed failing test cases and added new tests for the updated method. #591

Deepthi M|Updated to show details of error message only when a global property is enabled,Fixed failing test cases and added new tests for the updated method. #591

deepthi-mantena commented Sep 21, 2023 •

edited

Loading

gsluthra commented Sep 21, 2023

deepthi-mantena commented Sep 21, 2023

gsluthra Sep 21, 2023

deepthi-mantena Sep 21, 2023

gsluthra Sep 21, 2023

deepthi-mantena Sep 21, 2023

dkayiwa Sep 23, 2023

dkayiwa Sep 23, 2023

dkayiwa Sep 23, 2023 •

edited

Loading

dkayiwa Sep 23, 2023

dkayiwa Sep 23, 2023

dkayiwa Sep 23, 2023

dkayiwa Sep 23, 2023

deepthi-mantena Sep 25, 2023

dkayiwa commented Sep 25, 2023

deepthi-mantena commented Sep 25, 2023

dkayiwa commented Sep 25, 2023

deepthi-mantena commented Sep 25, 2023

Deepthi M|Updated to show details of error message only when a global property is enabled,Fixed failing test cases and added new tests for the updated method. #591

Deepthi M|Updated to show details of error message only when a global property is enabled,Fixed failing test cases and added new tests for the updated method. #591

Conversation

deepthi-mantena commented Sep 21, 2023 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

gsluthra commented Sep 21, 2023

deepthi-mantena commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Sep 25, 2023

deepthi-mantena commented Sep 25, 2023

dkayiwa commented Sep 25, 2023

deepthi-mantena commented Sep 25, 2023

deepthi-mantena commented Sep 21, 2023 •

edited

Loading

dkayiwa Sep 23, 2023 •

edited

Loading