Irsa 729 add ws commands #477

ejoliet · 2017-10-12T18:53:52Z

I’ve added the server side commands. Please have a look and we can discuss it.

I’ve left out the region files and packaging because it totally different path and I have doubt on how to do it, we probably need to in particular.

The ticket has more details on the JSON format and the server parameters expected.

I’ve also added tests, just ask me the password for running them or debug. Remember that the implementation used is defined in the properties and currently we support only authenticated workspace. The commands rely on a util class that uses a workspace manager, that can be switch to other implementation. The vospace protocol is behaving differently so i’m not sure if the classes will need adjustment in the future for different protocol.

loitly

The new map bug need to be fixed. The command comments are suggestions. We can talk about it if you like, or use your best judgement. Code looks good.

loitly · 2017-10-13T15:47:34Z

src/firefly/java/edu/caltech/ipac/firefly/data/ServerParams.java

-    public static final String WS_PUT_FILE = "wsPut";
+    public static final String WS_LIST = "wsList"; // Gets the list of content/files
+    public static final String WS_GET_FILE = "wsGet"; // TODO
+    public static final String WS_UPLOAD_FILE = "wsUpload";// to get the path of the file uploaded into firefly server from WS


I don't think this is needed. We already have an 'upload' command that will handle url and multipart. We can add from workspace in there as well.

We need to decide what what workspace upload even means. I would prefer for out stuff treat workspace file the same way we treat URLs. Not so much with URLDownload but as a file that is retrieved when it is used instread requiring the user to wait while a file is transferred from one server to another. it could come in as workspace://a/b/c.tbl then we have the workspace layer know how to deal with that.

if the request is sent to CmdSrv by using command ServerParams.UPLOAD, current request parameter has either file: or URL: , we may add workspace: for workspace upload.

loitly · 2017-10-13T15:49:33Z

src/firefly/java/edu/caltech/ipac/firefly/data/ServerParams.java

+    public static final String WS_GET_FILE = "wsGet"; // TODO
+    public static final String WS_UPLOAD_FILE = "wsUpload";// to get the path of the file uploaded into firefly server from WS
+    public static final String WS_PUT_IMAGE_FILE = "wsPut"; //FITS, PNG, comes from edu.caltech.ipac.firefly.server.servlets.AnyFileDownload
+    public static final String WS_PUT_TABLE_FILE = "wsPutTable";


Is there a need to distinguish image from table? Why not have WS_PUT_FILE and allow for additional params. I am sure there are other types beside image and table.

yes, a file should be a file.

i feel that will be in the context where we use the WS manager that piece can be moved later to the right place, keeping it like this for now.

loitly · 2017-10-13T15:52:54Z

src/firefly/java/edu/caltech/ipac/firefly/data/ServerParams.java

+    public static final String WS_DELETE_FILE = "wsDel";
+    public static final String WS_MOVE_FILE = "wsMove";
+    public static final String WS_GET_METADATA = "wsGetMeta";
+    public static final String WS_CREATE_PARENT = "wsParent";


Can we call this WS_CREATE_FOLDER? Parent implies that you know the child. Even if the parameter is a path to the child, it's easy to strip the filename out.

ok, i did refactor before merging.

loitly · 2017-10-13T15:58:17Z

src/firefly/java/edu/caltech/ipac/firefly/server/RequestOwner.java

+        if(requestAgent!=null) {
+            Map<String, Cookie> cookies = requestAgent.getCookies();
+            for (Cookie c : cookies.values()) {
+                cmap = new HashMap<>(cookies.size());


This does not look right. new HashMap() should be outside of loop.

loitly · 2017-10-13T16:05:13Z

src/firefly/java/edu/caltech/ipac/firefly/server/WebDAVWorkspaceManager.java

+        MoveMethod move = new MoveMethod(getResourceUrl(originalFileRelPath), newUrl, overwrite);
+        if (!executeMethod(move)) {
+            // handle error
+            LOG.error("Unable to move:" + originalFileRelPath + " -- " + move.getStatusText());


I am glad to see you take the time to log informative errors. This will come handy when debugging in operation. newUrl is not important?

loitly · 2017-10-13T16:29:59Z

src/firefly/java/edu/caltech/ipac/firefly/server/ws/WebDAVWorkspaceHandler.java

@@ -31,10 +31,13 @@ public WorkspaceManager withCredentials(WsCredentials cred) {

        synchronized (cred) {
            String id = cred.getWsId();


cred did not override equals. I doubt if this synchronized does anything. Plus, I don't think a wsPool is necessary. The manager is meant as a way to encapsulate logic. It's should be fine for single use.

I don't think it will work as expected either. However, I would like to know what you are trying to accomplish. What will happen it is is not synchronized? If it is necessary there might be another way to do it.

robyww

This mostly looks good. Seems well designed. I think you are on the right track. My big issue here is the upload/download concepts.

I think it is all confusing. What we want to do is read from the workspace and save to the workspace. While it might sit on the ui in the upload/download place it is different. Particularly reading from workspace. It is more like a server URL retrieve than an upload.

I think the workspace save can sit with the server side download code (AnyFileDownload and one or two others) and work by an extra parameter.

The retrieve is a a little different since it is more like an URL.
We need to look at our code and put a workspace retrieve at the place where get URLs. This is probably just two or three places. I don't think it will be much code.

We probably should meet a discuss it on Monday.

robyww · 2017-10-14T19:00:07Z

src/firefly/java/edu/caltech/ipac/firefly/data/ServerParams.java

-    public static final String WS_PUT_FILE = "wsPut";
+    public static final String WS_LIST = "wsList"; // Gets the list of content/files
+    public static final String WS_GET_FILE = "wsGet"; // TODO
+    public static final String WS_UPLOAD_FILE = "wsUpload";// to get the path of the file uploaded into firefly server from WS


We need to decide what what workspace upload even means. I would prefer for out stuff treat workspace file the same way we treat URLs. Not so much with URLDownload but as a file that is retrieved when it is used instread requiring the user to wait while a file is transferred from one server to another. it could come in as workspace://a/b/c.tbl then we have the workspace layer know how to deal with that.

robyww · 2017-10-14T19:00:50Z

src/firefly/java/edu/caltech/ipac/firefly/data/ServerParams.java

+    public static final String WS_GET_FILE = "wsGet"; // TODO
+    public static final String WS_UPLOAD_FILE = "wsUpload";// to get the path of the file uploaded into firefly server from WS
+    public static final String WS_PUT_IMAGE_FILE = "wsPut"; //FITS, PNG, comes from edu.caltech.ipac.firefly.server.servlets.AnyFileDownload
+    public static final String WS_PUT_TABLE_FILE = "wsPutTable";


yes, a file should be a file.

robyww · 2017-10-14T19:02:07Z

src/firefly/java/edu/caltech/ipac/firefly/data/ServerParams.java

+    public static final String WS_DELETE_FILE = "wsDel";
+    public static final String WS_MOVE_FILE = "wsMove";
+    public static final String WS_GET_METADATA = "wsGetMeta";
+    public static final String WS_CREATE_PARENT = "wsParent";


robyww · 2017-10-14T19:21:05Z

src/firefly/java/edu/caltech/ipac/firefly/server/WebDAVWorkspaceManager.java

-        }
-        return WsUtil.success(m.getStatusCode(), m.getStatusText(), relPath);
-    }
-
    public WspaceMeta getMeta(String relPath, WspaceMeta.Includes includes) {


Is there a way to get a set of props for all the files. i.e. owner?

yes, getMeta("/", ALL). See unit test or I can explain later when we meet again.

robyww · 2017-10-14T19:25:41Z

src/firefly/java/edu/caltech/ipac/firefly/server/ws/WebDAVWorkspaceHandler.java

@@ -31,10 +31,13 @@ public WorkspaceManager withCredentials(WsCredentials cred) {

        synchronized (cred) {
            String id = cred.getWsId();


I don't think it will work as expected either. However, I would like to know what you are trying to accomplish. What will happen it is is not synchronized? If it is necessary there might be another way to do it.

robyww · 2017-10-14T19:27:18Z

src/firefly/java/edu/caltech/ipac/firefly/server/ws/WebDAVWorkspaceHandler.java

                return wsPool.get(id);
-            }
+            }*/
            WorkspaceManager ws = null;//new WebDAVWorkspaceManager(cred.getWsId(), cred.getCookies());
            try {
                Class clazz = Class.forName(managerClass);


I like using reflection here. This should be very flexible.

ejoliet · 2017-10-19T23:41:27Z

As discussed, this code is newly added and is good start for integration step while File picker is being developed. The code in the commands can be easily extracted and used directly in places where it needs: downlaod images/ds9 (in servlet context) and upload part.

The main command to use is available which is getting the list of content. I'll merge this branch and continue later when we tried to integrate the file picker save/upload features.

ejoliet added 2 commits October 11, 2017 20:24

IRSA-729: get credentials from one place

bd5d77d

IRSA-729: added server commands

9bb0466

ejoliet requested review from loitly and cwang2016 October 12, 2017 18:53

robyww assigned ejoliet Oct 12, 2017

ejoliet requested a review from robyww October 12, 2017 22:54

IRSA-729: return boolean string if response is ok or not instead

6a329d1

loitly requested changes Oct 13, 2017

View reviewed changes

robyww requested changes Oct 14, 2017

View reviewed changes

cleanup from reviewers feedbacks.

8804c16

robyww approved these changes Oct 20, 2017

View reviewed changes

ejoliet merged commit a01d5aa into dev Oct 20, 2017

ejoliet deleted the IRSA-729-add-ws-commands branch October 20, 2017 22:56

		@@ -31,10 +31,13 @@ public WorkspaceManager withCredentials(WsCredentials cred) {

		synchronized (cred) {
		String id = cred.getWsId();

Irsa 729 add ws commands #477

Irsa 729 add ws commands #477

Uh oh!

Conversation

ejoliet commented Oct 12, 2017

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robyww Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robyww Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejoliet commented Oct 19, 2017

Uh oh!

Uh oh!

robyww Oct 14, 2017 •

edited

Loading

robyww Oct 14, 2017 •

edited

Loading