RESTWS-701 Added REST Implementation for Server logs #329

suthagar23 · 2018-03-03T18:14:48Z

It will be helped to expose the Server logs through the REST Webservices. There are some minor logics included using the Wrapper classes to increase the usage of the log lines.

ServerLogResource1_8 - REST Implementation for Server Logs
ServerLogResource1_8Test - Test cases for the Server Logs REST Implementations
ServerLogActionWrapper - Minor Pattern Matching logic included
MockServerLogActionWrapper - Used for mocking while executing the test cases.

The user needs the "Get Server Logs" privilege to access this endpoint.

Issue: RESTWS-701

suthagar23 · 2018-03-03T18:24:39Z

@dkayiwa Could you please take a look at here?

djazayeri · 2018-03-05T17:29:59Z

I think it's important that we add a privilege check here. (Or, maybe openmrs-core already enforces that?)

suthagar23 · 2018-03-07T03:24:18Z

Yes, we need to add privilege check here. Let me add it here. Thanks @djazayeri

suthagar23 · 2018-03-08T06:32:53Z

@djazayeri I have added the privilege check here, can you please take a look at here?

dkayiwa · 2018-03-13T20:20:24Z

Did you take a look at the above travis failure?

suthagar23 · 2018-03-14T10:12:01Z

@dkayiwa

Yah, I have noticed that one. But the error does not relate to this change. Every PR for this repo has the Travis failure due to this issue.
Non-resolvable parent POM for org.openmrs.module:webservices.rest-omod-2.2:[unknown-version]: Could not find artifact org.openmrs.module:webservices.rest:pom:2.22.0-SNAPSHOT in sonatype-snapshots

suthagar23 · 2018-03-18T11:57:01Z

Thanks, @sumangala028 for pointing out the new changes in the POM file to resolve the issue mentioned above.

@dkayiwa Can you please take a look at here for the test errors.

dkayiwa · 2018-03-18T12:20:09Z

Do the pass locally?

coveralls · 2018-03-18T13:01:01Z

Coverage decreased (-0.09%) to 41.562% when pulling 616d242 on suthagar23:RESTWS-701 into d76b294 on openmrs:master.

suthagar23 · 2018-03-18T13:06:45Z

@dkayiwa, I have fixed the issues. Can you please take a look again

dkayiwa · 2018-03-19T10:22:56Z

Does this appear in the online swagger docs?

suthagar23 · 2018-03-19T17:09:02Z

Sorry, I didn't get you properly. What do you mean by "online swagger docs"?

dkayiwa · 2018-03-19T17:12:44Z

@gayanW do you have a minute to respond to the above? 😊

suthagar23 · 2018-03-19T17:22:34Z

I think I need to add getGETModel() to get this into Online Swagger docs.
Am I correct @dkayiwa and @gayanW ?

gayanW · 2018-03-21T06:54:56Z

I think I need to add getGETModel() to get this into Online Swagger docs.
Am I correct @dkayiwa and @gayanW ?

Yeah you'd need implement getGETModel(). Once done you check if it correctly appears in the swagger documentation in: http://localhost:8080/openmrs/module/webservices/rest/apiDocs.htm

However your resource class does not seem extend any of the base resource classes.

FYI: methods such as getGETModel, getCreatableProperties comes from the DelegatingResourceHandler interface.

You can find more information here:
https://wiki.openmrs.org/display/docs/Documenting+REST+Resources

suthagar23 · 2018-03-21T10:23:01Z

I have implemented this class from Listable Interface, not with DelegatingResourceHandler Interface. Listable Interface doesn't have any methods like getGETModel, getCreatableProperties for the swagger access. So how can I override those methods here?

gayanW · 2018-03-21T12:30:38Z

To my understanding most resources extends one of the base resource classes which inherit DelegatingResourceHandler.

DelegatingResourceHandler is the one that is currently being used to describe resource representations.

@dkayiwa will have an idea on what to do in this case :)

suthagar23 · 2018-03-23T20:05:33Z

@dkayiwa Finally I have modified the code to get this in the online swagger docs 😄
I have attached the screenshot from the swagger docs here,

and Thanks for the comments @gayanW

suthagar23 · 2018-04-09T17:24:54Z

@dkayiwa If you have time, Could you please review it and help me to complete this work to release the SysAdmin 1.1 OWA :-)

dkayiwa · 2018-04-12T22:29:51Z

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

+	/**
+	 * Constants used for the Server Log REST Service privilege checking
+	 */
+	public static final String GET_SERVER_LOGS = "Get Server Logs";


Shouldn't you prefix this with PRIV_ to make it clearer from the name?

I have updated the variable name. Thanks @dkayiwa

dkayiwa · 2018-04-12T22:30:48Z

omod-common/src/main/java/org/openmrs/module/webservices/helper/ServerLogActionWrapper.java

+			return logElements;
+		}
+		catch (PatternSyntaxException e) {
+			// In case of Exception, It will return empty array


Then how will the client differentiate between finding no match and errors occurred?

dkayiwa · 2018-04-12T22:50:22Z

@suthagar23 did you compile, install, and test to see if your resource shows up in the online swagger docs?

suthagar23 · 2018-04-13T16:16:42Z

Yes @dkayiwa, I can see those resources in the online swagger docs. You can see it below,

dkayiwa · 2018-04-14T20:26:36Z

Do your resources show up in the online swagger docs when running on the latest snapshot version of openmrs-core?

suthagar23 · 2018-04-16T06:57:35Z

I have just checked the latest snapshot version of openmrs-core (openmrs-2.2.0-SNAPSHOT). But I couldn't see the swagger docs 😞 .
But I can see it in the openmrs-2.0.6-SNAPSHOT version.
What is the issue here? Could you please help me?

dkayiwa · 2018-04-16T07:52:56Z

Can you raise it on talk?

suthagar23 · 2018-04-16T17:08:30Z

Sure @dkayiwa I will post it to talk.

wluyima · 2018-04-16T18:59:12Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ServerLogResource1_8.java

+	@Override
+	public SimpleObject getAll(RequestContext context) throws ResponseException {
+		SimpleObject rest = new SimpleObject();
+		rest.put("serverLog", serverLogActionWrapper.getServerLogs());


Do you need to put the logs in single field? I'd Imagine that you just need to a return a list of server logs

I thought to get that list of logs from "serverlog" field. Anyway, returning that list directly to the resource also a good point. Thanks @wluyima

Since the server logs will return nearly 100 lines, I kept those inside a single key to reduce the complexity for now. We can add some other keys based later as well.

wluyima · 2018-04-16T19:00:16Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ServerLogResource1_8.java

+	@Override
+	public DelegatingResourceDescription getRepresentationDescription(Representation rep) {
+		DelegatingResourceDescription description = new DelegatingResourceDescription();
+		description.addProperty("serverLog", "serverLog");


The properties should be the individual fields on the ServerLogWrapper class

I have added this property to the ServerLogActionWrapper class here - https://github.com/openmrs/openmrs-module-webservices.rest/pull/329/files#diff-410f1f6f7c035020e68aa6abe419b84bR28

Do we need another ServerLogWrapper Class also?

wluyima · 2018-04-16T19:06:38Z

...a/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/ServerLogResource1_8.java

+	}
+
+	@Override
+	public String getUri(Object instance) {


Do you need to override this?

wluyima · 2018-04-16T19:07:53Z

omod-1.8/src/test/java/org/openmrs/module/webservices/rest/web/MockServerLogActionWrapper.java

+import java.util.ArrayList;
+import java.util.List;
+
+/***


Did you format your code?

Sorry for the mistake, I will make it better

dkayiwa · 2018-04-20T10:27:28Z

@suthagar23 did you eventually figure this out?
cc @gayanW

suthagar23 · 2018-04-22T05:03:56Z

Not yet @dkayiwa. I am working on this to figure out the actual problem and looking for the changes in the new release which is blocking this resource in the swagger documentation.

gayanW · 2018-04-22T05:22:15Z

Swagger doc loads fine in the qa-refapp. I didn't have chance to test it locally.

dkayiwa · 2018-04-30T09:59:48Z

@suthagar23 are you doing any more changes on this?

suthagar23 · 2018-05-02T18:34:29Z

@dkayiwa Let me check again the issue here.

dkayiwa · 2018-05-10T13:59:45Z

@suthagar23 did you address Wyclif's comments?

dkayiwa · 2018-05-21T13:29:13Z

@suthagar23 did you see the above?

dkayiwa · 2018-05-27T20:01:52Z

@suthagar23 can you finish this up?

suthagar23 · 2018-05-28T13:30:57Z

@dkayiwa I have updated the PR now. Extremely sorry for the delay since I forgot this work for a long time 😢

…d point. Added REST Implementation with Privilege check for the Server logs end point. Added REST Implementation with Privilege check for the Server logs end point. Added REST Implementation with Privilege check for the Server logs end point. Added REST Implementation with Privilege check for the Server logs end point. Added REST Implementation with Privilege check for the Server logs end point. Added REST Implementation with Privilege check for the Server logs end point. Added REST Implementation with Privilege check for Server Logs end point. Added REST Implementation for Server logs Privilege check added for the REST end point updated the privilege check Comments updated Changed support class for the resource Minor fix in REST Constants Added more improvements to get it in the Swagger Docs Added some testcases to serverLogController Added some unit tests to increase the coverage Added some unit tests Fix some minor issues Fixed minor formatting issues Fixed minor issues Added some tests Added some tests Added some tests Added some tests for serverLog resource Added some tests for serverLog resource Added some tests for serverLog resource

suthagar23 force-pushed the RESTWS-701 branch from 1a61344 to 3305f11 Compare March 8, 2018 06:31

suthagar23 force-pushed the RESTWS-701 branch from 3305f11 to 8e21228 Compare March 13, 2018 18:34

suthagar23 force-pushed the RESTWS-701 branch from 8e21228 to ff4ba6d Compare March 18, 2018 11:25

suthagar23 force-pushed the RESTWS-701 branch 2 times, most recently from 62fc4e6 to 94805ff Compare March 18, 2018 12:52

suthagar23 force-pushed the RESTWS-701 branch 3 times, most recently from f3135bc to a67ee55 Compare March 23, 2018 19:48

dkayiwa reviewed Apr 12, 2018

View reviewed changes

suthagar23 force-pushed the RESTWS-701 branch from a67ee55 to aeab1e5 Compare April 13, 2018 16:01

wluyima reviewed Apr 16, 2018

View reviewed changes

suthagar23 force-pushed the RESTWS-701 branch 2 times, most recently from 00f888a to 6db0cb8 Compare May 28, 2018 13:25

suthagar23 force-pushed the RESTWS-701 branch 4 times, most recently from 6be7252 to 6783a01 Compare May 29, 2018 06:45

suthagar23 force-pushed the RESTWS-701 branch from 6783a01 to 616d242 Compare May 29, 2018 07:00

dkayiwa merged commit c5f1730 into openmrs:master May 29, 2018

RESTWS-701 Added REST Implementation for Server logs #329

RESTWS-701 Added REST Implementation for Server logs #329

Conversation

suthagar23 commented Mar 3, 2018 • edited Loading

suthagar23 commented Mar 3, 2018

djazayeri commented Mar 5, 2018

suthagar23 commented Mar 7, 2018

suthagar23 commented Mar 8, 2018

dkayiwa commented Mar 13, 2018

suthagar23 commented Mar 14, 2018

suthagar23 commented Mar 18, 2018

dkayiwa commented Mar 18, 2018

coveralls commented Mar 18, 2018 • edited Loading

suthagar23 commented Mar 18, 2018

dkayiwa commented Mar 19, 2018

suthagar23 commented Mar 19, 2018

dkayiwa commented Mar 19, 2018

suthagar23 commented Mar 19, 2018

gayanW commented Mar 21, 2018 • edited Loading

suthagar23 commented Mar 21, 2018

gayanW commented Mar 21, 2018 • edited Loading

suthagar23 commented Mar 23, 2018

suthagar23 commented Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Apr 12, 2018

suthagar23 commented Apr 13, 2018

dkayiwa commented Apr 14, 2018

suthagar23 commented Apr 16, 2018

dkayiwa commented Apr 16, 2018

suthagar23 commented Apr 16, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Apr 20, 2018

suthagar23 commented Apr 22, 2018

gayanW commented Apr 22, 2018

dkayiwa commented Apr 30, 2018

suthagar23 commented May 2, 2018

dkayiwa commented May 10, 2018

dkayiwa commented May 21, 2018

dkayiwa commented May 27, 2018

suthagar23 commented May 28, 2018

suthagar23 commented Mar 3, 2018 •

edited

Loading

coveralls commented Mar 18, 2018 •

edited

Loading

gayanW commented Mar 21, 2018 •

edited

Loading

gayanW commented Mar 21, 2018 •

edited

Loading

suthagar23 commented Apr 9, 2018 •

edited

Loading