search

RaiMan / SikuliX1

SikuliX version 2.0.0+ (2019+)
https://sikulix.github.io
MIT License
2.79k stars 357 forks source link

[discussion] SikulixServer commands #130

Open RMizuguchi opened 5 years ago

RMizuguchi commented 5 years ago

Let share ideas and discuss SikulixServer commands !

I will post an idea when I have time :-)

RaiMan commented 5 years ago

@RMizuguchi thanks. nice weekend.

RMizuguchi commented 5 years ago

1. Use Scene I want to use SikulixServer more for RPA. TheUsingIdeaOfSikulixServer_2 *06/02/2019 picutere updated

2. Commands /run/scriptName?args=arg1;arg2;arg3 /run/subFolderName/scriptName?args=arg1;arg2;arg3 Same as current.

/async/scriptName?args=arg1;arg2;arg3 /async/subFolderName/scriptName?args=arg1;arg2;arg3 Run script asynchronized. SikulixServer responds with taskNo when it accepts the request. taskNo is a unique value(For example, it generates at the time of accepted run or async request) for that SikulixServer. taskNo is not persistent. They are cleared when SikulixServer is stoped.

/tasks Return the current uncompleted task list. (It may be good to add a filter to the query)

/tasks/{taskNo} Return the detail of task indicated by taskNo.

/tasks/{taskNo}/cancel Cancel the task indicated by taskNo. SikulixServer deletes the task in the task queue.

/pause Pause script execution. If there is a script running, it will pause after the script has finished processing.

/continue Cancel the pause.

3. Revise commands /start, /startp Move to SikulixServer initialize.

/scripts, /images Move to SikulixServer startup options.

/exit Remove (unnecessary because using Undertow)

/stop Remove (using pause command and terminate SikulixServer from local)

4. HTTP Method Since input commands to the browser address everyone should be easy and simple to use, it might be a good idea to keep run-command's method(GET). On the other hand, I made Control Box so that SikulixServer can be used more easily. So I think it's good to widen the verb range this opportunity.

Thank you.

RMizuguchi commented 5 years ago

One more thing, I want to change the content type of response from text to json.

RaiMan commented 5 years ago

@RMizuguchi

I want to change the content type of response from text to json.

agreed.

RaiMan commented 5 years ago

/asynch/...

I do not understand the difference to /run/

/scripts, /images Move to SikulixServer startup options.

Agreed, should be there also.

But they should also be available for the basic browser address line usage (verb GET) and for other types of clients (wget, curl, ...).

/stop

As a convenience should be available as command.

I still have to look at the ControlBox. I hope during next week (other priorities currently).

RMizuguchi commented 5 years ago

Thank you for your comment.

I do not understand the difference to /run/

/run will wait until the response of the script execution result comes back. On the other hand, /async only receives a reception response, which reduces the waiting time. I thought it would be better for the client(user) to be able to select it. There was also an idea to receive the result of script execution of async on WebSocket.

But they should also be available for the basic browser address line usage (verb GET) and for other types of clients (wget, curl, ...).

As a convenience should be available as command.

I agree. I was concerned about accidental script folder changes or unintended stop, but there should be better to put restrictions on reverse proxy server or authentication/authorization.

I still have to look at the ControlBox. I hope during next week (other priorities currently).

I understood. I look forward to :-)

RaiMan commented 5 years ago

Thanks :-)

/asynch/

I understand: build some job-queue with it. What about giving some startup timing as parameter?

I was concerned about accidental script folder changes or unintended stop ...

Understood and agreed for your (BTW: very nice picture :-)) RPA environment. So when starting a server it should be possible to define some restrictions (... but I think this is for fine tuning later).

RMizuguchi commented 5 years ago

What about giving some startup timing as parameter?

Ohh I see!! Is it like this? /run/scriptName : synchronized script execution /run/scriptName?asynch : asynchronized script execution

It is better not to need to create new commands. And, I changed the syntax of the arguments, so it's easy to add new parameters. It's very good idea!

BTW: very nice picture :-)

Thank you very much :-)

So when starting a server it should be possible to define some restrictions (... but I think this is for fine tuning later).

I agree. /scripts, /images and /stop remain as they are. Let's tune again later.

balmma commented 5 years ago

Sorry for my late response. We had a public holiday here. Today I'm quite a bit busy but as far as I can say there are already some really good ideas in here. Will give my 50 cents tomorrow :-)

balmma commented 5 years ago

Some comments from my side:

IMHO the API should be more REST like, although it will end up in a mix between REST and kind of RPC.

GET /scripts Returns a list of available scripts in the scripts folder e.g. [{"name": "MyScript", "type": "jython" }, ...]. Could be used to provide a select in the ControlBox.

GET /scripts/{name} Returns info about the named script. e.g. {"name": "MyScript", "type": "jython" }.

POST/GET /scripts/{scriptName}/run?async={async}&args={args} Runs the given script either synchronously or asynchronously, depending on the async parameter (true/false). Places the script in the task list and returns the task if async=true.

GET /groups (might also be called /folders, but /groups is more generic) Returns the available sub folders. e.g. [{"name": "public"}, {"name:": "private"}] This can map to the folder structure inside the scripts folder.

GET /groups/{groupName}/scripts Returns scripts of the given group.

POST/GET /groups/{groupName}/scripts/{scriptName}/run?async={async}&args={args} Runs the given script either synchronously or asynchronously, depending on the async parameter (true/false). Places the script in the task list and returns the task if async=true.

GET /tasks Returns the queued / running tasks. e.g. [{id: "5e12a8f9c9e959060fdcaea165393039", status: "waiting", script: MyScript }, {id: "17f6e4074209ea77ed6fc424eff66a40", "status": "running", "script": "Foo"}]

GET /tasks{id} Returns info about the given task e.g. {id: "17f6e4074209ea77ed6fc424eff66a40", "status": "running", "script": "Foo"}

POST/GET /tasks/{id}/cancel Cancels the given task

POST/GET /pause Finishes the currently running script and pauses further script execution. Calls to run after pause should give some 4xx error to indicate that the server is in pause mode. Asynchronous runs can still be placed in the task queue.

POST/GET /resume Resumes script execution.

I agree with getting rid of the /start and /startp requests. And yes, /images and /scripts rather belong to the server startup options.

Security wise as the bare minimum we should be able to define a list of allowed remote IPs (should be localhost only by default). If somebody is using a reverse proxy he can just leave this setting on localhost and implement access restrictions there. And when we map script/group names to file/folder names we should sanitize the names properly to avoid path traversal vulnerabilities.

RMizuguchi commented 5 years ago

Thank you for your comment :-)

I think that your URI is better for REST than so far. That's great! In the next PR, I would like to add these commands anew. (will leave old commands still, and will not yet add start options)

BTW, what should I do if I want to stop the script being executed by /pause? It seems impossible with IScriptRunner#close().

balmma commented 5 years ago

Well, probably the best would be to just let the current run to finish properly but not to accept new runs. Pausing a running script is not possible and cancelling it might leave your Script runner Desktop in an undefined / inconsistent state.

balmma commented 5 years ago

BTW: AbstractScriptRunner#close() also waits until the current run is done (close and runScript both synchronize on the runner instance).

Do we need an additional abort() method in IScriptRunner? Although this seems to be quite tricky with some runner implementations.

RMizuguchi commented 5 years ago

Thank you.

Well, probably the best would be to just let the current run to finish properly but not to accept new runs.

I was thinking the same thing (stopping execution from the next script). When SikulixServer accepts /pause, it changes the state to pause, and the /run handler looks at the state and changes its behavior.

Do we need an additional abort() method in IScriptRunner? Although this seems to be quite tricky with some runner implementations.

Certainly, I sometimes want abort() for emergency stop. ...But I think that implementation in JavaScriptRunner looks difficult.

balmma commented 5 years ago

Unfortunately it's not less difficult for the JythonRunner. See https://bugs.jython.org/issue2530 Most probably it's not much easier for all the other runners. Would have to dig quite a bit deeper into it and implement an individual solution for each runner when applicable. My suggestion is for the moment to let the current script run finishing properly and simply not to accept new calls.

RMizuguchi commented 5 years ago

Unfortunately it's not less difficult for the JythonRunner. See https://bugs.jython.org/issue2530

I thought JythonRunner could be implemented with close() / cleanup(), but I didn't think that would be possible without such a tricky way... I agree with you. Implementing abort() on all runners not be easy.

My suggestion is for the moment to let the current script run finishing properly and simply not to accept new calls.

I will try this way.

Thank you :-)

RaiMan commented 5 years ago

aborting a running script

I think running scripts with the SikulixServer can be compared to running unittests:

  1. tests are run one after the other
  2. each test has a startup and teardown
  3. the complete test series can have a startup and teardown
  4. if something goes wrong, that was not foreseen, the whole process must be killed

Only point 1 must be implemented by the SikulixServer, for the other points the user is responsible either by scripting accordingly and/or with some manual actions (like killing the server).

So we do not need any aborting of scripts as a server feature. On the list I have a feature, to abort/pause/resume a script by some "signal", that is checked internally by SikuliX features like shot, click, type before they are executed. This would allow a script to terminate/pause/resume abnormally in an ordered way even with some callback support.

Until this feature is available stop/pause have to wait until the running script finishes.

If a user thinks, that the running script has to be aborted, then he has to kill the server manually.

RaiMan commented 5 years ago

proposed commands

I principally agree.

Looking at the WebDriver protocol (https://w3c.github.io/webdriver/) I would prefer: (using :item instead of {item})

GET /scripts GET /script/:name GET /script/:name/run GET /script/:name/run?parm1=foo;bar;...

... similar for groups and tasks (examples): GET /group/:name/script/:name/run?.... GET /task/:id

RaiMan commented 5 years ago

run and asynch

I think it is not needed - this would do the same if sent instantly one after the other /script/scr1/run is started /script/scr2/run is queued /script/scr3/run is queued ...

Get information about run/queue state (an :id is not really needed) GET /tasks GET /task/:name

To delete from the queue, if not yet running: /script/scr3/cancel

RaiMan commented 5 years ago

feedback from script runs

GET /task/:name

... will give some information about the run-state of the script (queue position, start time, elapsed time since start, end time, return code, some special log info (to be implemented ;-), ...) either as ugly JSON or some easily readable HTML ;-)

RaiMan commented 5 years ago

GET vs. POST

IMHO we should let GET requests return HTML (if needed like for GET /task/:name) and use POST with JSON for scripted/programmed clients, that want to evaluate the result.

So we have: GET /script/:name/run?parm1=foo;bar;... for the browser returning HTML

and: POST /script/:name/run with a JSON body carrying the "query string" and returning a JSON result

I think this is ok, since both scenarios address different user groups / usage cases.

RaiMan commented 5 years ago

groups vs. folders

I really like the idea:

... GET /groups/{groupName}/scripts would not be needed: either: /group/:name or: /scripts (shows content of current default group)

RaiMan commented 5 years ago

obsolete

start, startp

images (should be done inside a script using the image path features)

RaiMan commented 5 years ago

security

With the group feature the access to scripts is already restricted by the options given at server start.

Runnable scripts have to be prepared by the server admin on the server.

He also has the option, to make the group folders read-only for the server (so scripts cannot create runnable scripts).

Only scripts available in a group tree can be run (/group/grp1/script/subfolder1/subfolder2/scr1/run) A folder spec pointing to outside of the tree is invalid.

Options to change the server environment are not available in the GET commands.

For the POST command we can think about some basic credential or session feature, to eventually allow more options (like e.g. upload or eval scripts or other critical things).

Generally it is up to the user to take care, that runnable scripts do not do bad things.

RMizuguchi commented 5 years ago

Thank you for many comments. I feel that this discussion is shaping up well!!

aborting a running script

Understood and agreed.

proposed commands

I agreed. I think it is even better.

run and asynch

There are two opinions.

feedback from script runs

I agreed.

some special log info,

This is the feature I need! Very good idea ;-)

GET vs. POST

If use different verbs depending on the type of content, should use the HTTP Accept header. If this is the case, we can also get JSON by GET. Default Accept header is HTML and if set JSON in the Accept header, returning a JSON result. How is this?

groups vs. folders

I think that is a good idea. However, I don't really like the need for configuration files.

java -jar sikulix.jar -s c:\sikulix\scripts\public If the argument is a folder, set it as the default group and start server. (only default group)

java -jar sikulix.jar -s c:\sikulix\groups.conf If the argument is a file, load group configuration and start server.

How is this?

obsolete

I agreed. I think that we agreed on removing these.

security

I almost agreed.

we can think about some basic credential or session feature, to eventually allow more options

I understood that this is the next stage of planning.

I think the idea of restricting by IP address is good, what about you?

Thank you.

balmma commented 5 years ago

aborting a running script

This has rather to be seen as a feature of IScriptRunner which SikulixServer can use. I definitely see the benefit of having it. Killing the server instance should always be the last resort. If nobody is against it, I will dig a bit deeper there (have already a quite elegant solution for jython/python). I would prefer a solution that doesn't require to add a check in every SikuliX method.

proposed commands

Probably we should simplify it even further and ditch the async param and /tasks calls altogether. This would mean that all run calls are executed asynchronously and the status of a script is returned by GET /script/:name. e.g. {"name": "foo", "type": "jython", "status": "running", "console": "consoleOutput", errorMessage: "message in case of failure"} or similar. Status can possibly be one of running, waiting, done, error or undefined. This has of course the consequence that we can't queue the same script multiple times but I have a feeling that we really don't need a sophisticated task queue in SikulixServer. If somebody needs such a thing he can implement this outside of SikuliX. The only thing we have to ensure is that there is only one script running at the same time. And ths async queing comes just handy here. BTW: In REST calls the type is always in pluralized form. e.g. /scripts, /scripts/:name, /scripts/:name/run. You can see it as a collection of objects where your can pick one of them.

GET vs. POST

Totally agree. The response content type should be determined by the Accept request header, not by the request method. Default for a REST service should be application/json nowadays. Browsers set the Accept header to text/html anyway. But I ask myself if we really want to implement the HTML representation. Since we have the ControlBox now, nice HTML output will be rarely needed.

groups vs. folders

Why having a configuration option for this? Can't we simply map to folders inside the scripts folder?

I would hesitate to implement the default group feature. This adds an additional state to the service. The proper REST way is to always define the group and the script in the request. e.g. /groups/Public/scripts/Foo/run. This request always runs the Foo script inside the Public group independent of some internal default group state.

security

I would stick to the source IP checking for the time being. And I would also hesitate to add a session feature to a web service.

balmma commented 5 years ago

Oh yes, and I also don't see a real use case for the /pause and /resume calls. Adding /scripts/:name/cancel should be enough. As long as we don't have the abort feature in IScriptRunner it is only possible to cancel queued (not yet running) scripts.

balmma commented 5 years ago

Task Id I think Id is necessary. For example: /script/registUser/run?args=Tom;Sales;

Ok, this is of course a valid use case where it would make sense to have a tasks list.

Better suggestion:

POST /scripts/:name/runs?args=Tom;Sales Triggers a run of the given script. Returns immediately with a run object e.g. {id: "46b5b3234345a345", "status": "running", "output": "console out"}. More properties to add.

GET /scripts/:name/runs Returns the runs for the given script. e.g. [ {id: "46b5b3234345a345", "status": "running", "output": "console out"}, {id: "92f4e64865", "status": "waiting", "queueTotal": 10, "queuePlace": 2}]

GET /scripts/:name/runs/:id Returns the run object with the given id.

Edit: Or probably it would be easier just to have the /tasks calls :-) But the tasks id is necessary anyway. And I somewhat like the decoupling of script definitions and script runs.

balmma commented 5 years ago

(using :item instead of {item})

This is just an implementation detail. Springframework uses {} for path parameters, no idea how request mappings are defined in Undertow. But as far I can say, even on https://w3c.github.io/webdriver/#endpoints they are using {param} in the endpoint definitions. Node Express on the other hand uses the :param notation.

balmma commented 5 years ago

I would stick to the source IP checking for the time being.

Another simple option would be to add a setting for a preshared API key. Probably better applicable to distributed systems.

RaiMan commented 5 years ago

Crawling backwards ;-)

I would stick to the source IP checking for the time being.

Agreed - my idea was only for later if needed.

(using :item instead of {item})

It is only for this discussion: easier to write and read. The implementation might do what ever is needed.

taskID

Agreed - we should have it for those cases where the same script is in the queue or run history with different parameters /script/:name/run should return a taskID

/pause and /resume

should be implemented, since we have a queue feature, that might need that for making the server system useable for "manual" interactions whatever.

groups

I might have a misunderstanding. I understand, that a group represents a folder location or more general a location somewhere in the world, that contains runnable scripts optionally in a subtree. So where and when is this relation group -> folder/location defined? If I understand right, then we

GET vs. POST

... is not what I am saying: I want to have both.

  1. GET with a limited feature set for the browser address line
    • is not really REST-like, since creating a new task in the queue (run a script) should be a POST request
    • might return JSON, which looks ugly when displayed in the browser
    • might be, that the ControlBox is the solution (sorry, not yet seen it)
  2. GET and POST for the API implementation to be used by programmed/scripted clients
    • everything in JSON - no query string needed

asynch

I have to change my opinion again: when saying /script/:name/run I will wait for the completion of the script and the result to be displayed. When adding a script to the queue, I instantly get the taskID and continue. Later I have to poll for state and result. So we need a differentiation like /script/:name/run and /script/:name/task Only the state and result of scriptruns triggered with /script/:name/task can be queried with the task command.

commands - use of plural

The only REST API I had a deeper look into was the mentioned Webdriver API. There they use it so. IMHO /script/:name/run simply looks more natural.

aborting

If we can implement a general abort in the runners, then I would simply clap my hands ;-)

The feature, I was talking about (gracefully abort/pause a script only before a SikuliX feature is started), will be implemented anyways. It is helpful for those people using the SikuliX IDE or more interactive script running from commandline. The idea is to not abort a script in the middle of some mouse, keyboard or screen action. Has not really anything to do, with what we are talking about here.

balmma commented 5 years ago

IMHO /script/:name/run simply looks more natural

Personally, I really like the naming conventions from https://restfulapi.net/resource-naming/ But of course to some degree it's personal taste. I can also recommend the other sections there like https://restfulapi.net/rest-architectural-constraints/ (especially the section about statelessness) BTW: They also use the {} notation for params there :-)

asynch

Your suggestion with the two separate requests is even better.

GET/POST /scripts/:name/run?args=:args Executes the script synchronously and waits for it to finish before returning. Might be the best to create a new task implicitly as this request is also going to be queued in.

POST /scripts/:name/tasks?args=:args Creates a new task which runs the given script and returns a task object e.g. {"id": "a6e8d3456e234", "status": "running",...otherUsefullTaskInfo...}

GET /scripts/:name/tasks Returns the tasks for the script

GET /scripts/:name/tasks/:id Returns the given task.

PUT /scripts/:name/tasks/:id/cancel Cancels the given task

might be, that the ControlBox is the solution (sorry, not yet seen it)

Definitely :-) I'm pretty sure that using the address bar directly won't be needed anymore when the ControlBox fully supports the interface.

groups

Still not sure what's the best way for this one...

RaiMan commented 5 years ago

@balmma Finally agreed with respect to your pointer to https://restfulapi.net. I added it to the intro post.

ControlBox

agreed

groups

I like the feature, since it allows to hide complexity and to restrict the access to ressource locations.

groups startup feature

No changes from the client should be possible (only switching default among existing groups)

balmma commented 5 years ago

change the default group: /groups/:name/default

No, the client should not be able to change the default group on the fly. This is exactly the kind of application state we want to avoid in a REST service (https://restfulapi.net/statelessness). It will only cause confusion, especially if we imagine situations where multiple users use the same SikulixServer.

groups startup feature

I like this simple startup configuration idea.

RaiMan commented 5 years ago

@balmma

change the default group: /groups/:name/default

agreed - not a feature

RMizuguchi commented 5 years ago

I'm sorry, I could not reply. I was tied up today. But, this discussion seems to be shaping up more. That's you guys!!

I will summarize the discussion so far tomorrow :-)

RaiMan commented 5 years ago

@RMizuguchi No problem - take your time.

Thanks for your effort. What about creating a WikiPage now, that sums up the design state?

RMizuguchi commented 5 years ago

@RaiMan Does it mean that we will put what we decided in the discussion together in the Wiki? I think that it is good because it is easy to edit. Would you like me to create the WikiPage tomorrow?

RaiMan commented 5 years ago

@RMizuguchi Exactly so ;-) Please do it. Thanks.

RMizuguchi commented 5 years ago

@RaiMan Yes, sir :-)

RMizuguchi commented 5 years ago

I created the WikiPage. If you find anything that I misunderstood or that can be added to it, please edit it.

RaiMan commented 5 years ago

@RMizuguchi Thanks for the good work. I made a first review.

RMizuguchi commented 5 years ago

@RaiMan Thank you for the review and editing. And thank you for correcting my poor English.

RaiMan commented 5 years ago

@RMizuguchi & @balmma I have pushed the implementation of -g and -x and revised the wiki accordingly. The implicit base folder for SikulixServer is the current working folder, where relative folder and file specs are tried to resolve in. I slightly revised the server logging, to show error (<0), info (0) and debug(>0). Switch on on command line using -d 3

RMizuguchi commented 5 years ago

@RaiMan The speed of your work is amazing! Thanks :-) Since I found a bug, commented to f7ec2a1d7389fb17e2610034a9201a78dcf1b202 about it.

RaiMan commented 5 years ago

Since I found a bug, commented to f7ec2a1 about it.

Yes, thanks for feedback also. Already fixed that, but forgot to push the commit ;-) Should be fixed now.

RMizuguchi commented 5 years ago

I looked it. Thanks :-)

balmma commented 5 years ago

@RMizuguchi Please see PR #142 for abort support to consider for your task queue implementation.

RaiMan commented 5 years ago

@RMizuguchi I rebased the server branch to the current master.

I will also commit the RunTime revision in the server branch to master and rebase again when finished.

RMizuguchi commented 5 years ago

@RaiMan

I rebased the server branch to the current master.

Thank you :-)

I will also commit the RunTime revision in the server branch to master and rebase again when finished.

Ok, understood.