Skip to content

fix(session): store session_kv_data.svalue as NVARCHAR(MAX) on MSSQL - #421

Merged
usernane merged 2 commits into
mainfrom
dev
Sep 27, 2026
Merged

usernane merged 2 commits into
mainfrom
dev

Conversation

@usernane

Copy link
Copy Markdown
Member

Summary

Store the per-key session value column (session_kv_data.svalue) as NVARCHAR(MAX) on SQL Server so a single session key is not capped at 4000 characters.

Motivation

The per-key value column is declared as mediumtext. On MySQL that is ~16 MB, but on SQL Server the database layer auto-maps an unsized text type to nvarchar(4000), silently limiting each session key's value to 4000 characters. Any larger value (e.g. a serialized user object, a big cart, cached payload) would be truncated or rejected on MSSQL only. This also aligns with the recent MSSQL per-key session regressions.

Changes

  • SessionSchema::createSessionKvDataTable() now declares svalue per engine:
    • MSSQL: nvarchar with size -1 → renders as NVARCHAR(MAX) (~2 GB).
    • MySQL: mediumtext (unchanged, ~16 MB).
    • SQLite: unbounded text (unchanged).
  • No chunking is introduced; the per-key version/optimistic-concurrency model is untouched.

UI Changes

Not applicable.

How to Test / Verify

Unit tests in tests/WebFiori/Framework/Tests/Session/SessionSchemaTest.php:

  • testKvDataSvalueIsNvarcharMaxOnMSSQL — asserts MSSQL svalue is nvarchar with size -1.
  • testKvDataSvalueIsMediumTextOnMySQL — asserts MySQL keeps mediumtext.
  • testKvDataSessionIdMatchesSessionsPk — asserts the s_id FK type/size matches the sessions PK on mysql/mssql/sqlite.

Also verified manually that the migration's generated CREATE TABLE for MSSQL contains [svalue] [nvarchar](max) not null. Schema test group: 8 tests / 31 assertions passing.

Breaking Changes and Migration Steps

None for fresh installs — new session_kv_data tables get the correct column type. NOTE: existing MSSQL deployments that already ran the 3.1.0 migration will still have nvarchar(4000); widening an existing column (ALTER COLUMN svalue NVARCHAR(MAX)) is intentionally out of scope for this PR.

Checklist

  • I reviewed my own diff before requesting review
  • My commits follow Conventional Commits
  • I added/updated tests (or explained why not)
  • I updated docs (if needed) Docs Repo
  • I ran lint/cs-fixer (if applicable) (composer fix-cs)
  • I considered backward compatibility
  • I considered security

Related issues

Not Applicable

Ibrahim BinAlshikh added 2 commits September 27, 2026 15:37
On SQL Server the per-key value column (declared as 'mediumtext') was
auto-mapped to nvarchar(4000), capping a single session key's value at
4000 characters — far smaller than MySQL's mediumtext (~16 MB). Declare
svalue as nvarchar with size -1 for MSSQL so the schema/migration render
it as NVARCHAR(MAX) (~2 GB). MySQL keeps mediumtext and SQLite keeps an
unbounded text column. The per-key version/optimistic-concurrency model
is unchanged (no chunking needed).
…tches PK

Covers the MSSQL nvarchar(max) fix for session_kv_data.svalue, that
MySQL keeps mediumtext, and that the session-ID foreign key type/size
matches the sessions primary key across mysql/mssql/sqlite.
@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.75%. Comparing base (ea4946e) to head (1517dd3).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #421      +/-   ##
============================================
+ Coverage     81.73%   81.75%   +0.01%     
- Complexity     3287     3288       +1     
============================================
  Files           106      106              
  Lines          9714     9721       +7     
============================================
+ Hits           7940     7947       +7     
  Misses         1774     1774              
Flag Coverage Δ
php-8.1 80.57% <100.00%> (+<0.01%) ⬆️
php-8.2 80.57% <100.00%> (+<0.01%) ⬆️
php-8.3 88.96% <100.00%> (+<0.01%) ⬆️
php-8.4 88.96% <100.00%> (+<0.01%) ⬆️
php-8.5 88.84% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud

Copy link
Copy Markdown

@usernane
usernane merged commit ea6999a into main Sep 27, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant