[k2] add support multipart/form-data to HTTP server #1423

nekishdev · 2025-10-09T09:37:32Z

No description provided.

apolyakov · 2025-10-10T09:17:44Z

runtime-light/server/http/init-functions.cpp

 #include "runtime-common/core/runtime-core.h"
 #include "runtime-common/core/std/containers.h"
 #include "runtime-common/stdlib/server/url-functions.h"
+#include "runtime-common/stdlib/string/string-functions.h"


Looks like these includes are actually unused

apolyakov · 2025-10-10T09:22:11Z

runtime-light/server/http/init-functions.cpp

      f$parse_str(body_str, superglobals.v$_POST);
    } else if (content_type.starts_with(CONTENT_TYPE_MULTIPART_FORM_DATA)) {
-      kphp::log::error("unsupported content-type: {}", CONTENT_TYPE_MULTIPART_FORM_DATA);
+      std::string_view boundary = parse_boundary(content_type);


Minor comments:

We use braced initialization throughout the codebase. Please, update the initialization to use braces: std::string_view boundary{parse_boundary};.

Ensure consistency in namespace usage by either consistently applying the kphp::http:: prefix or omitting it across the codebase. Let's only use single approach for clarity and maintainability.

apolyakov · 2025-10-10T09:31:17Z

runtime-light/server/http/multipart.h

Please, add missing copyright header

apolyakov · 2025-10-10T09:33:13Z

runtime-light/server/http/multipart.h

+
+namespace kphp::http {
+
+void parse_multipart(const std::string_view &body, const std::string_view &boundary, mixed &v$_POST, mixed &v$_FILES);


Actually, there is no need to passing a std::string_view object by reference as it's quite small. Passing by reference introduces an indirection on the other hand

apolyakov · 2025-10-10T09:33:28Z

runtime-light/server/http/multipart.h

+
+void parse_multipart(const std::string_view &body, const std::string_view &boundary, mixed &v$_POST, mixed &v$_FILES);
+
+std::string_view parse_boundary(const std::string_view &content_type);


Please, mark all functions noexcept

apolyakov · 2025-10-10T11:42:14Z

runtime-light/server/http/multipart.cpp

+constexpr std::string_view HEADER_CONTENT_DISPOSITION_FORM_DATA = "form-data;";
+constexpr std::string_view MULTIPART_BOUNDARY_EQ = "boundary=";
+
+struct Header {


Just to follow the code style of the runtime-light, let's name types in lowercase

apolyakov · 2025-10-10T11:42:51Z

runtime-light/server/http/multipart.cpp

+struct Header {
+  std::string_view header_str, name, value;
+
+  Header() {}


Please, use = default if you want a trivial constructor

apolyakov · 2025-10-10T11:44:07Z

runtime-light/server/http/multipart.cpp

+    : header_str{header_str_} {}
+
+  void parse() {
+    char delim = ':';


There is vk::split_string_view function. Here is the usage reference: runtime-light/server/http/init-functions.cpp:119

apolyakov · 2025-10-10T11:46:08Z

runtime-light/server/http/multipart.cpp

+  }
+
+  size_t 
+    attrStart = header.find_first_not_of(' ', pos),


Please, use separate lines to initialize/assign multiple variables

apolyakov · 2025-10-10T11:51:52Z

runtime-light/server/http/multipart.cpp

+      file[string("size")].push_back(file_size);
+      file[string("tmp_name")].push_back(string(tmp_name.data(), tmp_name.size()));
+      file[string("error")].push_back(0);
+    } else {


Is it really possible that file_size < 0?

astrophysik · 2026-02-12T08:06:54Z

runtime-light/server/http/multipart.h

+
+void parse_multipart(const std::string_view body, const std::string_view boundary, mixed &v$_POST, mixed &v$_FILES);
+
+std::string_view parse_boundary(const std::string_view content_type);


Let's change return type to std::optional<std::string_view> to indicate that result can be none. Also const in function arg type isn't meaningful

astrophysik · 2026-02-12T08:21:09Z

runtime-light/server/http/multipart.cpp

+    }
+  }
+
+  bool is_valid() {


Can we make object header to be valid always? For example it can take two arguments name and value and create object by static method.
example

struct header { std::string_view m_name; std::string_view m_value; header() = delete; static std::optional<header> new(std::string_view name, std::string_view value) noexcept{...} bool name_is(const std::string_view name) const noexcept {...} };

I think it will simplify work this with type

astrophysik · 2026-02-12T08:23:09Z

runtime-light/server/http/multipart.cpp

+// 1) attr = "name", value = "avatar"
+// 2) attr = "filename", value = "my_avatar.png"
+struct partAttr {
+  std::string_view attr, value;


Let's separate the field declarations into different lines

astrophysik · 2026-02-12T08:24:15Z

runtime-light/server/http/multipart.cpp

+struct partAttr {
+  std::string_view attr, value;
+
+  partAttr() = default;


In fact, default constructor used only in next_attr. We can get rid of it if we use optional

astrophysik · 2026-02-12T08:26:45Z

runtime-light/server/http/multipart.cpp

+  std::string_view attr, value;
+
+  partAttr() = default;
+  partAttr(const std::string_view attr_, const std::string_view value_) : attr{attr_}, value{value_} {};


Let's remove const from args and add it to type fields, this will be more useful

astrophysik · 2026-02-12T08:31:14Z

runtime-light/server/http/multipart.cpp

+};
+
+// Represents a parser of Content-Disposition header string.
+struct attrParser {


We can use use class here to reduce private/public keywords
class attrParser { // private data public: // public data }

astrophysik · 2026-02-12T08:33:34Z

runtime-light/server/http/multipart.cpp

+// Represents a parser of Content-Disposition header string.
+struct attrParser {
+  private:
+    std::string_view header;


This name is a bit confusing. We’ve declared a header type above, but this variable has a different type. As I understand it from how it’s used, it’s the value field of header. Maybe we should call it header_value?

astrophysik · 2026-02-12T08:35:18Z

runtime-light/server/http/multipart.cpp

+  public:
+    attrParser(const std::string_view header_) : header{header_} {}
+    partAttr next_attr();
+    bool end() {


Is this method actually useful? If we make next_attr return an optional type, then when using it we could keep calling it until it starts returning None

astrophysik · 2026-02-12T08:37:09Z

runtime-light/server/http/multipart.cpp

+    }
+
+  private:
+    void markEnd() {


I think this method can be removed. It’s used only once in the implementation, but make class more complex

astrophysik · 2026-02-12T08:38:24Z

runtime-light/server/http/multipart.cpp

+}
+
+// Represents one part of multipart content
+struct part {


Same about different lines

astrophysik · 2026-02-12T08:38:57Z

runtime-light/server/http/multipart.cpp

+  std::string_view name, filename, content_type, data;
+};
+
+struct MultipartBody {


Same about class and public/private keywords

astrophysik · 2026-02-12T08:40:30Z

runtime-light/server/http/multipart.cpp

+
+namespace {
+
+const int TMP_FILENAME_LENGTH = 10;


Let's use constexpr and bounded int type int8_t, int16_t, etc.

astrophysik · 2026-02-12T08:42:42Z

runtime-light/server/http/multipart.cpp

+
+    // Returns true if current pos refers to one of \r, \n, \r\n
+    bool is_crlf() {
+      return body[pos] == '\r' || body[pos] == '\n' || (body[pos] == '\r' && body[pos+1] == '\n');


Third condition is unreachable since body[pos] == '\r' since already checked

astrophysik · 2026-02-12T09:01:35Z

runtime-light/server/http/multipart.h

+
+namespace kphp::http {
+
+void parse_multipart(const std::string_view body, const std::string_view boundary, mixed &v$_POST, mixed &v$_FILES);


I think this interface could be improved, but I can’t give a precise opinion. Maybe we should think in the direction of parse_multipart(body, boundary) -> ranges<part>?

astrophysik · 2026-02-12T09:21:27Z

runtime-light/server/http/multipart.cpp

+};
+
+partAttr attrParser::next_attr() {
+  if (pos == 0) {


Generally about this function. As I understand, code use it like

while(!end()) { next_attr() }

I think it can be simplified by c++ ranges, something like:

auto res = std::views::split(header, ";") | std::views::transform([](auto part) { std::string_view part_view{vk::trim(std::string_view(part))}; auto [name_view, value_view]{vk::split_string_view(part_view, '=')}; if (value_view.size() >= 2 && value_view.starts_with('"') && value_view.ends_with('"')) { value_view = value_view.substr(1, value_view.size()-2); } return partAttr{name_view, value_view}; }); return res;

Case for pos == 0 and HEADER_CONTENT_DISPOSITION_FORM_DATA can be processed in constructor. If it can be rewrited by ranges, we also can simplify this logic to one function parse_part_attr(header_value) -> ranges<partAttr>

apolyakov requested changes Oct 10, 2025

View reviewed changes

nekishdev force-pushed the ngoryanoy/k2/http-multipart branch 3 times, most recently from 9a96972 to e22a5e9 Compare October 15, 2025 15:45

nekishdev force-pushed the ngoryanoy/k2/http-multipart branch 2 times, most recently from 65da1f4 to 8374d13 Compare February 11, 2026 11:14

[k2] add support multipart/form-data to HTTP server

f1a82c7

nekishdev force-pushed the ngoryanoy/k2/http-multipart branch from 8374d13 to f1a82c7 Compare February 11, 2026 11:17

astrophysik reviewed Feb 12, 2026

View reviewed changes


		namespace kphp::http {

		void parse_multipart(const std::string_view &body, const std::string_view &boundary, mixed &v$_POST, mixed &v$_FILES);


		void parse_multipart(const std::string_view &body, const std::string_view &boundary, mixed &v$_POST, mixed &v$_FILES);

		std::string_view parse_boundary(const std::string_view &content_type);

[k2] add support multipart/form-data to HTTP server #1423

Are you sure you want to change the base?

[k2] add support multipart/form-data to HTTP server #1423

Conversation

nekishdev commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrophysik Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

astrophysik Feb 12, 2026 •

edited

Loading