Support for multipart requests, new ways of routing the request to MP graph #3250

dkalinowski · 2025-04-18T14:31:10Z

🛠 Summary

CVS-163721

If request content-type is application-json - route based on model JSON field
If request content-type s multipart - route based on mode multipart field
In other cases try to deduce graph name from URI: /v3/GRAPH-NAME

multipart field/file access for calculators
unit tests

🧪 Checklist

Unit tests added.
Change follows security best practices.

mzegla · 2025-04-23T10:26:57Z

src/BUILD

@@ -810,6 +816,7 @@ cc_library(
    deps = [
        "@com_github_tencent_rapidjson//:rapidjson",
        "libovmsclient_connection",
+        "libovmsmulti_part_parser",


Why split multi and part? How about naming it libovmsmultipart_parser
I can see such split is made in other places as well

good idea, also removing ovms from name per @atobiszei request

mzegla · 2025-04-23T10:30:59Z

src/http_frontend/multi_part_parser_drogon_impl.hpp

@@ -0,0 +1,49 @@
+//*****************************************************************************
+// Copyright 2024 Intel Corporation


Suggested change

// Copyright 2024 Intel Corporation

// Copyright 2025 Intel Corporation

mzegla · 2025-04-23T10:35:30Z

src/http_frontend/multi_part_parser_drogon_impl.cpp

+    return this->parser->getParameter<std::string>(name);
+}
+
+std::string_view DrogonMultiPartParser::getFileContentByName(const std::string& name) const {


Does it work based on extension? I wonder how much can we trust the name in establishing the content type.
Yet I'm not sure if we should dive into this here since we use third party parser.

it is not file name, but field name. it is not clear from method name, will change that to getFileContentByFieldName

mzegla · 2025-04-23T10:46:18Z

src/http_rest_api_handler.cpp

+        if (isMultiPart) {
+            OVMS_PROFILE_SCOPE("multipart parse");
+            if (!multiPartParser->parse()) {
+                return StatusCode::REST_INVALID_URL;


add logger?

there is no specific logger for http component, however, I will add specific status codes

mzegla · 2025-04-23T10:50:27Z

src/http_rest_api_handler.cpp

+                SPDLOG_DEBUG("Model name from deduced from MultiPart field: {}", model_name);
+            }
+            // Set json parser in invalid state in order to get HasParseError to respond with true
+            doc->Parse("error");


Is it possible to rename doc to jsonParser? What do you think about such idea?

it is already named parsedJson in HttpPayload class, will name it similarly here

mzegla · 2025-04-23T11:00:31Z

src/http_rest_api_handler.cpp

+            if (uri.size() <= 4) {  // nothing after "/v3/..."
+                return StatusCode::REST_INVALID_URL;
+            }
+            model_name = std::string(uri.substr(4));


So we assume model name comes right after /v3/.
Don't we want to nest it a bit like /v3/models/<model_name> or something like that?

I don't think its good idea, with this we wont be able to support OpenAI image variation API:
https://platform.openai.com/docs/api-reference/images/createVariation

mzegla · 2025-04-23T11:03:23Z

src/http_rest_api_handler.cpp

@@ -838,11 +879,11 @@ Status HttpRestApiHandler::processRequest(

    headers->clear();
    response->clear();
-    headers->push_back({"Content-Type", "application/json"});
+    (*headers)["content-type"] = "application/json";


Should this line still be here since we can now support other content types and here we do hard assignment.

very good catch. all our tests use dispatchToProcessor directly, therefore we never noticed this overwrite! will remove it

mzegla · 2025-04-23T11:06:47Z

src/test/http_openai_handler_test.cpp

        ovms::StatusCode::OK);

    std::string expectedResponse = R"(/v3/v1/completions/
-
+content-typeapplication/json


is that correct?

yes, the calculator concatenates all headers keys and values so we can test the final output

mzegla · 2025-04-23T11:15:15Z

src/http_rest_api_handler.cpp

-        if (!doc->IsObject()) {
-            return Status(StatusCode::JSON_INVALID, "JSON body must be an object");
-        }
+        std::string model_name;


Suggested change

std::string model_name;

std::string modelName;

mzegla · 2025-04-23T11:21:55Z

src/test/multipart_calculator_test.cpp

+    });
+
+    ASSERT_EQ(
+        handler->dispatchToProcessor("/v3/v1/completions/", requestBody, &response, comp, responseComponents, writer, multiPartParser),


Using /v3/v1/completions here is a bit confusing to me. It's pretty much OpenAI dedicated URI and here it's used for completely different thing. If I understand it correctly you could use any other v3 uri like /v3/test/multipart and it would work since model name is in the content right?

this is only a test and does not matter what URI we pass. i can change it to any but multipart. this test ensures that we route not based on URI, but rather multipart model field

will change to any URI and comment

atobiszei · 2025-04-23T14:27:21Z

src/BUILD

+    local_defines = COMMON_LOCAL_DEFINES,
+    copts = COPTS_ADJUSTED,
+    linkopts = LINKOPTS_ADJUSTED,
+    alwayslink = 1,


is it required?

it wasnt needed, removed that

atobiszei · 2025-04-23T14:31:17Z

src/http_frontend/multi_part_parser_drogon_impl.hpp

+    bool parse() override;
+
+    bool hasParseError() const override;
+


Add comment that it returns empty in case of error

atobiszei · 2025-04-24T09:35:33Z

src/test/mediapipe/calculators/BUILD

@@ -49,6 +49,7 @@ cc_library(
        "streaming_test_calculator.cpp",
        "two_input_calculator.cpp",
        "openai_chat_completions_mock_calculator.cpp",
+        "multipart_accepting_calculator.cpp",


This calculator will be rebuild with every change in ovms_lib and its dependencies. We could depend only onhttp payload. Please check if other calculators are easily splittable as well.

atobiszei · 2025-04-24T09:55:40Z

src/http_frontend/multi_part_parser_drogon_impl.hpp

+        request(request),
+        parser(std::make_shared<drogon::MultiPartParser>()) {}
+
+    bool parse() override;


Why we need both parse & hasParseError?

parse is called once, by ovms core in processV3 during route logic, hasParseError is API for calculator users, to check if request was valid multipart content

atobiszei · 2025-04-24T10:21:37Z

src/http_rest_api_handler.cpp

+            model_name = std::string(uri.substr(4));
+            SPDLOG_DEBUG("Model name from deduced from URI: {}", model_name);
+            // Set json parser in invalid state in order to get HasParseError to respond with true
+            doc->Parse("error");


Make it more explicit what happens here and why is it required

ensureDocInErrorState?

atobiszei · 2025-04-24T10:30:34Z

src/http_rest_api_handler.cpp

@@ -163,7 +163,7 @@ void HttpRestApiHandler::registerHandler(RequestType type, HandlerCallbackFn f)
 }

 void HttpRestApiHandler::registerAll() {
-    registerHandler(Predict, [this](const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, HttpResponseComponents& response_components, std::shared_ptr<HttpAsyncWriter> serverReaderWriter) -> Status {
+    registerHandler(Predict, [this](const std::string_view uri, const HttpRequestComponents& request_components, std::string& response, const std::string& request_body, HttpResponseComponents& response_components, std::shared_ptr<HttpAsyncWriter> serverReaderWriter, std::shared_ptr<MultiPartParser> multiPartParser) -> Status {


Why this need to be shared_ptr?

dkalinowski · 2025-04-25T14:30:58Z

src/test/http_openai_handler_test.cpp

        ovms::StatusCode::OK);

    std::string expectedResponse = R"(/v3/v1/completions/
-
+content-typeapplication/json


Suggested change

content-typeapplication/json

Key: content-type Value: application/json

???

will fix it to be more readable

dkalinowski added 22 commits March 6, 2025 11:01

save

b77f219

save

78a0e1e

save

9c070cb

save

eddcb01

save

a17f3a1

save

20f4818

save

e2d712a

save

5b40e69

save

dafa7d1

adjust unit tests

4c93d9e

test

0f5fcd7

test

ab09d59

test

f4e02d9

Test

3a4a6e7

Merge remote-tracking branch 'origin/main' into multipart-rb

a020028

ut

b9292a0

compilable but failing unit tests

c11d885

3 fails remaining

e91b214

all working

684b0eb

add test

4980010

push

e17afe4

save

156bcc7

dkalinowski added the WIP Do not merge until resolved label Apr 18, 2025

dkalinowski added 7 commits April 18, 2025 16:40

save

96eeb09

save

b463504

save

bee88e9

save

a8c3bfa

save

3c0adf0

save

7461504

sanity tests

3047943

dkalinowski requested review from atobiszei and mzegla April 22, 2025 11:15

mzegla reviewed Apr 23, 2025

View reviewed changes

atobiszei reviewed Apr 23, 2025

View reviewed changes

atobiszei reviewed Apr 24, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into multipart-rb

4246051

dkalinowski commented Apr 25, 2025

View reviewed changes

mzegla approved these changes Apr 25, 2025

View reviewed changes

dkalinowski added 2 commits April 25, 2025 17:43

post review

0641d39

save

4b4336e

dkalinowski force-pushed the multipart-rb branch from 4bfc031 to 4b4336e Compare April 25, 2025 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multipart requests, new ways of routing the request to MP graph #3250

Support for multipart requests, new ways of routing the request to MP graph #3250

dkalinowski commented Apr 18, 2025 •

edited

Loading

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

mzegla Apr 23, 2025

dkalinowski Apr 25, 2025

dkalinowski Apr 25, 2025

atobiszei Apr 23, 2025

dkalinowski Apr 25, 2025

atobiszei Apr 23, 2025

dkalinowski Apr 25, 2025

atobiszei Apr 24, 2025

atobiszei Apr 24, 2025

dkalinowski Apr 25, 2025

atobiszei Apr 24, 2025

dkalinowski Apr 25, 2025

atobiszei Apr 24, 2025

dkalinowski Apr 25, 2025

dkalinowski Apr 25, 2025

		@@ -0,0 +1,49 @@
		//*****************************************************************************
		// Copyright 2024 Intel Corporation

	// Copyright 2024 Intel Corporation
	// Copyright 2025 Intel Corporation

	content-typeapplication/json
	Key: content-type Value: application/json

Support for multipart requests, new ways of routing the request to MP graph #3250

Are you sure you want to change the base?

Support for multipart requests, new ways of routing the request to MP graph #3250

Conversation

dkalinowski commented Apr 18, 2025 • edited Loading

🛠 Summary

🧪 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkalinowski commented Apr 18, 2025 •

edited

Loading