Skip to content

SqlVariant::Get<T>() is noexcept but calls std::get — a type mismatch terminates the process #607

Description

@christianparpart

Problem

SqlVariant::Get<T>() (src/Lightweight/DataBinder/SqlVariant.hpp) is declared noexcept, and its fallback path is std::get<T>(value). std::get throws std::bad_variant_access when the variant holds a different alternative, and a throw crossing a noexcept boundary calls std::terminate. So asking for the wrong type aborts the process instead of reporting an error.

auto v = SqlVariant { 42 };        // holds int
auto d = v.Get<double>();          // std::terminate — not an exception, not a wrong value

This is reachable from ordinary code: which alternative a column fills depends on the driver's reported SQL type, so a caller can be right about the column and still wrong about the alternative. #602 and #586 are both examples of a driver filling an alternative the caller did not expect.

Scope

Get<T>() converts for the cases where a column's alternative recently moved (a DECIMAL column fills SqlDynamicNumeric, a binary column fills SqlBinary), so those specific conversions are safe. The general mismatch is not. ValueOr<T>() was already made total in #606 — it returns the caller's default rather than reaching for an alternative the variant does not hold.

Why it is filed rather than fixed

Fixing it is a contract decision, not a mechanical change, and there are three defensible answers:

  1. Drop noexcept and let std::bad_variant_access propagate. Matches TryGetNumeric()/TryGetBinary(), which already throw. Changes the exception profile of a hot accessor.
  2. Keep noexcept and return T {} on mismatch. Total, but silently turns a caller bug into a zero — the failure mode this library generally refuses.
  3. Keep Get<T>() for the checked case and steer callers to TryGet*/ValueOr<T>.

(1) looks right, but it is an API change that deserves its own PR rather than riding along with unrelated work.

Context

Found while working on #606, which is also where ValueOr<T>() was fixed. Get<T>() was deliberately left alone there to keep that PR to one concern.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions