fix: Determine if query is introspection query #1255

islamaliev · 2023-03-29T15:17:29Z

Relevant issue(s)

Resolves #911

Description

This PR removes hard-coded string search for "IntrospectionQuery" and replaces it with a method that checks it's content.
It also adjusts all the tests.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Integration tests

Specify the platform(s) on which this was tested:

MacOS

codecov · 2023-03-29T15:21:37Z

Codecov Report

Merging #1255 (f95afa4) into develop (706e55d) will increase coverage by 0.08%.
The diff coverage is 88.37%.

@@             Coverage Diff             @@
##           develop    #1255      +/-   ##
===========================================
+ Coverage    70.12%   70.20%   +0.08%     
===========================================
  Files          183      184       +1     
  Lines        17373    17394      +21     
===========================================
+ Hits         12182    12212      +30     
+ Misses        4253     4247       -6     
+ Partials       938      935       -3

Impacted Files	Coverage Δ
db/subscriptions.go	`74.28% <0.00%> (ø)`
request/graphql/parser/introspection.go	`73.33% <73.33%> (ø)`
db/request.go	`84.61% <100.00%> (-3.96%)`	⬇️
request/graphql/parser.go	`87.50% <100.00%> (+4.64%)`	⬆️
request/graphql/parser/request.go	`72.43% <100.00%> (ø)`

... and 3 files with indirect coverage changes

fredcarle

LGTM. I assume this will help our support of GraphQL clients.

fredcarle · 2023-03-29T16:20:04Z

db/request.go

@@ -37,7 +35,7 @@ func (db *db) execRequest(ctx context.Context, request string, txn datastore.Txn
 		return res
 	}

-	pub, subRequest, err := db.checkForClientSubsciptions(parsedRequest)
+	pub, subRequest, err := db.checkForClientSubscriptions(parsedRequest)


praise: Thanks for catching this typo!

fredcarle · 2023-03-29T16:21:36Z

db/subscriptions.go

@@ -39,7 +39,7 @@ func (db *db) checkForClientSubsciptions(r *request.Request) (
 			return pub, subRequest, nil
 		}

-		return nil, nil, client.NewErrUnexpectedType[request.ObjectSubscription]("SubcriptionSelection", s)
+		return nil, nil, client.NewErrUnexpectedType[request.ObjectSubscription]("SubscriptionSelection", s)


praise: Again for catching the typo. That's on me... Subscription is my most error prone word haha!

AndrewSisley

The code looks good, but I think the solution needs discussion as I think it is breaking the GQL spec in a different way IMO that will just need to be redone sometime soon anyway.

AndrewSisley · 2023-03-29T16:39:47Z

request/graphql/parser/introspection.go

+			for _, selection := range astOpDef.SelectionSet.Selections {
+				switch node := selection.(type) {
+				case *ast.Field:
+					if node.Name.Value == "__schema" || node.Name.Value == "__type" {


discussion: I think we need to rethink the whole introspection flow, I believe these are valid GQL elements within a 'normal' query (like __typename, which we do currently support).

In the short-term it might still be work checking the name (or whichever prop it is) equals "IntrospectionQuery" - checking for __schema/__type could incorrectly flag normal queries as introspection only and cause them to miss the main query code blocks (i.e. planner)

I think those values are specifically for introspection: https://graphql.org/learn/introspection

They wouldn't be used on a normal query.

They are valid within a normal query though, pretty sure you are free to use them within any part of any query

A normal query with sub introspection queries?

I might be wrong actually - they are root only. They can however be used alongside normal query/mutations within the same request (top-level) only, which we dont yet support and this solution doesnt cause problems for that.

@orpheuslummis had a chat with me a month or so about __schema/__type when looking at the gql client problem - he might know an example?

By my read of the spec and of the graphql ecosystem.

This looks valid.

query { users { name } __type (name: "users") { name } }

While this doesn't look valid.

query { users { name __type (name: "users") { name } } }

I don't have an example of the later.
The first is compliant and required. Implementing the later could be perhaps be compliant and useful, but is not necessary for compliance (in my understanding.

supporting introspection queries along with normal ones is not supported yet.
I think we should merge this one as is, because it's a good step forward and IMO should be in 0.5.0.
And create another task to support different types of queries.
What do you think @AndrewSisley?

I agree in that we shouldn't try to support multiple request operations in a single request. We didn't support it prior to this PR so I see no reason to accommodate our now.

Should be done in another pr, but it's fairly low priority ATM.

supporting introspection queries along with normal ones is not supported yet.

No, but I was concerned that we are putting effort into a solution that doesnt help us in the long term, by continuing to treat introspection elements as a 'special other', when in reality they can/might be embedded anywhere within a request block.

Looking for __schema/__type elements is better than string contains, but it is applying more effort into what I think might be the wrong direction (treating introspection as a special 'other' that doesn't get fed into the planner).

Sounds like this concern is not shared (or at least not thought of as a blocker) though, so crack on and merge once everything else is sorted :)

I don't see this hindering or blocking the ability to do multiple operations per request. If anything, Islams approach to actually using the AST here opens the door to "splitting" the AST into planner and introspection requests, executing them independently, and merging the final results.

I can't necessarily think of a reason why you would ever want/need to run both a introspection and a regular query tho 🙃.

jsimnz

Issue regarding duplicating ast parsing which is (relatively) expensive.

jsimnz · 2023-03-30T01:17:44Z

db/request.go

@@ -22,8 +21,7 @@ import (
 // execRequest executes a request against the database.
 func (db *db) execRequest(ctx context.Context, request string, txn datastore.Txn) *client.RequestResult {
 	res := &client.RequestResult{}
-	// check if its Introspection request
-	if strings.Contains(request, "IntrospectionQuery") {
+	if db.parser.IsIntrospection(request) {


todo: One notable problem currently is you are duplicating the buildAST call if it isn't an introspection request. Which is a somewhat expensive operation in the grand scheme of the request execution.

Can we find a way to call it once, perhaps exposing the BuildAST and calling it within execRequest and passing the built/parsed AST to all the other places we currently pass the raw request to.

Alternatively Parse could return both the built AST and the parsed request. Not sure which is better.

I was going to flag this, benched it, and changed my mind - the code in buildAST only took 17 microseconds on first run locally of the query in TestQueryOneToOne. It is a bit ugly, but IMO it isnt really worth worrying over just yet (this is quite localized code too)?

It is quite hard to give a concrete answer to this, but at what kind of cost would you (John) currently draw the line on performance stuff?

I agree with John. I wanted myself to extract this method but held back because I wasn't sure I should change the interface.

as discussed

Parse query content query to determine introspection Remove hard-coded IntrospectionQuery from tests

islamaliev added feature New feature or request action/no-benchmark Skips the action that runs the benchmark. labels Mar 29, 2023

islamaliev added this to the DefraDB v0.5 milestone Mar 29, 2023

islamaliev requested review from orpheuslummis, jsimnz and AndrewSisley March 29, 2023 15:17

islamaliev self-assigned this Mar 29, 2023

fredcarle approved these changes Mar 29, 2023

View reviewed changes

AndrewSisley previously requested changes Mar 29, 2023

View reviewed changes

jsimnz requested changes Mar 30, 2023

View reviewed changes

islamaliev changed the title ~~feat: determine if query is introspection query~~ feat: Determine if query is introspection query Mar 30, 2023

islamaliev requested review from jsimnz and AndrewSisley March 30, 2023 11:22

islamaliev added 6 commits March 30, 2023 14:14

Parse query content query to determine introspection

5ada3a1

Remove hard-coded IntrospectionQuery from tests

ec78626

Polish

8121250

Expose BuildRequestAST method

28e6ba8

Adjust benchmark tests

83af8fc

Use consts

f95afa4

islamaliev force-pushed the islam/feat/I911-determine-introspection-query branch from cedb3f4 to f95afa4 Compare March 30, 2023 12:14

jsimnz changed the title ~~feat: Determine if query is introspection query~~ fix: Determine if query is introspection query Mar 30, 2023

jsimnz added bug Something isn't working and removed feature New feature or request labels Mar 30, 2023

jsimnz approved these changes Mar 30, 2023

View reviewed changes

islamaliev merged commit bce78a1 into develop Mar 30, 2023

islamaliev deleted the islam/feat/I911-determine-introspection-query branch March 30, 2023 21:47

shahzadlone pushed a commit that referenced this pull request Apr 13, 2023

fix: Determine if query is introspection query (#1255)

0023556

Parse query content query to determine introspection Remove hard-coded IntrospectionQuery from tests

orpheuslummis mentioned this pull request May 6, 2023

Failure upon example introspection query #1151

Closed

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024

fix: Determine if query is introspection query (sourcenetwork#1255)

b37a544

Parse query content query to determine introspection Remove hard-coded IntrospectionQuery from tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Determine if query is introspection query #1255

fix: Determine if query is introspection query #1255

islamaliev commented Mar 29, 2023 •

edited

Loading

codecov bot commented Mar 29, 2023 •

edited

Loading

fredcarle left a comment

fredcarle Mar 29, 2023

fredcarle Mar 29, 2023

AndrewSisley left a comment

AndrewSisley Mar 29, 2023

fredcarle Mar 29, 2023

AndrewSisley Mar 29, 2023

fredcarle Mar 29, 2023

AndrewSisley Mar 29, 2023

orpheuslummis Mar 29, 2023

islamaliev Mar 30, 2023

jsimnz Mar 30, 2023

AndrewSisley Mar 30, 2023

jsimnz Mar 30, 2023

jsimnz left a comment

jsimnz Mar 30, 2023

AndrewSisley Mar 30, 2023

islamaliev Mar 30, 2023

fix: Determine if query is introspection query #1255

fix: Determine if query is introspection query #1255

Conversation

islamaliev commented Mar 29, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Mar 29, 2023 • edited Loading

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev commented Mar 29, 2023 •

edited

Loading

codecov bot commented Mar 29, 2023 •

edited

Loading