refactor: Decouple db.db from gql #912
Conversation
AndrewSisley
added
area/query
Codecov Report
@@ Coverage Diff @@
## develop #912 +/- ##
===========================================
- Coverage 59.84% 59.80% -0.04%
===========================================
Files 159 159
Lines 17335 17335
===========================================
- Hits 10374 10368 -6
- Misses 6036 6041 +5
- Partials 925 926 +1
|
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AndrewSisley
added
the
action/no-benchmark
There was a problem hiding this comment.
Mostly looks good, sorry I think I got carried away with the Request and Query terminology suggestions, specially since we have introduced the request terminology more throughout the code. Feel free to ignore some of those as I think some of my comments might be out of scope, but would be nice to take care of some of those if they are fresh in your head at the moment.
| _, err := exec.ParseRequestString(query) | ||
| if err != nil { | ||
| return errors.Wrap("failed to parse query string", err) | ||
| _, errs := parser.Parse(query) |
There was a problem hiding this comment.
todo: Rename Parse to ParseRequest?
| _, errs := parser.Parse(query) | |
| _, errs := parser.ParseRequest(query) |
There was a problem hiding this comment.
I prefer Parse, as I dont think the suffix Request adds anything here, as well as being shorter and Request being covered by the type info (which shouldn't IMO be baked into the name unless avoiding collisions)
There was a problem hiding this comment.
I prefer ParseRequest as previously it was ParseRequestString we also have two other methods on the interface called:
IsIntrospectionRequest(request string)ExecuteIntrospectionRequest(request string)
I think it's clearer to have Request suffix, as right now the input variable is named query and can cause a confusion as well until we sort out the terminology properly.
There was a problem hiding this comment.
I really doubt anyone will confuse Parse with IsIntrospectionRequest or ExecuteIntrospectionRequest, leaving as is unless someone else chimes in here.
There was a problem hiding this comment.
I really doubt anyone will confuse
ParsewithIsIntrospectionRequestorExecuteIntrospectionRequest, leaving as is unless someone else chimes in here.
I am not saying Parse can be confused with them, I meant that the Request suffix is already being used.
There was a problem hiding this comment.
Ah... If all the functions on an interface have the same suffix, the suffix should probably be dropped from all of them as it adds nothing
There was a problem hiding this comment.
Ah... If all the functions on an interface have the same suffix, the suffix should probably be dropped from all of them as it adds nothing
I agree with that. The parser package's purpose is to handle requests. Adding the Request suffix doesn't improve clarity. I support dropping the suffix.
There was a problem hiding this comment.
- Drop
Requestsuffix from interface params (if they survive other discussion-threads)
There was a problem hiding this comment.
👍Happy with dropping suffixes from all names, for sake of consistency.
AndrewSisley
force-pushed
the
sisley/refactor/I905-decouple-from-gql-2
branch
from
9582392 to
e398c10
Compare
|
Question: wondering from the commit title ( |
Ops, I am referring to the type |
AndrewSisley
changed the title
Thanks, Cheers. |
AndrewSisley
force-pushed
the
sisley/refactor/I905-decouple-from-gql-2
branch
from
95b348f to
2ffd5f3
Compare
| type Schema struct { | ||
| // The individual declarations of types defined by this schema. | ||
| Definitions []SchemaDefinition | ||
|
|
||
| // The collection descriptions created from/defined by this schema. | ||
| Descriptions []client.CollectionDescription | ||
| } |
There was a problem hiding this comment.
thought: It seems weird to me that a Schema can have multiple definitions and descriptions. Maybe it's because I'm missing something.
There was a problem hiding this comment.
They were defined and iterated over separately before I made the change, I think I mostly just needed/wanted a single return type instead of returning two params
There was a problem hiding this comment.
maybe then it would be appropriate to name it Schemas?
There was a problem hiding this comment.
A single Schema supports multiple collections - this struct is a single Schema. SchemaDefinition and Definitions are a bit poorly named, but they are the old names and I want to avoid fixing everything in this PR. If any suggestions pop up I'll probably change these though if I'm about anyway.
There was a problem hiding this comment.
Agreed, they can be refactored in a future PR. Was actually thinking about just this the other day when working through some other stuff.
There was a problem hiding this comment.
I'll create a ticket and sort this out pretty soon after merge, it is important - as per discord discussion, but it is localized and not worth blocking the rest of the changes from going in
- create ticket once other convos resolved
|
|
||
| // Parser represents the object responsible for handling stuff specific to | ||
| // a query language. This includes schema and query parsing, and introspection. | ||
| type Parser interface { |
There was a problem hiding this comment.
question: Can you explain why you decided to put this interface in the core package? Ideally, interfaces are defined in the package where they are used. Since it's used in the db package, why not define it there?
There was a problem hiding this comment.
Ideally, interfaces are defined in the package where they are used
This is very much a different mind set to most other langs IMO :) I can for sure see the merit with the way Golangs magic/inferred interfaces work though. I'll revisit this once the other structural threads have been resolved (in case I would otherwise end up changing this multiple times)
- [ ] Move Parser interface to db package?
Update: Leaving as is as mentioned deeper in the convo
There was a problem hiding this comment.
I know it's different :) I may sound like a broken record at this point cuz I say this often, but I think it's important to follow the language recommended best practices as much as possible regardless of our habits with other langs. Bringing other cool pattern as additional gravy is fine though if it makes sense (i.e. Option type).
I agree with waiting on the other threads to be resolved.
There was a problem hiding this comment.
Ah :) I wasn't arguing with you one this one, as it is more a structural thing than just a styling thing - it'll probably be moved if it survives
Similar to the (positive) mindset shift RE pointers/ownership etc I experienced when learning Rust
There was a problem hiding this comment.
I would also prefer to put these internal use packages into an internal/ directory.
There was a problem hiding this comment.
Eg: If I (or anyone) wanted to play around in a new repo on some experimental tooling that maybe interacts with the
core.Replicatedtype I wouldn't be able to as theinternalstuff would block it.
yes. That's the point of internal. It block access by external packages. It does make the type accessible to any packages within the tree rooted at the parent of the internal directory. So if internal is a the root of defradb, then all our packages within defradb can access internal packages. But internal packages wont be part of our public APIs.
There was a problem hiding this comment.
Lol.. Yes I'm aware, I'm saying I don't want that, as internal is too strict. I want the freedom to play with core (and other types) from other repos
There was a problem hiding this comment.
That does come with the requirement that we maintain those exposed types more carefully because external users can interact with them.
There was a problem hiding this comment.
That does come with the requirement that we maintain those exposed types more carefully because external users can interact with them.
50-50, we can declare that directly using anything but a subset of our packages is unsupported, but it is definitely not going to be clear enough to avoid the occasional support ticket coming through from someone external using pseudo-internal code, as well as occasional confusion for source devs.
There was a problem hiding this comment.
Overall looks great!
Main concern / change is to keep the GQL coupling for the schema stuff (as that is a FAR more complex aspect to decouple with respect to the future plans).
For the Query system, I think the decoupling looks great!
Lastly, wanted to add a comment on this PR (since its open) related to a previously closed PR for the parser. You moved mapper into the planner but I still think planner should be under query (note: just query, not query/graphql).
query/graphql/parser.go
Outdated
| return strings.Contains(request, "IntrospectionQuery") | ||
| } | ||
|
|
||
| func (p *parser) ExecuteIntrospectionRequest(request string) *client.QueryResult { |
There was a problem hiding this comment.
suggestion: I don't think the "parser" should be responsible for executing Introspection Requests, even tho it has all the necessary information.
Fundementally, we should have a single execution API that runs Introspection, Explain, data requests, etc.
There was a problem hiding this comment.
Introspection is very much query-language dependent? You are introspecting the query language, not the collection/defra types
There was a problem hiding this comment.
Indeed it's query language dependent. But from a DevEx perspective, having a single unified execution API feels more consistent. That entrypoint/API should of course delegate to the query language dependant implementation.
Moreover, having it on the Parser level, means that all implementations would need to support it, but its not necessarily the case that all future possible query langauges support the notion of "introspection".
If all requests are handed off to a language dependant Executor or Query or something, with an interface, then its up the the implementation to figure out what to do with the request.
There was a problem hiding this comment.
From a DevEx perspective, this is internal code and provides a unified interface for doing anything query-language specific.
Moreover, having it on the Parser level, means that all implementations would need to support it, but its not necessarily the case that all future possible query langauges support the notion of "introspection".
That is not a problem specific to this PR and is present in develop. IMO the proposed code makes it easier to remove that need, but either way it is irrelevant to this PR.
Query introspection has nothing to do with our database or query execution, the only 'stuff' it touches is 100% query language dependent.
There was a problem hiding this comment.
DevEx has to do with both external and internal developers. When I am making changes/working on the internals of the DB, it would be nice to have a single "Execute" API. The Introspection queries, are still queries, they just get their information from a diff location. All request execution should be through a single API as far as DevEx is concerned. That API implementation would of course go on to determine if its one of a series of kinds of queries supported by that query implementation, and take the appropriate action.
It just feels weird at the moment that an interface called "Parser" is handling a request exection.
There was a problem hiding this comment.
For (future) language implementations that don't support "Introspection" would this just be an empty noop method?
There was a problem hiding this comment.
No idea, I dont see that as important to figure out now. That would be one possible solution, could also break up the interface and then check the type or something.
There was a problem hiding this comment.
Since we've opened the door for this decoupled/abstracted layer for the query system, I think we could go full steam and really abstract everything related to processing requests.
If for example, we had a Requester in the query package, that works with a Parser and Executor interfaces, then we can make sure that the entire query lifecycle is handled by the respective implementation. This (imo) would solve the single execution API question, as well as having a more appropriate location for the Introspection on the executor, while leaving the parser for the parser.
There was a problem hiding this comment.
note: Still using the one planner system tho obvi
There was a problem hiding this comment.
I think we could go full steam and really abstract everything related to processing requests
Not in this PR, it's scope is already large enough and it is starting to disrupt the team's dev flow.
I still strongly disagree that the introspection stuff should be in planner, as it has absolutely nothing to do with anything but the query language
core/parser.go
Outdated
| IsIntrospectionRequest(request string) bool | ||
|
|
||
| // Executes the given introspection request. | ||
| ExecuteIntrospectionRequest(request string) *client.QueryResult |
There was a problem hiding this comment.
suggestion: (Commented on implementation in the query package, adding here) I don't think the parser should be responsible for Introspection execution. All execution should go through a single consistent API
There was a problem hiding this comment.
This doesnt change the public API, and we can discuss the rest in the other comment on this topic
| @@ -61,8 +60,7 @@ type db struct { | |||
|
|
|||
| events client.Events | |||
|
|
|||
| schema *schema.SchemaManager | |||
There was a problem hiding this comment.
thought: I think its OK to keep the GQL coupling on the schema level, as I don't see us supporting multiple schema languages. That isn't to say the query langauge can't have multiple implementations.
Therefore, I think its OK to keep a reference to the schema (manager) on the DB.
There was a problem hiding this comment.
TLDR: The GQL coupling applies to queries, not schemas (imo)
There was a problem hiding this comment.
Therefore, I think its OK to keep a reference to the schema (manager) on the DB.
What for, it would be dead code atm (not to mention it contains types in the gql lib)
There was a problem hiding this comment.
I'm in favour of decoupling if it doesn't impede our functionalities.
There was a problem hiding this comment.
What for, it would be dead code atm (not to mention it contains types in the gql lib)
It would only be dead code if you kept everything about this PR the same. But this PR is two things, decouples Schema from GQL and decouples Query/Request language from GQL.
The query language should be decoupled as is written, but I don't think the schema should be decoupled. Thats a far more complex problem to be able to support different schema systems in one system. It should be a single consistent Schema approach, which can then be queried with whatever language (we implement).
Theres already enough complexities on the roadmap around schemas, I don't want to add this to it, if that makes sense. Perhaps once we get to a good place w.r.t to migration design.
For example/context: Fauna DB supports both FQL (their custom language) and GQL, so theyve decoupled their query language, but they only have a single language for schema definition. Dgraph does the same thing, supports DQL (their custom language) and GQL, but again, only a single schema definition language.
There was a problem hiding this comment.
From what I can tell, youre (hoped) future plans would do the first step twice.
It would do a small subset of it (very roughly speaking) twice, FromSDL includes all the query input types (the bulk of the effort. Whereas the schema creation code only cares about field names and types (a simplification, but you get my point hopefully).
There was a problem hiding this comment.
then we are not decoupled from gql
This is my point tho, "decoupled" is a somewhat loaded term, it is referring to two diff systems, schema and query. I'd prefer to not decouple the schema, as I feel like its unnecessary complicated and we don't get anything as there are no plans to ever not use SDL.
I'm 100% in favor of this PR as it relates to decoupling the query from GQL.
There was a problem hiding this comment.
Yes there would be duplication of effort. At the moment the code is conflating two different concepts and is using gql types (query-language dependent) to defined collectionDescriptions (query-language agnostic).
Indeed, the code conflates the two, as there was no need/goal to seperate them, but internally, they are actually seperated into two steps as seen here:
defradb/query/graphql/schema/generate.go
Line 376 in dbc8cd0
Which is only responsible for taking the SDL, and returning structured GQL objects (without any query type generation, like filtering, sorting, aggregating, etc), just pure developer type as text (sdl) -> structured object.
Additionally, the collection descriptions use the gql objects defined above only, and only to then generate the descriptions, which themselves are free from GQL types technically (eg; we have our own definitions for Scalar types, field definitions, etc... independant from the GQL package).
https://github.com/sourcenetwork/defradb/blob/develop/query/graphql/schema/descriptions.go
There was a problem hiding this comment.
but internally, they are actually seperated into two steps as seen here
It is broken into two funcs sure, but if the first func calls the second it is not seperate.
We have two entirely separate systems collectionDescription generation and query-type generation, which have no reason to be coupled to each other, however they are and the collectionDescription code has been made dependent on the query-type generation code.
Conceptually there is no reason for this, and it causes some confusion. Technically the current system saves a few lines of code, but binds the two together - making substitution much harder, and increasing the damage done by any given bug (if query-generation fails in some way, collectionDescription-generation will likely also be impacted), as well as causing conceptual confusion. It also represents a significant complication to the maintaining of the code, as we now have one code block that must support the requirements of two entirely different systems/requirements/concepts (this IMO is the number one cause of crap and hard to maintain code in projects).
There was a problem hiding this comment.
Had a good chat on zoom with the team. Consensus largely achieved, there are some ways in which both the current and long-term goals can be more clearly described but they will be handled in a new issue (same issue as Schema type convo) to be picked up shortly after this is merged.
| return planner.MakePlan(&request.Request{ | ||
| Queries: []*request.OperationDefinition{ | ||
| { | ||
| Selections: []request.Selection{ | ||
| slct, | ||
| }, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
thought: Might be nice to have some helpers on the request package to remove the labor in making such a request struct.
There was a problem hiding this comment.
Is a personal preference I think - I prefer this atm as you can see the structure that you are using
| Parse(request string) (*request.Request, []error) | ||
|
|
||
| // Adds the given schema to this parser's model. | ||
| AddSchema(ctx context.Context, schema string) (*Schema, error) |
There was a problem hiding this comment.
thought: Might be able to drop the AddSchema API if based on my other comments, we don't remove the schema manager from the DB.
There was a problem hiding this comment.
parking this thought until the rest is resolved
| _, err := exec.ParseRequestString(query) | ||
| if err != nil { | ||
| return errors.Wrap("failed to parse query string", err) | ||
| _, errs := parser.Parse(query) |
There was a problem hiding this comment.
I think hes saying for consistency of naming of the interface methods. I have a slight preference for ParseRequest but not significantly.
Removes odd function from collection/db abi that uses gql schema manager for internal query logic.
Decouples the `defra.db` type from the gql.parser, allowing other query languages to (in theory) be swapped in instead. Does not expose this decoupling to users, as that requires futher thought and design.
Function now uses the request model. Keeps things more cleanly seperated, at a small performance cost.
Declares it's usage to be internal to the planner package, hopefull discouraging it's usage outside, and hopefully clarifying it's purpose. Is a straight cut-paste, no logic has changed in this commit.
AndrewSisley
force-pushed
the
sisley/refactor/I905-decouple-from-gql-2
branch
from
2ffd5f3 to
b31db1b
Compare
|
Last push was just a rebase on latest develop |
* Replace relationManager call with collection logic Removes odd function from collection/db abi that uses gql schema manager for internal query logic. * Decouple db type from gql parser Decouples the `defra.db` type from the gql.parser, allowing other query languages to (in theory) be swapped in instead. Does not expose this decoupling to users, as that requires futher thought and design. * Remove misuse of mapper types from collection func Function now uses the request model. Keeps things more cleanly seperated, at a small performance cost. * Rename planner constructor * Remove unhelpful func * Remove executor * Move mapper internal to planner Declares it's usage to be internal to the planner package, hopefull discouraging it's usage outside, and hopefully clarifying it's purpose. Is a straight cut-paste, no logic has changed in this commit.
Relevant issue(s)
Resolves #566 #905
Description
Further reorganizes a few areas to internally decouple defra.db from our GQL query language. It does not expose our query-model via the collection abi, as we may want further discussion around how to best do that from an UX perspective.
Change also enables the solving of #566, and brings the mapper package into the planner package.
Suggest reviewing commit-by-commit.