-
Notifications
You must be signed in to change notification settings - Fork 291
Implement the GET verb for FindById operation for MsSql #98
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
Conversation
| } | ||
|
|
||
| services.AddSingleton<GraphQLService, GraphQLService>(); | ||
| services.AddSingleton<RestService, RestService>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats a good point to discuss - we need to have both GraphQL and REST active at the same time.
I tested localhost:5001/graphql alongwith the REST GET and this continues to the graphql route..
That said, it might collide for other REST verbs. We need to make sure the routes don't collide.
One option would be to modify the route to be localhost:5001/databasename/entityName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested localhost:5001/graphql after creating a table named graphql but the GET would return 404 not found proving the GraphQL controller route took precedence. I think we shouldn't be allowing tables with 'graphql' as their names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm as opposed to adding rules to what devs have in their own databases, localhost:5001/api/databasename/entityName would also mitigate us looking into ASP.NET route restrictions , as we still want to expose localhost:5001/graphql no matter what for GraphQL requests. another reason to be open to adding /api would be for Runtime endpoint versioning. i.e. localhost:5001/api/v1.0/entity can bring this up in future Hawaii PM sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely in favour of prefixing all REST requests with some well known path. I think /api/v1.0/ would indeed be a good choice because it allows for versioning. Another way to do versioning would be to use a version=v1.0 query parameter.
mbhaskar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the changes.
For cosmos, the primary key is combination of id + partition key. Without partition key, findById could return various documents that match the id but differ in partitionkey.
@moderakh We should probably document about partition key header x-ms-documentdb-partitionkey
…ns project using Preserve Newest
seantleonard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting this in and addressing PR comments. Very helpful and looking forward to using this to progress further on more REST cases!
|
|
||
| for (int primaryKeyIndex = 0; primaryKeyIndex < primaryKeyValues.Length; primaryKeyIndex += 2) | ||
| { | ||
| queryStructure.Conditions.Add($"{primaryKeyValues[primaryKeyIndex]} = @{primaryKeyValues[primaryKeyIndex]}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for SQL injection if the URL is provided is something like localhost:5000/users/id; DROP TABLE users/2
Summary
Alongwith graphql, DataGateway Service should support REST api requests too. This change introduces the necessary classes that help add REST support and implements the GET verb for the FindById operation for MsSql only.
How
RestControllerto accept routes matchinghttps://localhost:5001/users/id/1?_f=id,usernamewhere
usersis theentityName,idis the primary key field name,1is the value for the primary key_fin the query string is the keyword used for the selected field names.https://localhost:5001/users/id/1/partition_key/200?_f=id,usernamewhereidandpartition_keytogether form the primary key.RestServiceto handle the request by invoking theRequestParserto parse the request and populate theFindQueryStructureclass which holds the major components of the query to be generatedMsSqlQueryBuilderPostgresQueryBuilderuse theFindQueryStructureclass to build the required query for the FindById operation.Testing
MsSqlRestApiTeststo test the same. Moved some common test code toMsSqlTestBase.Motivation and future thoughts
The
FindQueryStructureis similar to theSqlQueryStructureclass from that PR(Initial version of query generation for Postgres and MS SQL #55).FindQueryStructurecould be useful for CosmosDb query generation so the classSqlQueryStructureshould be derived fromFindQueryStructure. Once this and the draft PR are merged in, queries for SQL like databases will be autogenerated for both REST and GraphQL.FindQueryStructure, at which point again more abstraction would be needed.Issues to be addressed in future