Resolve "Provide japi handler that lists registered commands" #116

Closed fraunhofer-iis-bot closed 1 year ago

In GitLab by @vornkat-iis on Jul 4, 2023, 15:16

Closes #27

In GitLab by @vornkat-iis on Jul 4, 2023, 17:36

added 1 commit

3b5b11ed - Add and register default function japi_cmd_list

Compare with previous version

In GitLab by @vornkat-iis on Jul 4, 2023, 17:38

marked this merge request as ready

In GitLab by @vornkat-iis on Jul 4, 2023, 17:38

requested review from @Michael-M-Baron

In GitLab by @vornkat-iis on Jul 4, 2023, 17:42

changed target branch from master to dev

In GitLab by @Michael-M-Baron on Jul 5, 2023, 11:30

Looks good so far. One thing I am thinking about. In the Interstellar-Project we are returning success and failure after a JAPI-Request.

If the japi request was successfull, we return something like.

json_object_object_add(response, "JAPI_RESPONSE", json_object_new_string("success"));

On Failure.

json_object_object_add(response, "JAPI_RESPONSE", json_object_new_string("failure"));

This is specific to the Interstellar-Project. We should discuss, if this could/should be generalized. I am not sure right now, but I think the Interstellar-Frontend expects the JAPI_RESPONSE key, what means that the new command "japi_cmd_list" cannot be used.

In GitLab by @Michael-M-Baron on Jul 5, 2023, 11:32

added 10 commits

3b5b11ed...0687def5 - 9 commits from branch dev
d97bcf5e - Merge branch 'dev' of git01.iis.fhg.de:ks-ip-lib/software/libjapi into...

Compare with previous version

In GitLab by @vornkat-iis on Jul 7, 2023, 11:42

For push service subscribe and unsubscribe, there is a field "success" with the value true or false, see libjapi-demo/README.md

We should really discuss which way to go but for the list commands, failure would mean no response to the client as an empty list is a valid result and there are at least three commands registered anyways:

    "japi_response": "japi_cmd_list",
    "data": {
        "commands": [
            "request_not_found_handler",
            "japi_cmd_list",
            "japi_pushsrv_list"
        ]
    }

In GitLab by @vornkat-iis on Jul 7, 2023, 11:59

@jannismain What would you expect as result? There are several default commands, which are currently listed only partially:

"japi_response": "japi_cmd_list",
    "data": {
        "commands": [
            "request_not_found_handler",
            "japi_cmd_list",
            "japi_pushsrv_list"
        ]
    }

missing: japi_pushsrv_subscribe and ...unsubscribe. I would prefer having them instead of the request_not_found_handler. Or do we want to list only user commands?

In GitLab by @Michael-M-Baron on Jul 7, 2023, 13:30

added 3 commits

d97bcf5e...e242bb66 - 2 commits from branch dev
5da958d4 - Add and register default function japi_cmd_list

Compare with previous version

In GitLab by @Michael-M-Baron on Jul 7, 2023, 13:30

approved this merge request

In GitLab by @Michael-M-Baron on Jul 7, 2023, 13:32

Ok I would sugggest to keep the software as it is for the moment and merge this branch and open a new issue/feature to discuss this topic.

In GitLab by @Michael-M-Baron on Jul 7, 2023, 13:32

resolved all threads

In GitLab by @Michael-M-Baron on Jul 7, 2023, 13:32

enabled an automatic merge when the pipeline for 5da958d4b73e55f1f1fc1a5cf2d304e3b15d4eeb succeeds

In GitLab by @jannismain on Jul 7, 2023, 13:32

I agree, request_not_found_handler should not be part of that list and _subscribe/_unsubscribe should be added.

Otherwise, I like the fact that all system commands (i.e. those provided by libjapi) are prefixed with japi. So user commands can be identified by the absence of that prefix. Additionally, we should disallow registering user commands with the japi_ prefix in order to enforce this distinction.

All commands useful to the user should be returned. IMO that includes everything other than the request_not_found_handler.

In GitLab by @jannismain on Jul 7, 2023, 13:36

marked this merge request as draft

In GitLab by @jannismain on Jul 7, 2023, 13:38

@Michael-M-Baron there are still commands missing from the return list. I would wait with merging until japi_pushsrv_subscribe and _unsubscribe are also returned.

As an aside, I noticed that the MR was about to be merged with open discussions. I think it is sensible to only allow MRs to be merged when all discussions are resolved. Could you change the repository configuration accordingly? For now, I marked this MR as draft again to halt the merge for now. If you think it should still be merged, go ahead.

In GitLab by @vornkat-iis on Jul 7, 2023, 15:16

I just found out: The user has to provide and register the request_not_found_handler. Therefore I would show it in the list if it exists. Should we provide a fallback to the fallback?

In GitLab by @jannismain on Jul 7, 2023, 17:47

What happens if it is not registered by the user and a non-existing command is requested? I think there should be a sensible default, if the alternative is crashing.

In GitLab by @vornkat-iis on Jul 8, 2023, 14:48

aborted the automatic merge because source branch was updated

In GitLab by @vornkat-iis on Jul 8, 2023, 14:48

added 1 commit

f73d1153 - Register subscribe commands just as every other command

Compare with previous version

In GitLab by @vornkat-iis on Jul 11, 2023, 10:33

added 1 commit

65a7e4f8 - Disallow user commands from starting with "japi_"

Compare with previous version

In GitLab by @vornkat-iis on Jul 11, 2023, 10:39

added 2 commits

df8ac77c - Add and register default function japi_cmd_list
ff415393 - Disallow user commands from starting with "japi_"

Compare with previous version

In GitLab by @vornkat-iis on Jul 11, 2023, 14:14

added 1 commit

2709e1f7 - Register default handler

Compare with previous version

In GitLab by @vornkat-iis on Jul 11, 2023, 14:16

I added a japi_request_not_found_handler and docs to explain the user how to overwrite that behavior.

In GitLab by @vornkat-iis on Jul 11, 2023, 14:16

resolved all threads

In GitLab by @vornkat-iis on Jul 11, 2023, 14:57

added 1 commit

28ca04aa - fixup?

Compare with previous version

In GitLab by @jannismain on Jul 11, 2023, 15:53

For me, the tests are not running locally (even on dev or another branch):

ERROR: Thread not running.
ERROR: Thread not running.
ERROR: push service context is NULL
make[3]: *** [CMakeFiles/run_test] Segmentation fault: 11
make[2]: *** [CMakeFiles/run_test.dir/all] Error 2
make[1]: *** [CMakeFiles/run_test.dir/rule] Error 2
make: *** [run_test] Error 2

In GitLab by @jannismain on Jul 11, 2023, 16:04

On master, tests are running fine for me.

When I git bisect to find out, which commit lead to the tests failing on my machine, the result is 8b4420db609c7f25fe9b96637cede7b0b4397747

In GitLab by @jannismain on Jul 11, 2023, 16:07

The failing test is JAPI_Push_Service.PushServiceDestroy. The commit referenced above introduced that feature.

In GitLab by @vornkat-iis on Jul 11, 2023, 17:39

added 1 commit

1ca1e19d - Revert checking for empty push service context in destroy as tests fail

Compare with previous version

In GitLab by @jannismain on Jul 11, 2023, 17:45

resolved all threads

In GitLab by @vornkat-iis on Jul 11, 2023, 17:52

added 7 commits

1ca1e19d...d311e276 - 2 commits from branch dev
1e50c18a - Add and register default function japi_cmd_list
09509fd1 - Disallow user commands from starting with "japi_"
62978461 - Register default handler
32eaeb0b - fixup?
7f15c667 - Revert checking for empty push service context in destroy as tests fail

Compare with previous version

In GitLab by @vornkat-iis on Jul 11, 2023, 17:52

marked this merge request as ready

In GitLab by @vornkat-iis on Jul 11, 2023, 17:55

added 4 commits

3eb513f4 - Add and register default function japi_cmd_list
f147abba - Disallow user commands from starting with "japi_"
905a9658 - Register default handler
79790e7d - Revert checking for empty push service context in destroy as tests fail

Compare with previous version

Fraunhofer-IIS / libjapi

Resolve "Provide japi handler that lists registered commands" #116