Updated GraphQL Nested Filter integration tests and GraphQL filter processing#1407
Merged
seantleonard merged 13 commits intomainfrom Apr 5, 2023
Merged
Conversation
…nested filter processing to handle normal filters. updated test scenarios and config for mssql.
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
…tamping on cosmosquerystructure to align with new checks in filter parser.Updated cosmos test config to align with config generator script
… has accurate account of entityname even in nesting
Aniruddh25
reviewed
Apr 4, 2023
Aniruddh25
reviewed
Apr 4, 2023
…o longer needed after later changes were made. Updates constant referring to error message to not have nested ref.
Aniruddh25
approved these changes
Apr 4, 2023
Collaborator
Aniruddh25
left a comment
There was a problem hiding this comment.
LGTM, thanks for adding tests and improving our GraphQL filtering processing!
abhishekkumams
approved these changes
Apr 5, 2023
tarazou9
added a commit
that referenced
this pull request
Apr 13, 2023
## Why make this change? - Closes this issue referenced here #1423. Additional issue related to this change #597 - Cosmos DB currently doesn't support field level authorization, it's expected that we are not honoring ```operationToColumnMap.Excluded``` and ```operationToColumnMap.Included``` and by introducing field level auth check into the ```GQLFilterParser``` [recent update](#1407) results the issue that we are seeing. - Cosmos have tests to exercise the filter parser, the reason the tests didn't catch this issue is because the changes here [added lines](https://github.com/Azure/data-api-builder/blob/1431ffc8c11bcd80f62d51c0c0fd6f15383b147a/src/Service.Tests/CosmosTests/TestBase.cs#L77-L82) that's added in this PR [recent update](#1407), this mocks up the field auth check to always return true for all Cosmos filter tests. - ```CosmosMetadataProvider``` does not translate a field wild card (*) or list of hardcoded include columns to the list of AllowedFields in the engine's ```AuthorizationResolver``` currently, but we can add a pass through to the ```TryGetExposedColumnName``` as suggested so it doesn't throw a ```NotImplementedException``` when user accidently passed in the fields settings in the Cosmos configuration file. ## What is this change? - Address this by skipping the field auth check inside of the GQLFilterParser when the database type is Cosmos ## How was this tested? - [ x ] Integration Tests - [ x ] Unit Tests ## Sample Request(s) ```json query planets { planets (filter: {id: {eq: 1000}}) { items { id } } } ```
tarazou9
added a commit
that referenced
this pull request
Apr 25, 2023
## Why make this change? - Closes this issue referenced here #1423. Additional issue related to this change #597 - Cosmos DB currently doesn't support field level authorization, it's expected that we are not honoring ```operationToColumnMap.Excluded``` and ```operationToColumnMap.Included``` and by introducing field level auth check into the ```GQLFilterParser``` [recent update](#1407) results the issue that we are seeing. - Cosmos have tests to exercise the filter parser, the reason the tests didn't catch this issue is because the changes here [added lines](https://github.com/Azure/data-api-builder/blob/1431ffc8c11bcd80f62d51c0c0fd6f15383b147a/src/Service.Tests/CosmosTests/TestBase.cs#L77-L82) that's added in this PR [recent update](#1407), this mocks up the field auth check to always return true for all Cosmos filter tests. - ```CosmosMetadataProvider``` does not translate a field wild card (*) or list of hardcoded include columns to the list of AllowedFields in the engine's ```AuthorizationResolver``` currently, but we can add a pass through to the ```TryGetExposedColumnName``` as suggested so it doesn't throw a ```NotImplementedException``` when user accidently passed in the fields settings in the Cosmos configuration file. ## What is this change? - Address this by skipping the field auth check inside of the GQLFilterParser when the database type is Cosmos ## How was this tested? - [ x ] Integration Tests - [ x ] Unit Tests ## Sample Request(s) ```json query planets { planets (filter: {id: {eq: 1000}}) { items { id } } } ```
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
What is this change?
@relationshipdirective metadata in addition to a reference to the graphql object name.How was this tested?