Conformance tests & bugfixes for SQLServer

[berndverst]

Description

Adds conformance tests to and fixes bugs in the SqlServer state store component.

The new conformance test suite has been added to the GitHub workflow and has already successfully run as part of this PR.

Conformance tests can manually be run like so:

docker run --name sqlserver -e "ACCEPT_EULA=Y" -e "SA_PASSWORD=Pass@Word1" -p 1433:1433 -d mcr.microsoft.com/mssql/server:2019-GA-ubuntu-16.04
go test -v -tags=conftests -count=1 ./tests/conformance -run="TestStateConformance/sqlserver"

Integration tests can then be run via the following which checks for the same variable to be set

export DAPR_TEST_SQL_CONNSTRING="server=localhost;user id=sa;password=Pass@Word1;port=1433;"
go test -v ./state/sqlserver

Note that I did not add the DAPR_TEST_SQL_CONNSTRING variable. This has been in the code for a long time.

Issue reference

#949
Fixes #1036

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

[CodeMonkeyLeet]

Thanks for all the hard work digging into this! The stored proc logic still seems iffy to me though, might want to check with the maintainers as to the expectations here.

[artursouza]

Great progress. It is almost there.

[berndverst]

@artursouza sorry for all the confusion here. I believe I was able to address all concerns and was also able to simplify some error handling.

Please note that the code changed can't meaningfully be tested via unit tests -- there are no similar unit tests today. There are however integration tests at state/sqlserver/sqlserver_integration_test.go which can be run by setting the environment variable I mentioned. Unfortunately CODE COV has no awareness of these tests so the drop in test coverage isn't accurate.

[artursouza]

LGTM. Thanks for revisiting the logic in the stored procedure. Please, wait for approval from @CodeMonkeyLeet too.

[CodeMonkeyLeet]

See extended discussion on existing questions on error state handling. I'm okay with taking the PR, modulo tracking ongoing issues in the concurrency handling contract in Dapr.

[CodeMonkeyLeet]

This is not always the case though: this case can also be because the row was never written before and the key does not exist.

[berndverst]

That's an excellent point. What do you suggest? Changing the error message?

If the record does not exist it's not actually possible to specify an ETAG (Row Version) to be written -- the stored procedure / DB won't allow the use of ETAGs other than to update an existing record.

[CodeMonkeyLeet]

Yeah, I would just change the error message to something more general

[CodeMonkeyLeet]

Overall, I think the logic here is still problematic. I understand the desire not to churn existing code, but it looks like there are still issues relative to the expected behaviors expressed in #2739 1c. Condensed here for reference:

StateOptions.Concurrency	Req.Etag	Expected Behavior in State Store
CONCURRENCY_FIRST_WRITE	not nil	Write if etag matches
CONCURRENCY_FIRST_WRITE	nil	Write if key does not exist
CONCURRENCY_LAST_WRITE or unspecified	not nil	Return error (Upcoming breaking change. Today, state components write regardless of etag)
CONCURRENCY_LAST_WRITE or unspecified	nil	Write regardless of etag

"Write" in this case refers to an upsert (i.e. insert key if it does not already exist, otherwise update). The original code ignores the top half of that table, but it also looks like it already did not implement the bottom half correctly. Please correct any readings of the code here:

• If an ETag is provided and the key does not exist, it attempts to update, which returns 0 rows modified but does not error and does not insert the value. It is silently lost.
• If an ETag is provided and the key does exist, it will attempt to update using the ETag. If the ETag does not match, it again does not error and does not update the value, which has FIRST_WRITE semantics instead of LAST_WRITE (default) semantics. It should ignore the ETag completely in this case.

Preserving the original logic propagates these issues forwards. The new logic for handling FIRST_WRITE works, although it raises some questions about the specification for behavior:

• If ETag provided and key does not exist, ETag can never match. Should the method raise an error or silently ignore the write?
  • This implementation returns an error
• If ETag provided and key exists, but ETag does not match, should the method raise an error or silently ignore the write?
  • This implementation silently ignores the update. (The update on row 290 finds no matches).

That seems potentially inconsistent as an error contract for Dapr.

@artursouza To keep this moving forwards, since you've already approved the PR and the new code is okay, I suggest we file a separate issue to track the logical inconsistencies in the existing code. In continuing the discussion of Dapr ETag/concurrency state handling, I'll also follow up on the issue with my questions on the expected error handling.

[artursouza]

The issue for LAST_WRITE and eTag not null is behavior change beyond the scope of this change because it is a broader concern of the expectations for the state API behavior. I am OK with the current logic in this component.

The error handling for silent etag mistmatch in the stored procedure is handled in code by checking number of rows affected.

@CodeMonkeyLeet Let me know if you have any other concerns on this PR.

[CodeMonkeyLeet]

Nope, I think it's definitely a step forward and we should take it, thanks for reminding me about the downstream handling of returning errors from the zero rows updated check.

[codecov]

Codecov Report

Merging #1078 (fb8c824) into master (253ef85) will decrease coverage by 0.54%.
The diff coverage is 23.64%.

@@            Coverage Diff             @@
##           master    #1078      +/-   ##
==========================================
- Coverage   34.53%   33.98%   -0.55%     
==========================================
  Files         132      134       +2     
  Lines       10870    11283     +413     
==========================================
+ Hits         3754     3835      +81     
- Misses       6736     7057     +321     
- Partials      380      391      +11     

Impacted Files	Coverage Δ
authentication/azure/storage.go	0.00% <0.00%> (ø)
...indings/azure/servicebusqueues/servicebusqueues.go	15.06% <0.00%> (-0.21%)	⬇️
bindings/postgres/postgres.go	6.66% <0.00%> (-0.38%)	⬇️
pubsub/rocketmq/rocketmq.go	0.00% <ø> (ø)
pubsub/rocketmq/settings.go	86.66% <ø> (ø)
state/azure/cosmosdb/cosmosdb.go	16.52% <0.00%> (+0.21%)	⬆️
state/azure/tablestorage/tablestorage.go	13.20% <0.00%> (ø)
state/sqlserver/migration.go	0.00% <0.00%> (ø)
state/sqlserver/sqlserver.go	42.80% <0.00%> (-0.33%)	⬇️
tests/conformance/common.go	14.79% <0.00%> (-0.14%)	⬇️
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71f4748...fb8c824. Read the comment docs.