Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 30 additions & 50 deletions DataGateway.Service.Tests/SqlTests/MySqlRestApiTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -338,14 +338,36 @@ SELECT JSON_OBJECT('id', id) AS data
{
"PutOne_Insert_Test",
@"
SELECT JSON_OBJECT('id', id) AS data
SELECT JSON_OBJECT('id', id, 'title', title, 'issueNumber', issueNumber ) AS data
FROM (
SELECT id, title, publisher_id
FROM " + _integrationTableName + @"
WHERE id > 5000 AND title = 'The Hobbit Returns to The Shire'
AND publisher_id = 1234
SELECT id, title, issueNumber
FROM " + _integration_NonAutoGenPK_TableName + @"
WHERE id > 5000 AND title = 'Batman Returns'
AND issueNumber = 1234
) AS subq
"
},
{
"PutOne_Insert_Nullable_Test",
@"SELECT JSON_OBJECT('id', id, 'title', title, 'issueNumber', issueNumber ) AS data
FROM (
SELECT id, title, issueNumber
FROM " + _integration_NonAutoGenPK_TableName + @"
WHERE id = " + $"{STARTING_ID_FOR_TEST_INSERTS + 1}" + @" AND title = 'Times'
AND issueNumber is NULL
) as subq
"
},
{
"PutOne_Insert_AutoGenNonPK_Test",
@"SELECT JSON_OBJECT('id', id, 'title', title, 'volume', volume ) AS data
FROM (
SELECT id, title, volume
FROM " + _integration_AutoGenNonPK_TableName + @"
WHERE id = " + $"{STARTING_ID_FOR_TEST_INSERTS}" + @" AND title = 'Star Trek'
AND volume IS NOT NULL
) as subq
"
}
};

Expand Down Expand Up @@ -376,60 +398,18 @@ public override string GetQuery(string key)
return _queryMap[key];
}

[TestMethod]
[Ignore]
public override Task InsertOneTest()
{
throw new NotImplementedException();
}

[TestMethod]
[Ignore]
public override Task InsertOneInCompositeKeyTableTest()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test work even though its composite PK? I thought it doesnt... So, why remove the ignore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works as far as only one autogen column.

Copy link
Collaborator

@Aniruddh25 Aniruddh25 Feb 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so the problem with composite key happens only when both are autogenerated.. I'll note this in the issue #264

{
throw new NotImplementedException();
}

[TestMethod]
[Ignore]
public override Task PutOne_Update_Test()
{
throw new NotImplementedException();
}

[TestMethod]
[Ignore]
public override Task PutOne_Insert_Test()
{
throw new NotImplementedException();
}

[TestMethod]
[Ignore]
public override Task PutOne_Insert_BadReq_Test()
{
throw new NotImplementedException();
}

[TestMethod]
[Ignore]
public override Task PutOne_Insert_BadReq_NonNullable_Test()
{
throw new NotImplementedException();
}

[TestMethod]
[Ignore]
public override Task PutOne_Insert_PKAutoGen_Test()
{
throw new NotImplementedException();
throw new NotImplementedException("error: Fail, still able to insert");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you still able to insert ? Is there an issue in request validation ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may need some help to understand the tests here. Why this insert should fail?

Copy link
Collaborator

@Aniruddh25 Aniruddh25 Feb 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This insert should fail because the PK is autogenerated but the request also specifies a PK value in the route. We wouldn't know if the PK specified is present in the DB or not - hence we cannot stop this case in request validation. Instead, we need to send it to the DB and let the DB handle this conflicting situation.

PUT should have upsert semantics is recommended by the API https://github.com/Microsoft/api-guidelines/blob/vNext/Guidelines.md#74-supported-methods
Does MySQL still allow inserting even if it were autogenerated?
What ID does it get inserted in that case?
If it still gets inserted, we can allow the insert to happen for MySQL if the ID with which it is inserted is the same as the request ID. Created this issue to tackle this : #274

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL allow insert with an id which is auto_inc. it will allow to insert anyway. If the id is larger than the current max id, the max id is updated to avoid duplicate id later.

My suggestion is that we should deal with this in high level instead of adapter.

If a autogen id is in the given column list, upsert should be handled as update as insert will anyway fail. The adapter should get a insert request and upper layer handles the different cases. Thoughts?

}

[TestMethod]
[Ignore]
public override Task PutOne_Insert_BadReq_AutoGen_NonNullable_Test()
public override Task PutOne_Update_Test()
{
throw new NotImplementedException();
throw new NotImplementedException("error: While processing your request the server ran into an unexpected error");
}
}
}
4 changes: 2 additions & 2 deletions DataGateway.Service.Tests/SqlTests/SqlTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,8 @@ protected static async Task SetupAndRunRestApiTest(
actionResult,
expected,
expectedStatusCode,
expectedLocationHeader);

expectedLocationHeader,
!exception);
}

/// <summary>
Expand Down
15 changes: 11 additions & 4 deletions DataGateway.Service.Tests/SqlTests/SqlTestHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ public static void VerifyResult(
IActionResult actionResult,
string expected,
HttpStatusCode expectedStatusCode,
string expectedLocationHeader)
string expectedLocationHeader,
bool isJson = false)
{
string actual;
switch (actionResult)
Expand Down Expand Up @@ -158,9 +159,15 @@ public static void VerifyResult(
break;
}

// if whitespaces are not consistent JsonStringDeepEquals should be used
// this will require deserializing and then serializing the strings for JSON
Assert.AreEqual(expected, actual);
Console.WriteLine($"Expected: {expected}\nActual: {actual}");
if (isJson && !String.IsNullOrEmpty(expected))
{
Assert.IsTrue(JToken.DeepEquals(JToken.Parse(expected), JToken.Parse(actual)));
}
else
{
Assert.AreEqual(expected, actual);
}
}
}
}
9 changes: 8 additions & 1 deletion DataGateway.Service/MySqlBooks.sql
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ DROP TABLE IF EXISTS authors;
DROP TABLE IF EXISTS books;
DROP TABLE IF EXISTS publishers;
DROP TABLE IF EXISTS magazines;
DROP TABLE IF EXISTS comics;

CREATE TABLE publishers(
id bigint AUTO_INCREMENT PRIMARY KEY,
Expand Down Expand Up @@ -43,6 +44,12 @@ CREATE TABLE magazines(
issueNumber bigint NULL
);

CREATE TABLE comics(
id bigint PRIMARY KEY,
title text NOT NULL,
volume bigint AUTO_INCREMENT UNIQUE KEY
);

ALTER TABLE books
ADD CONSTRAINT book_publisher_fk
FOREIGN KEY (publisher_id)
Expand Down Expand Up @@ -80,5 +87,5 @@ ALTER TABLE books AUTO_INCREMENT = 5001;
ALTER TABLE publishers AUTO_INCREMENT = 5001;
ALTER TABLE authors AUTO_INCREMENT = 5001;
ALTER TABLE reviews AUTO_INCREMENT = 5001;

ALTER TABLE comics AUTO_INCREMENT = 5001

89 changes: 78 additions & 11 deletions DataGateway.Service/Resolvers/MySqlQueryBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,33 @@ public string Build(SqlQueryStructure structure)
/// <inheritdoc />
public string Build(SqlInsertStructure structure)
{
// TODO: these should be put in a transcation
// No need to put into transaction as LAST_INSERT_ID is session level variable
return $"INSERT INTO {QuoteIdentifier(structure.TableName)} ({Build(structure.InsertColumns)}) " +
$"VALUES ({string.Join(", ", (structure.Values))}); " +
$"SELECT {MakeInsertSelections(structure)}";
$" SET @ROWCOUNT=ROW_COUNT(); " +
$"SELECT {MakeInsertSelections(structure)} WHERE @ROWCOUNT > 0;";
}

/// <inheritdoc />
public string Build(SqlUpdateStructure structure)
{
// TODO: these should be put in a transaction
return $"UPDATE {QuoteIdentifier(structure.TableName)} " +
// Create local variables to store the pk columns
string sets = String.Join(";\n", structure.PrimaryKey().Select((x, index) => $"SET {"@LU_" + index.ToString()} := 0"));

// Fetch the value to local variables
string updates = String.Join(", ", structure.PrimaryKey().Select((x, index) =>
$"{QuoteIdentifier(x)} = (SELECT {"@LU_" + index.ToString()} := {QuoteIdentifier(x)})"));

// Select local variables and mapping to original column name
string select = String.Join(", ", structure.PrimaryKey().Select((x, index) => $"{"@LU_" + index.ToString()} AS {QuoteIdentifier(x)}"));

return sets + ";\n" +
$"UPDATE {QuoteIdentifier(structure.TableName)} " +
$"SET {Build(structure.UpdateOperations, ", ")} " +
$"WHERE {Build(structure.Predicates)}; " +
$"SELECT {Build(structure.PrimaryKey())} " +
$"FROM {QuoteIdentifier(structure.TableName)} " +
$"WHERE {Build(structure.Predicates)}; ";
", " + updates +
$" WHERE {Build(structure.Predicates)}; " +
$" SET @ROWCOUNT=ROW_COUNT(); " +
$"SELECT " + select + $" WHERE @ROWCOUNT > 0;";
}

/// <inheritdoc />
Expand All @@ -88,8 +99,26 @@ public string Build(SqlDeleteStructure structure)
/// <inheritdoc />
public string Build(SqlUpsertQueryStructure structure)
{
// TODO: these should be put in a transcation
throw new NotImplementedException();
// Create local variables to store the pk columns
string sets = String.Join(";\n", structure.PrimaryKey().Select((x, index) => $"SET {"@LU_" + index.ToString()} := 0"));

// Fetch the value to local variables
string updates = String.Join(", ", structure.PrimaryKey().Select((x, index) =>
$"{QuoteIdentifier(x)} = (SELECT {"@LU_" + index.ToString()} := {QuoteIdentifier(x)})"));

// Select local variables and mapping to original column name
string select = String.Join(", ", structure.PrimaryKey().Select((x, index) => $"{"@LU_" + index.ToString()} AS {QuoteIdentifier(x)}"));

string insert = $"INSERT INTO {QuoteIdentifier(structure.TableName)} ({Build(structure.InsertColumns)}) " +
$"VALUES ({string.Join(", ", (structure.Values))}) ";

return sets + ";\n" +
insert + " ON DUPLICATE KEY " +
$"UPDATE {Build(structure.UpdateOperations, ", ")}" +
$", " + updates + ";" +
$" SET @ROWCOUNT=ROW_COUNT(); " +
$"SELECT " + select + $" WHERE @ROWCOUNT = 2;" +
$"SELECT {MakeUpsertSelections(structure, true)} WHERE @ROWCOUNT = 1;";
}

/// <summary>
Expand Down Expand Up @@ -126,10 +155,44 @@ private string MakeInsertSelections(SqlInsertStructure structure)
{
List<string> selections = new();

List<string> fields = structure.PrimaryKey()
.Union(structure.InsertColumns).ToList();

int index = 0;
foreach (string colName in fields)
{
string quotedColName = QuoteIdentifier(colName);
if (structure.InsertColumns.Contains(colName))
{
selections.Add($"{structure.Values[index]} AS {quotedColName}");
index++;
}
else if (structure.GetColumnDefinition(colName).IsAutoGenerated)
{
//TODO: This assumes one column PK
Copy link
Collaborator

@Aniruddh25 Aniruddh25 Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test for Insert with multi column PKs already :

InsertOneInCompositeKeyTableTest Is that working for you ?

selections.Add($"LAST_INSERT_ID() AS {quotedColName}");
}
}

return string.Join(", ", selections);
}

private string MakeUpsertSelections(SqlUpsertQueryStructure structure, bool includePK)
{
List<string> selections = new();

List<string> fields = structure.AllColumns();

int index = 0;
foreach (string colName in structure.PrimaryKey())
foreach (string colName in fields)
{
string quotedColName = QuoteIdentifier(colName);

if (!includePK && structure.PrimaryKey().Contains(colName))
{
continue;
}

if (structure.InsertColumns.Contains(colName))
{
selections.Add($"{structure.Values[index]} AS {quotedColName}");
Expand All @@ -139,6 +202,10 @@ private string MakeInsertSelections(SqlInsertStructure structure)
{
selections.Add($"LAST_INSERT_ID() AS {quotedColName}");
}
else
{
selections.Add($"NULL AS {quotedColName}");
}
}

return string.Join(", ", selections);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Azure.DataGateway.Service.Models;
using Azure.DataGateway.Services;
using HotChocolate.Language;
Expand Down Expand Up @@ -94,6 +95,14 @@ public List<string> PrimaryKey()
return GetTableDefinition().PrimaryKey;
}

/// <summary>
/// get all columns of the table
/// </summary>
public List<string> AllColumns()
{
return GetTableDefinition().Columns.Select(col => col.Key).ToList();
}

/// <summary>
/// Add parameter to Parameters and return the name associated with it
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ public SqlUpsertQueryStructure(string tableName, IMetadataStoreProvider metadata
}
}

/// <summary>
/// Get the definition of a column by name
/// </summary>
public ColumnDefinition GetColumnDefinition(string columnName)
{
return GetTableDefinition().Columns[columnName];
}

private void PopulateColumns(
IDictionary<string, object> mutationParams,
TableDefinition tableDefinition)
Expand Down
2 changes: 1 addition & 1 deletion DataGateway.Service/appsettings.MySql.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"DatabaseType": "MySql",
"ResolverConfigFile": "sql-config.json",
"DatabaseConnection": {
"ConnectionString": "server=localhost;database=graphql"
"ConnectionString": "server=localhost;database=graphql;Allow User Variables=true;"
}
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"DataGatewayConfig": {
"DatabaseConnection": {
"ConnectionString": "server=localhost;database=datagatewaytest;uid=root;pwd=REPLACEME"
"ConnectionString": "server=localhost;database=datagatewaytest;Allow User Variables=true;uid=root;pwd=REPLACEME"
}
}
}