Add SQL-InsertAndSelect with associated test project - #2
Conversation
| var cmdText = "SELECT TestColumn FROM TestTable"; | ||
| if (condition != null) | ||
| { | ||
| cmdText += $" WHERE {condition}"; |
There was a problem hiding this comment.
🚨This is a SQL injection issue. We should use SqlParameter or similar as in the Insert method in this class. One way to do this would be to create a string that contains the parameters. It might make sense to update the string? condition to be another type with that to make the constructing of the parametrized string easier.
|
|
||
| namespace SQL_InsertAndSelect.Tests | ||
| { | ||
| public class SqlOperationsTests : IAsyncLifetime |
There was a problem hiding this comment.
Good job using IAsyncLifetime here, rather than IAsyncDisposable, as xUnit v2 does not call IAsyncDisposable. https://xunit.net/docs/shared-context
| using var conn = new SqlConnection(db.ConnectionString); | ||
| await conn.OpenAsync(); | ||
| using var cmd = new SqlCommand("CREATE TABLE TestTable (TestColumn varchar(255))", conn); | ||
| await cmd.ExecuteNonQueryAsync(); |
There was a problem hiding this comment.
While it's great that the database is reused between tests for better performance, the database isn't returned to the original state after each test. Ideally each test would be completely isolated from the others. Do you think dropping and recreating the tables would be a good idea, or is there a different solution we could implement?
|
|
||
| foreach (var item in expected) | ||
| { | ||
| var cmdText = "INSERT INTO TestTable VALUES (@value)"; |
There was a problem hiding this comment.
nit: Multiple values could be inserted in a single database call. It doesn't really matter in this case though as it's test code with a list of 2 items.
| using var conn = new SqlConnection(db.ConnectionString); | ||
| await conn.OpenAsync(); | ||
| using var cmd = new SqlCommand("DROP TABLE TestTable", conn); | ||
| await cmd.ExecuteNonQueryAsync(); |
There was a problem hiding this comment.
What do you think about refactoring the ADO.NET code to reduce duplication? At a minimum we could refactor the InitializeAsync and DisposeAsync methods to reduce them to a single statement calling another method with a string.
| return results; | ||
| } | ||
|
|
||
| public async Task Insert(string value) |
There was a problem hiding this comment.
Would someone using this method potentially be calling it multiple times in a row? If so, should we change the string value to params string[] values to allow multiple items to be inserted at the same time?
| using var cmd = new SqlCommand(cmdText, conn); | ||
| using var reader = cmd.ExecuteReader(); | ||
|
|
||
| var results = new List<string>(); |
There was a problem hiding this comment.
Constructing an in-memory list may work for a small result set, but if we anticipate large amounts of data, we may want to look into IAsyncEnumerable or something else.
| } | ||
|
|
||
| using var cmd = new SqlCommand(cmdText, conn); | ||
| using var reader = cmd.ExecuteReader(); |
There was a problem hiding this comment.
We could use async here, unless there's a reason to leave it synchronous.
| using var reader = cmd.ExecuteReader(); | |
| using var reader = await cmd.ExecuteReaderAsync(); |
| using var reader = cmd.ExecuteReader(); | ||
|
|
||
| var results = new List<string>(); | ||
| while (reader.Read()) |
There was a problem hiding this comment.
We could use async here, unless there's a reason to leave it synchronous.
| while (reader.Read()) | |
| while (await reader.ReadAsync()) |
1 participant
No description provided.