Skip to content

Conversation

@JelteF
Copy link
Contributor

@JelteF JelteF commented Oct 7, 2021

This is an initial version of the query generation approach proposed in this RFC: https://github.com/Azure/project-hawaii/pull/18

It allows for JOINs to happen automatically.

There's quite some features this does not implement yet, some of
which would have been possible with the previous approach of
hardcoded queries:

  1. Parameters
  2. Selecting a single top row (this pretty much requires parameters)
  3. Putting a LIMIT on rows
  4. ORDER BY primary key
  5. One-To-One relationships
  6. Many-To-Many relationships

All these features are postponed for follow up PRs, so they can easily be worked on
concurrently by different people. The code in this PR provides a good base.

The database schema I've been using to test this can be found in the books.sql file.

A basic GrahpQL query like this works with the code:

query {
    getBooks {
        id
        title
        publisher_id
        publisher {
            id
            name
        }
        reviews {
            id
            content
        }
    }
}

But a crazy query like this also works (but it is pretty slow):

query {
  getBooks {
    id
    title
    publisher_id
    publisher {
      id
      name
      books {
        title
        publisher {
          books {
            publisher {
              books {
                publisher {
                  books {
                    publisher {
                      books {
                        publisher {
                          books {
                            publisher {
                              books {
                                publisher {
                                  books {
                                    publisher {
                                      books {
                                        publisher { 
                                          books {
                                            publisher {
                                              name
                                            }
                                          }
                                        }
                                      }
                                    }
                                  }
                                }
                              }
                            }
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
        
      }
    }
    reviews {
        id
        content
    }
  }
}

@JelteF JelteF force-pushed the sql-query-structure branch from e0f53f5 to d19b84f Compare October 7, 2021 16:04
@JelteF JelteF changed the base branch from main to log-errors October 7, 2021 16:05
@JelteF JelteF force-pushed the sql-query-structure branch from d19b84f to 629594b Compare October 7, 2021 16:06
@jarupatj
Copy link
Contributor

what the query generated for the crazy example that you gave? Can you add tests?

@jarupatj
Copy link
Contributor

jarupatj commented Oct 12, 2021

How many queries will be generated and executed? How do we handle N+1 problem in GraphQL? #Resolved

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.

Did an initial review, need to still look deeper in the SqlQueryStructure and builders.
Have some changes requested till then.

@jarupatj
Copy link
Contributor

Nvm. RFC answered this question.


In reply to: 940620331

fromPart += String.Join(
"",
structure.JoinQueries.Select(
x => $" OUTER APPLY ({Build(x.Value)}) AS {QuoteIdentifier(x.Key)}({structure.DataIdent})"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps, we need to identify the Many:Many relationship, get that from metadata and based on the query fieldName do an INNER JOIN if needed, check the suggestion on the RFC...

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 think this PR is already much bigger than I would like it to be. I'd like to punt on the Many-To-Many code for the next iteration.

@JelteF JelteF force-pushed the log-errors branch 3 times, most recently from 072a716 to 4e272f8 Compare November 8, 2021 17:47
Base automatically changed from log-errors to main November 8, 2021 18:10
result.RootElement.TryGetProperty(context.Selection.Field.Name.Value, out jsonElement);
if (result != null && hasProperty)
{
//TODO: Try to avoid additional deserialization/serialization here.
Copy link
Contributor

Choose a reason for hiding this comment

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

//TODO: Try to avoid additional deserialization/serialization here.

Can we create a work time to track this work?

@JelteF JelteF force-pushed the sql-query-structure branch 3 times, most recently from 3a27271 to 498c74c Compare November 10, 2021 15:41
Aniruddh25 added a commit that referenced this pull request Nov 23, 2021
## 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
- Adds a `RestController` to accept routes matching `https://localhost:5001/users/id/1?_f=id,username` 
where `users` is the `entityName`, `id` is the primary key field name, `1` is the value for the primary key 
`_f` in the query string is the keyword used for the selected field names.
- For composite primary keys, the route would be `https://localhost:5001/users/id/1/partition_key/200?_f=id,username` where `id` and `partition_key` together form the primary key.
- Adds a `RestService` to handle the request by invoking the `RequestParser` to parse the request and populate the `FindQueryStructure` class which holds the major components of the query to be generated
- `MsSqlQueryBuilder` `PostgresQueryBuilder` use the `FindQueryStructure` class to build the required query for the FindById operation.

## Testing
- Tested using PostMan that the route for FindById returns expected Json document when given no fields and also with specific fields.
- Added `MsSqlRestApiTests` to test the same. Moved some common test code to `MsSqlTestBase`.

## Motivation and future thoughts
- This change uses some of the request parsing logic from existing work done here [SqlRestApi](https://msdata.visualstudio.com/DefaultCollection/Database%20Systems/_git/SqlRestApi). In future, for addition of filter clause etc, we can similarly reuse that parsing logic. 
- It is also inspired by the draft PR(#55) for query generation for GraphQL. 
The `FindQueryStructure` is similar to the `SqlQueryStructure` class from that PR(#55).
`FindQueryStructure` could be useful for CosmosDb query generation so the class `SqlQueryStructure` should be derived from `FindQueryStructure`. Once this and the draft PR are merged in, queries for SQL like databases will be autogenerated for both REST and GraphQL.
- When support for CosmosDb query generation #71 is added to the mix, its possible to further redesign the query generation class structure.
- For other REST verbs like POST, PUT, DELETE, we would need similar classes like `FindQueryStructure`, at which point again more abstraction would be needed.
- [Routing](https://docs.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-6.0)

## Issues to be addressed in future
- Schema Validation - verify PrimaryKey matches the one obtained from the schema tracked by issue : #99
- Tested against PostgreSQL but it needs parameter names in the query to be type casted to the right type. This would need reading schema from the database and add the parameter accordingly. This is tracked by issue : #103
- Do we need streaming ? #102
@Aniruddh25
Copy link
Collaborator

Please merge with the FindQueryStructure class I added in the PR #98

@seantleonard
Copy link
Contributor

nit: in the PR description, the sample GraphQL queries have (first:3) . May want to remove for now, so sample queries can be directly tested. Currently, as your PR notes, Putting Limit on rows is not supported yet.

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

:shipit: Thanks for adding additional tests and addressing comments. Just a few more questions added from me and Jarupat, but functionality wise, is working in my few queries used against your provided GQL and SQL schemas and got all the integration tests passing locally.

@jarupatj
Copy link
Contributor

jarupatj commented Dec 3, 2021

        Utf8JsonWriter writer = new(stream, new JsonWriterOptions { Indented = false });

Can you add "using" for both MemoryStream and Utf8JsonWriter. Both of them implements IDisposable


Refers to: DataGateway.Service.Tests/MsSqlTests/MsSqlRestApiTests.cs:153 in 0eae5f1. [](commit_id = 0eae5f1, deletion_comment = False)

@jarupatj
Copy link
Contributor

jarupatj commented Dec 3, 2021

I signed off with some additional comments. I enjoyed reading this code. Thanks!


In reply to: 985188021

Copy link
Contributor

@jarupatj jarupatj left a comment

Choose a reason for hiding this comment

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

:shipit:

@seantleonard
Copy link
Contributor

Note for others: Since Parameters are not currently supported in this PR and will be introduced in a later PR, REST functionality will temporarily be non-functional. REST requests, currently, FindById, depend on parameters, which is not populated during MsSqlQueryBuilder.Build(SqlQueryStructure structure) method.

The query generated from the request:
https://localhost:5001/books/id/2
will look something like:
"SELECT TOP 1 * FROM [books] AS [books] WHERE [books].[id] = @param1"

@JelteF
Copy link
Contributor Author

JelteF commented Dec 6, 2021

Note for others: Since Parameters are not currently supported in this PR and will be introduced in a later PR, REST functionality will temporarily be non-functional. REST requests, currently, FindById, depend on parameters, which is not populated during MsSqlQueryBuilder.Build(SqlQueryStructure structure) method.

@seantleonard Actually REST functionality still works. Parameters are not used during query building but during query execution.

@JelteF JelteF enabled auto-merge (squash) December 6, 2021 09:51
@JelteF JelteF merged commit 05fcab0 into main Dec 6, 2021
@JelteF JelteF deleted the sql-query-structure branch December 6, 2021 09:51
@seantleonard
Copy link
Contributor

Note for others: Since Parameters are not currently supported in this PR and will be introduced in a later PR, REST functionality will temporarily be non-functional. REST requests, currently, FindById, depend on parameters, which is not populated during MsSqlQueryBuilder.Build(SqlQueryStructure structure) method.

@seantleonard Actually REST functionality still works. Parameters are not used during query building but during query execution.

My bad. Yes I got REST working. For some reason, though the errors were because of non filled in parameters, and only saw the true error (SQL login issue in connection string) from BananaCakePop interface. I can look into better logging errors like login issues into the console for REST side of code.

public string PredicatesSql()
{
if (Predicates.Count() == 0)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This piece makes sense now. This is to accommodate dynamically building query and all conditions can be added like and X ?

https://stackoverflow.com/questions/242822/why-would-someone-use-where-1-1-and-conditions-in-a-sql-clause

@Aniruddh25 Aniruddh25 linked an issue Jan 12, 2022 that may be closed by this pull request
abhishekkumams added a commit that referenced this pull request Jul 25, 2022
abhishekkumams added a commit that referenced this pull request Aug 3, 2022
* Initial commit

* added first design proposal

* CODE_OF_CONDUCT.md committed

* LICENSE committed

* README.md updated to template

* SECURITY.md committed

* SUPPORT.md committed

* new console app

* initial config generator

* updated implementation proposal

* Update README.md

* adding .gitignore

* updating gitignore

* removing compiled files

* added support for command-add

* updating gitignore

* cleaning files and adding todos

* Update README.md

* updated file path for config generation

* using classes of hawaii-gql

* adding design_doc and added update command

* removing temp file

* updating readme

* updating readme

* removing temp files

* removing temp files

* updating README

* removing temp files

* fixing the update command

* removing temp files

* Added relationship compatibility

* updated ReadMe

* removing temp files

* updating gitignore

* update README.md (#5)

* Update README.md (#8)

* Update README.md

* Update README.md

* Fix for connection-string issue (#9)

* fix connection-string issue

* removing temp files

* removing resolver config as it is specific to cosmosDB

* updating .gitignore

* updating .gitignore

* Dev/abhishekkuma/code refactoring (#16)

* fix connection-string issue

* removing temp files

* Use Action class from Runtime Config

* removing temp files

* minor fix

* code refactoring

* added summary for all the methods

* making mappings.field optional (#13)

* Update default host global settings (#19)

* updated default host global settings

* updating default value of Cors.Origin

* fixing spelling errors

* Add support for linking object in relationship (#22)

* adding support for linking object in relationship

* updating validation checks

* fixing grammatical errors

* Add CODEOWNERS file (#24)

* added CODEOWNERS file

* updating names

* adding Davide as code owner

* Update version to 0.0.2 (#25)

* updating version to 0.0.2

* fix indentation

* Add commands in help window (#27)

* added commands in help window

* updated help screen

* updated help screen

* fixing typo

* Update README.md (#26)

* Update README.md

Updating Install Information in Readme.

* Update README.md

Co-authored-by: Aniruddh Munde <[email protected]>

Co-authored-by: Aniruddh Munde <[email protected]>

* Updated Typos (#30)

* Quick fix for typo (#31)

* Add editorconfig to auto format code (#35)

* Add editorconfig

* Fix formatting warning

* Fix more warning

* use is null

* Update CommandLineOptions property to start with capital letter.

* Add skeleton tests project (#36)

* Add tests project

* Update gitignore

* fixing update command for permissions (#34)

* Refactoring commandline handling options. (#37)

**What are the changes**
This change refactors the code to use Verb attribute https://github.com/commandlineparser/commandline/wiki/Verbs
to link command line options to command line operation.

The main change are in CommandLineOptions.cs. With this change, we can use the default help print from Commandline library. We do not need to write our own help text.
Remove Operations.cs and Commandlinehelp.cs. I fold the functionality to program.cs and configgenerator.cs.

Make options class immutable. https://github.com/commandlineparser/commandline/wiki/Immutable-Options-Type

Make database type and host option explicit type from Enum instead of string.

**Test**
Manual testing. I don't change how the config is generated. Same variables are passed to config generator.

* Update cosmosDB options (#29)

* updated cosmosDB options

* fix formatting

* making code more modular

* Update src/Models/CommandLineOptions.cs

Co-authored-by: Aniruddh Munde <[email protected]>

* updating test name

* refactoring code

* refactoring code

Co-authored-by: Aniruddh Munde <[email protected]>

* Move relationship related options to update. (#46)

* Update README.md (#51)

* Refactor config generator for Init and Add command (#41)

* Add tests for initConfig and addEntity. Refactor code a bit to make it more readable.

* Add comments.

* More verbose with arg passing.

* Format code.

* Add more tests for AddEntity.

* resolving comments

* removing temp changes

* small fix

Co-authored-by: Abhishek Kumar <[email protected]>

* fixed tests (#55)

* Update README.md (#59)

updating commands from --permission to --permissions

* Simplify Update Entity logic (#52)

* replace name to config

* symplifying update logic

* adding validations

* added more tests

* resolved comments

* adding test description

* updating file names for Add and Update EntityTests

* adding comments and tests

* adding class comments

* adding class comments

* adding more tests and comments

* updated accessebiity of some methods

* updating tests

* adding TODO for policy support

* updating few comments

* updated function summary

* minor change

* fixed comments

* refactoring changes

* using csproj instead of dll files (#62)

* Create CI pipeline (#56)

* Create main.yml

* Update main.yml

* adding Datagateway.Config.csproj

* fixing path

* fixing tests

* add tests validation in pipeline

* adding formatting check

* updating pipeline

* removing temp changes

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* updating pipeline

* updating pipeline files

* updating pipeline files

* updating pipeline

* updating pipeline

* updating pipeline

* updating pipeline

* fix pipeline

* fix pipeline

* fix pipeline

* fix pipeline

* fix pipeline

* updating runner

* updating runner

* adding pipeline for ubuntu

* fixing build job name

* using matrix for running multiple os

* updating .editorconfig

* publishing code coverage

* updating coverage directory

* getting dir info

* fixing pipeline

* updating test script

* fixing code coverage path

* fixing code coverage path

* fixing code coverage issue

* fixing code coverage issue

* fixing code coverage issue

* adding deploy steps for the nuget package

* adding deploy steps for the nuget package

* add source path for nuget package

* add source path for nuget package

* fix source path for nuget package

* fix test

* updating format check

* updating format check

* updating format check

* updating format check

* updating format check

* updating format check

* updating format check

* updating format check

* testing submodules

* fixing yaml

* fixing yaml

* fixing yaml

* fixing yaml

* updating .gitmodules

* updating .gitmodules

* updating .gitmodules

* checking submodules

* checking submodules

* checking submodules

* checking submodules

* checking submodules

* adding submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* testing submodules

* removing temp files

* updating yaml

* updating yaml

* updating tests

* updating tests

* testing workflow_dispatch

* testing workflow_dispatch

* refactoring changes

* testing coverage report to pr

* testing coverage report to pr

* testing coverage report to pr

* testing coverage report to pr

* testing coverage report to pr

* testing coverage report to pr

* testing coverage report to pr

* fix coverage report to pr

* fix coverage report to pr

* fix coverage report to pr

* testing different test reported

* testing different test reported

* testing coverage summary

* fix pipeline

* updating comments

* fixing coverage report

* removing temp files

* removing submodules coverage

* removing unused import

* fixing pipeline

* cleaning comments

* Add support for custom graphql singular plural types (#65)

* adding support for custom graphql singular plural types

* fixed formatting

* fixing pipeline

* updating comments

* removing redundant steps in pipeline

* Adding test for Case Sensitive Entity Name (#64)

* Adds test for entity creation

* Adds change to create the options differently

* Adding change to summary and comments

* Removing jwt from the config json strings

* Changing the default rest api endpoint

* Builds config differently to avoid duplication

* Fixing formatting issues

* Preventing users from creation Permissions with Invalid Actions (#70)

* adding tests

* simplyfying code

* added a new test

* simplifying code

* Changing true/false to be treated as a Boolean when used with --rest and --graphql options  (#74)

* Making true/false bool in --rest/graphql options

* addressing review comments

* checking in latest hawaii-engine commits

* addressing review comments

* adding a check for serialization of entity.graphql value

* Add support for Policy in Permission Actions (#71)

* added support for policy

* fix formatting

* added tests

* fix build failure

* fix formatting

* adding comments

* removed redundant code

* updating tests

* updating authentication provider

* updating tests

* updating editorconfig

* updating submodules

* updating submodules

* fixing some dependencies

* Add support for mappings (#76)

* adding test

* fix formatting

* fix error

* fix formatting

* updating option description

* updating comment

* fixing build

* Add support for cors origin (#77)

* added support for cors-origin

* fix formatting

* fix test

* fix formatting

* Prevent user from adding relationship in cosmos db (#79)

* preventing user from creating relationship in cosmosdb

* updating test

* Using IEnumerables for Options (#80)

* adding changes for using ienumerables for options

* removes redundant conversion

* Adding checks for empty items

* fixing console output messages

* adding empty checks for ienumerable options in addition to null

* checking in hawaii-engine sub-module

* Checking in changes from engine

* fixes formatting

* adding test for update command with multiple mapping fields

* fix formatting

* removing extra spaces

* incorporating PR review suggestions

* checnking-in latest changes from hawaii engine

* adding changes to remove empty mappings property in config json

* minor correction in the test command

* adding a test to validate the config json generated

* minor correction in the test

* New Release 0.0.4 (#84)

* Fix serialization issue with special character (#87)

* fixed serialization issue with special charachter

* updating test

* fix formatting

* removing temp changes

* fix test

* updating test

* updating test

* updating test summary

* fix pipeline

* Renaming the option from --mapping.fields to --relationship.fields (#86)

* renaming command option from mapping.fields to relationship.fields

* changing sample commadns in README fiel

* adding latest changes from hawaii engine

* adding latest changes from engine

* updating tests to use relationship.fields

* merge cli into engine

* updating csproj file

* removing cli sln file

* removing duplicate files

* fix formatting

* fix formatting

* adding cli tests into build pipeline

* fix format

* updating folder name for cli-tests

* updating csproj filename for hawaii-cli

* updating csproj filename for hawaii-cli

* fixing build

* fix format

* updating pipeline to create nuget package

* publish nuget packe to artifact

* fix build pipeline

* testing

* testing

* testing

* fixing path for exe

* fixing path for exe

* updating feed for nuget package

* commenting nuget push

* using dependency version from Directory.Build.Props

* testing nuget push

* fixing publish of nuget package

* Update CLI to use Operation Enum.

* Fix build break in tests.

* Verify that operation is valid for action.

* Remove unnessary using.

* updating pipeline

* updating nuget command in pipeline

* updating nuget command in pipeline

* updating nuget command in pipeline

* updating nuget command in pipeline

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg creation

* testing nupkg publishing

* updating pipeline for nuget push

* fixing pipeline

* fixing pipeline

* fixing pipeline

* fixing pipeline

* fixing pipeline

* fixing pipeline

* fixing pipeline

* fixing pipeline

* fix formatting

* fix formatting

Co-authored-by: Microsoft Open Source <[email protected]>
Co-authored-by: Sajeetharan <[email protected]>
Co-authored-by: Aniruddh Munde <[email protected]>
Co-authored-by: Chris LaFreniere <[email protected]>
Co-authored-by: Jarupat Jisarojito <[email protected]>
Co-authored-by: Shyam Sundar J <[email protected]>
Co-authored-by: Shyam Sundar J <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MsSql/PgSql: Automatic building of MsSql or PgSql query for resolvers

5 participants