Skip to content

Add MySQL RESTAPI POST/PUT support and Tests #260

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Feb 26, 2022
Merged

Add MySQL RESTAPI POST/PUT support and Tests #260

merged 8 commits into from
Feb 26, 2022

Conversation

@junsu0ms
Copy link
Contributor

@junsu0ms junsu0ms commented Feb 24, 2022 •
edited
Loading

@junsu0ms junsu0ms changed the title Use a different way to get the updated rowid Add MySQL RESTAPI Tests Feb 24, 2022
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
DataGateway.Service.Tests/SqlTests/MySqlRestApiTests.cs

[TestMethod]
[Ignore]
public override Task InsertOneInCompositeKeyTableTest()
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.

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

Copy link
Contributor Author

@junsu0ms junsu0ms Feb 25, 2022

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 •
edited
Loading

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

Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
DataGateway.Service.Tests/SqlTests/MySqlRestApiTests.cs Outdated
public override Task PutOne_Insert_PKAutoGen_Test()
{
throw new NotImplementedException();
throw new NotImplementedException("error: Fail, still able to insert");
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.

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

Copy link
Contributor Author

@junsu0ms junsu0ms Feb 25, 2022

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 •
edited
Loading

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

@junsu0ms junsu0ms Feb 26, 2022

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?

Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
DataGateway.Service/Resolvers/MySqlQueryBuilder.cs
}
else if (structure.GetColumnDefinition(colName).IsAutoGenerated)
{
//TODO: This assumes one column PK
Copy link
Collaborator

@Aniruddh25 Aniruddh25 Feb 25, 2022 •
edited
Loading

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 ?

Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Feb 25, 2022
View reviewed changes
Aniruddh25
Aniruddh25 requested changes Feb 25, 2022
View reviewed changes
Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Thanks for this quick PR. Have some questions and suggestions.
Could you also please update the PR description to say it adds support for MySQL REST POST/PUT

@junsu0ms junsu0ms requested a review from Aniruddh25 February 25, 2022 09:48
@junsu0ms junsu0ms force-pushed the personal/junsu/rowid branch from 6813442 to 88b24cf Compare February 25, 2022 09:52
@junsu0ms junsu0ms force-pushed the personal/junsu/rowid branch from 88b24cf to 2a75ca0 Compare February 25, 2022 09:57
@Aniruddh25 Aniruddh25 changed the title Add MySQL RESTAPI Tests Add MySQL RESTAPI POST/PUT support and Tests Feb 26, 2022
Aniruddh25
Aniruddh25 approved these changes Feb 26, 2022
View reviewed changes
Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you again for the quick implementation.
Lets tackle the PutOne_Insert_PKAutoGen failure in a next PR...

@junsu0ms junsu0ms merged commit 33972a4 into main Feb 26, 2022
@junsu0ms junsu0ms deleted the personal/junsu/rowid branch February 26, 2022 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Aniruddh25 Aniruddh25 Aniruddh25 approved these changes

@mbhaskar mbhaskar Awaiting requested review from mbhaskar

@JelteF JelteF Awaiting requested review from JelteF

@seantleonard seantleonard Awaiting requested review from seantleonard

@jarupatj jarupatj Awaiting requested review from jarupatj

@Mathos1432 Mathos1432 Awaiting requested review from Mathos1432

@gledis69 gledis69 Awaiting requested review from gledis69

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@simplynaveen20 simplynaveen20 Awaiting requested review from simplynaveen20

@milismsft milismsft Awaiting requested review from milismsft

@swviswan swviswan Awaiting requested review from swviswan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MySQL: HTTP POST/PUT Request Implementation Changes

4 participants

@junsu0ms @Aniruddh25 @junsu-msft