Resolve "Provide japi handler that lists registered commands"

[fraunhofer-iis-bot]

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

Closes #27

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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

marked this merge request as ready

[fraunhofer-iis-bot]

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

requested review from @Michael-M-Baron

[fraunhofer-iis-bot]

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

changed target branch from master to dev

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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"
        ]
    }

[fraunhofer-iis-bot]

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?

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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

approved this merge request

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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

resolved all threads

[fraunhofer-iis-bot]

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

enabled an automatic merge when the pipeline for 5da958d4b73e55f1f1fc1a5cf2d304e3b15d4eeb succeeds

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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

marked this merge request as draft

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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?

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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

aborted the automatic merge because source branch was updated

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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

added 1 commit

• 2709e1f7 - Register default handler

Compare with previous version

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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

resolved all threads

[fraunhofer-iis-bot]

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

added 1 commit

• 28ca04aa - fixup?

Compare with previous version

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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 8b4420d

[fraunhofer-iis-bot]

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.

[fraunhofer-iis-bot]

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

[fraunhofer-iis-bot]

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

resolved all threads

[fraunhofer-iis-bot]

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

added 7 commits

• 1ca1e19d...d311e276 - 2 commits from branch dev
• 1e50c18 - Add and register default function japi_cmd_list
• 09509fd - Disallow user commands from starting with "japi_"
• 6297846 - Register default handler
• 32eaeb0 - fixup?
• 7f15c66 - Revert checking for empty push service context in destroy as tests fail

Compare with previous version

[fraunhofer-iis-bot]

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

marked this merge request as ready

[fraunhofer-iis-bot]

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

added 4 commits

• 3eb513f - Add and register default function japi_cmd_list
• f147abb - Disallow user commands from starting with "japi_"
• 905a965 - Register default handler
• 79790e7 - Revert checking for empty push service context in destroy as tests fail

Compare with previous version