atlesn / rrr

RRR (Read Route Record) is a general purpose acquirement, transmission and processing daemon supporting HTTP, MQTT, TCP, UDP and other I/O devices. RRR has a variety of different modules which can be chained together to retrieve, transmit, modify and save messages.
Other
5 stars 4 forks source link

Add option to extend topic generated by httpserver with HTTP method and path #25

Closed fre-ros closed 6 months ago

fre-ros commented 7 months ago

As discussed here is the proposed change described in the man pages, please give some feedback if I should proceed with implementation.

fre-ros commented 7 months ago

I would also need some guidance on where to extract the path information from the URI. I can see now that the path and the query string is placed inside request_uri_nullsafe

struct rrr_http_part {
        ...
    struct rrr_nullsafe_str *request_uri_nullsafe;
        ...
};

I could try to the extract only the path information from that string but it seems like the wrong thing to do so should we create a new field besides request_uri_nullsafe that only holds the path information and one for the query string? That would lead to changes in __rrr_http_part_query_string_parse in http_part.c correct?

atlesn commented 7 months ago

First of all, could you do the modifications on top of the development branch instead of master? You should be able to just merge development into your branch, at least it works for this one. I'm looking into the "topic" option in more detail now :)

atlesn commented 7 months ago

I tried to specify in more detail how the topic stuff is to work, could you have a look? I'm not sure if the PR will become a complete mess if we merge in development branch on top of it though. Maybe we'll just leave this one on master, I can fix any merge conflicts later.

atlesn commented 7 months ago

I would also need some guidance on where to extract the path information from the URI. I can see now that the path and the query string is placed inside request_uri_nullsafe

This value, despite the name, contains only the endpoint and any query string (?a=2 etc.). When receiving full requests, this value is added to the http_endpoint array value. If we are to use this value for the topic, we have to remove any # and ? stuff from this value or else the MQTT topic could become invalid.

There is some functionality rrr_http_util_uri_endpoint_and_query_string_split to split out the query string on the raft-core branch although it does not remove any # part. We can just make a new rrr_http_util_uri_endpoint_clean which just takes an rrr_nullsafe_str and chops of any # or ? part, giving the result in a callback just like the split function. In practice we only need to pass a shorter length to the callback if the endpoint is chopped.

So i think from p_httpserver.c we would use a new helper function in http_util.c to clean the endpoint. We then make a topic string (simple or request format) before calling httpserver_receive_callback_send_array_message.

fre-ros commented 6 months ago

I tried to specify in more detail how the topic stuff is to work, could you have a look?

Yes much better described than my attempt

I'm not sure if the PR will become a complete mess if we merge in development branch on top of it though. Maybe we'll just leave this one on master, I can fix any merge conflicts later.

I'll make sure any future pull requests I make will originate from the development branch.

I have made a first attempt to implement support for httpserver_topic_format, I followed your advice and implemented a rrr_http_util_uri_endpoint_clean in likeness with rrr_http_util_uri_endpoint_and_query_string_split.

Because the result is passed through callbacks I placed the result in the target_array in the last callback to actually use it when creating the topic. Because of this a new entry called http_endpoint_path will be passed in the message to receivers. I'm not sure if this is the preferred way?

I choose http_endpoint_path because the resulting string after the clean will be the HTTP path.

Please take a look and come with feedback.

atlesn commented 6 months ago

This change was kindof complex due to how the receive function was structured. I've made a rewrite of this so that the new topic format parameter is independent of the receive full request functions.

Processing of the topic otherwise looked good only that we need to check for NULL bytes as HTTP allows this but not MQTT. Also, we don't need the http_endpoint_path value in the resulting messages, this is now only a temporary.

We need to make another commit ensuring that response messages with the new topic format is identified (when request topic format is used, simply add /# to the topic filter when we check).

Please have a look :)

fre-ros commented 6 months ago

We need to make another commit ensuring that response messages with the new topic format is identified (when request topic format is used, simply add /# to the topic filter when we check).

I added a commit for this, please take a look.

atlesn commented 6 months ago

Looks good, I applied some OCD to make the code a little neater. I also updated the manual as we now require the topic to have at least 4 levels when 'request' format is used. Added the new parameter to one of the tests.

Have you used this parameter in practice yet? Does it solve the problem you had?

atlesn commented 6 months ago

If we need to work on this any further, please open a new PR. I've fixed the conflicts with development branch and merged.

fre-ros commented 6 months ago

Have you used this parameter in practice yet? Does it solve the problem you had?

Yes I have tested it for our use case and it works very well, thanks for all the help :)