Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stored saga json state can cause custom tooling parsing issues on SQL Server #1331

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

ramonsmits
Copy link
Member

@ramonsmits ramonsmits commented Nov 14, 2023

When updating saga state where the serialized json is smaller than whats already present in the database the updated payload is corrupted.

Expected Behavior

When string or array values are updated with shorted values that there will be not data left from previous versions:

First : `111111`
Second: `2222`

Observed Behavior

In version 7.0.3 data of the previous version will remain in the database column for SQL server.

First:  `111111`
Second: `222211` // Trailing `11` from previous value

How to reproduce

  1. Design a saga that has an array or string value
  2. Create a saga instance that has a value with size X (i.e. 111)
  3. Update the saga value with a value that is less than X (i.e. 11)
  4. Query the SQL Server database table and observe that the value will have some trailing data from the previous value

Root cause

7.0.3 introduced logic to set the SQL command parameter length to either 4000 or -1 for strings and arrays. This to ensure that SQL server will not create a new query plan so it will use the query cache.

Unfortunately, this has a side effect that SQL Server does not remove trailing data in an existing column for INSERT/UPDATE if the new value is shorter than the previous.

Resolution

The length should only be set to 4000/-1 for SELECT queries

Versions

NServiceBus.Persistence.Sql 7.0.3

Who's affected

Only version 7.0.3 is affected when using SQL Server

Additional Information

…PDATE. Now only applied for `ExecuteReaderAsync` and `ExecuteReaderAsync`.
@andreasohlund
Copy link
Member

Seems like we need to use a separate table for the new test to not break others?

@andreasohlund andreasohlund changed the title Fix issue where sql parameter optimalisation was applied for INSERT/UPDATE Stored json gets corrupted on SqlServer after an update with a smaller sized payload Nov 15, 2023
@andreasohlund andreasohlund enabled auto-merge (squash) November 15, 2023 08:22
@andreasohlund andreasohlund merged commit 3f48828 into master Nov 15, 2023
9 checks passed
@andreasohlund andreasohlund deleted the fix-trailing-state branch November 15, 2023 08:25
ramonsmits added a commit that referenced this pull request Nov 15, 2023
…r sized payload (#1331)

* Fix issue where sql parameter optimalisation was applied for INSERT/UPDATE. Now only applied for `ExecuteReaderAsync` and `ExecuteReaderAsync`.

* Fix failing CI tests due to new test using incorrect table for testing

* Updated the test name to better express its intent
@ramonsmits ramonsmits added this to the 7.0.4 milestone Nov 15, 2023
andreasohlund pushed a commit that referenced this pull request Nov 15, 2023
…r sized payload (#1332)

* Stored json gets corrupted on SqlServer after an update with a smaller sized payload (#1331)

* Fix issue where sql parameter optimalisation was applied for INSERT/UPDATE. Now only applied for `ExecuteReaderAsync` and `ExecuteReaderAsync`.

* Fix failing CI tests due to new test using incorrect table for testing

* Updated the test name to better express its intent

* Version still supports NETFRAMEWORK and await  using isn't supported in NETFX
@ramonsmits ramonsmits changed the title Stored json gets corrupted on SqlServer after an update with a smaller sized payload Stored saga json state can cause custom tooling parsing issues on SQL Server Nov 16, 2023
@SimonCropp
Copy link
Contributor

what versions does this effect?

@andreasohlund
Copy link
Member

Only 7.0.3 (see "Who's affected")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants