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.
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
refactor: Decouple db.db from gql #912
refactor: Decouple db.db from gql #912
Changes from all commits
d7ff982
62b6823
06745d5
697a8cf
a13cceb
c99afc5
b31db1b
8546150
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe then it would be appropriate to name it
Schemas
?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.
A single Schema supports multiple collections - this struct is a single
Schema
.SchemaDefinition
andDefinitions
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Ticket: #922
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.
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 thedb
package, why not define it there?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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you weren't ;)
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 would also prefer to put these internal use packages into an
internal/
directory.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parking this thought until the rest is resolved
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a personal preference I think - I prefer this atm as you can see the structure that you are using
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.
sounds good.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR: The GQL coupling applies to queries, not schemas (imo)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of decoupling if it doesn't impede our functionalities.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.