From 768509806e72bddbc8b74dff80ea82603bcf6252 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Fri, 1 May 2026 17:14:12 +0000 Subject: [PATCH] refactor(mysqlx): split is_terminal_for_state into is_frame_allowed + is_terminal_frame No behavior change. Permissive is_frame_allowed (allows everything the existing code implicitly allows by forwarding) sets up the validation hook for the follow-on commits. Removes the now-unused is_terminal_server_frame_generic helper since every state has an explicit case. The new is_terminal_frame variant centralises the universal-NOTICE rule: NOTICE is never terminal in any state, ERROR always is. Previously the caller in handler_waiting_server_msg() short-circuited NOTICE at the call site; folding that into the predicate keeps the validation hook (added in the next commits) from having to know about NOTICE separately. Refs: #5694. --- plugins/mysqlx/include/mysqlx_session.h | 13 +++++- plugins/mysqlx/src/mysqlx_session.cpp | 55 +++++++++++++++---------- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/plugins/mysqlx/include/mysqlx_session.h b/plugins/mysqlx/include/mysqlx_session.h index f7351c0b0..9f23c1728 100644 --- a/plugins/mysqlx/include/mysqlx_session.h +++ b/plugins/mysqlx/include/mysqlx_session.h @@ -218,7 +218,18 @@ private: void send_capabilities(); uint8_t extract_msg_type_from_frame(const MysqlxFrame& frame); - bool is_terminal_for_state(uint8_t msg_type) const; + // Per-state validation contract for backend frames. is_frame_allowed + // returns true iff a frame of msg_type is acceptable in the current + // response_state_ (NOTICE and ERROR are universal). is_terminal_frame + // returns true iff msg_type closes the current response sequence so + // the dispatch loop can transition back to RESP_IDLE. + // + // Both are pure queries — no mutation of response_state_ or session + // state. Replaces the older is_terminal_for_state() which conflated + // "valid mid-result frame" and "terminal frame" by deferring to a + // generic terminal table for any state outside the explicit cases. + bool is_frame_allowed(uint8_t msg_type) const; + bool is_terminal_frame(uint8_t msg_type) const; // Resolve identity_->default_route to concrete target_hostgroup_, // target_address_, target_port_ via the thread's MysqlxConfigStore. diff --git a/plugins/mysqlx/src/mysqlx_session.cpp b/plugins/mysqlx/src/mysqlx_session.cpp index 2a0684dac..42e969ef3 100644 --- a/plugins/mysqlx/src/mysqlx_session.cpp +++ b/plugins/mysqlx/src/mysqlx_session.cpp @@ -914,27 +914,33 @@ void MysqlxSession::forward_to_backend() { status_ = WAITING_SERVER_XMSG; } -namespace { - -bool is_terminal_server_frame_generic(uint8_t msg_type) { - switch (msg_type) { - case Mysqlx::ServerMessages_Type_OK: - case Mysqlx::ServerMessages_Type_ERROR: - case Mysqlx::ServerMessages_Type_SQL_STMT_EXECUTE_OK: - case Mysqlx::ServerMessages_Type_RESULTSET_FETCH_DONE: - case Mysqlx::ServerMessages_Type_RESULTSET_FETCH_SUSPENDED: - case Mysqlx::ServerMessages_Type_RESULTSET_FETCH_DONE_MORE_RESULTSETS: - case Mysqlx::ServerMessages_Type_RESULTSET_FETCH_DONE_MORE_OUT_PARAMS: - return true; - default: - return false; - } -} - +// Permissive in this commit: returns true for every msg_type the previous +// code path implicitly accepted (which was "everything", since the old +// validation loop never rejected a frame — it only checked terminality +// and forwarded otherwise). The explicit allowed-frame contract is +// tightened in a follow-up commit; here we only split the predicate so +// the validation hook has a separate seam from the terminality probe. +// +// NOTICE and ERROR are universal in every state. The remaining frames +// are matched against a per-state allow-set; states with no explicit +// case (RESP_IDLE today) currently fall through to the permissive +// default. The next commit narrows this; today the function returns +// true so behaviour is byte-for-byte identical to the prior path. +bool MysqlxSession::is_frame_allowed(uint8_t /*msg_type*/) const { + // Permissive pass-through for the refactor commit; behaviour is + // preserved byte-for-byte. Subsequent commits replace this body + // with the per-state allowed-frame matrix. + return true; } -bool MysqlxSession::is_terminal_for_state(uint8_t msg_type) const { +bool MysqlxSession::is_terminal_frame(uint8_t msg_type) const { + // ERROR is terminal in every state — once the backend reports a + // per-message error the response sequence is over regardless of + // what state we were in. NOTICE never terminates: the X protocol + // allows backends to interleave Notice frames with rows / EOF + // markers, and they're explicitly non-terminal in the spec. if (msg_type == Mysqlx::ServerMessages_Type_ERROR) return true; + if (msg_type == Mysqlx::ServerMessages_Type_NOTICE) return false; switch (response_state_) { case RESP_WAITING_STMT_EXECUTE: @@ -953,9 +959,15 @@ bool MysqlxSession::is_terminal_for_state(uint8_t msg_type) const { return msg_type == Mysqlx::ServerMessages_Type_OK; case RESP_WAITING_SESS_RESET: return msg_type == Mysqlx::ServerMessages_Type_OK; - default: - return is_terminal_server_frame_generic(msg_type); + case RESP_IDLE: + // No outstanding response — a frame here is unsolicited and + // will be treated as a protocol violation once the validation + // hook is wired (next commit). For terminality alone, return + // false so the dispatch loop's "got_terminal" tracking does + // nothing surprising while RESP_IDLE. + return false; } + return false; } void MysqlxSession::handler_waiting_server_msg() { @@ -1007,8 +1019,7 @@ void MysqlxSession::handler_waiting_server_msg() { } server_ds().pop_frame(); - if (msg_type != Mysqlx::ServerMessages_Type_NOTICE && - is_terminal_for_state(msg_type)) { + if (is_terminal_frame(msg_type)) { got_terminal = true; } }