feat: Add primary directive for schema definitions (@primary) #650
Conversation
This file contains 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
jsimnz
added
feature
Codecov Report
@@ Coverage Diff @@
## develop #650 +/- ##
===========================================
+ Coverage 57.11% 57.14% +0.02%
===========================================
Files 122 122
Lines 14652 14662 +10
===========================================
+ Hits 8368 8378 +10
Misses 5567 5567
Partials 717 717
|
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shahzadlone
added
the
action/no-benchmark
fredcarle
approved these changes
Jul 22, 2022
query/graphql/schema/generate.go
Outdated
| @@ -432,11 +432,19 @@ func (g *Generator) buildTypesFromAST( | |||
| return nil, err | |||
| } | |||
|
|
|||
| // set the primary relation bit on the relation type if the directive exists on the | |||
| // field | |||
| isPrimary := (findDirective(field, "primary") != nil) | |||
There was a problem hiding this comment.
suggestion (non-blocking): I think there shouldn't be any parentheses around the logical operation. But that is mostly a personal preference.
There was a problem hiding this comment.
I casully use them in this instance to re-inforce the fact that the value is a bool encapsulated by the result of whatever is inside the parenthesis. But I'll remove
query/graphql/schema/generate.go
Outdated
| // set the primary relation bit on the relation type if the directive exists on the | ||
| // field | ||
| isPrimary := (findDirective(field, "primary") != nil) | ||
| reltype := client.Relation_Type_ONE |
There was a problem hiding this comment.
suggestion: I think relType would be more idiomatic
shahzadlone
reviewed
Jul 22, 2022
| Name: RelationLabel, | ||
| Args: gql.FieldConfigArgument{ | ||
| "name": &gql.ArgumentConfig{ | ||
| Type: gql.String, |
There was a problem hiding this comment.
question: Just to confirm, if we only get @relation without a name: = ... argument we still don't want a default name value set.
There was a problem hiding this comment.
Thats a good points. I guess for now, if @relation is present with no name: argument, we do just want the default generated name. The alternative would be to throw an error.
There was a problem hiding this comment.
What is the default generated name right now?
query/graphql/schema/generate.go
Outdated
Comment on lines
1297
to
1318
| func findDirective(field *ast.FieldDefinition, directiveName string) *ast.Directive { | ||
| for _, directive := range field.Directives { | ||
| if directive.Name.Value == directiveName { | ||
| return directive | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
thought: reminds me of this function I wrote in parser/query.go:
// parseExplainDirective returns true if we parsed / detected the explain directive label
// in this ast, and false otherwise.
func parseExplainDirective(directives []*ast.Directive) bool {
// Iterate through all directives and ensure that the directive is at there.
// - Note: the location we don't need to worry about as the schema takes care of it, as when
// request is made there will be a syntax error for directive usage at the wrong location,
// unless we add another directive named `@explain` at another location (which we should not).
for _, directive := range directives {
// The arguments pased to the directive are at `directive.Arguments`.
if directive.Name.Value == parserTypes.ExplainLabel {
return true
}
}
return false
}
But the more I think about it I think it's okay to define it again here as you have done because this is at the schema step and the other at the parsing step.
There was a problem hiding this comment.
I did indeed see this one, but as you noted the diff between the parser and schema packages, and its simple enough.
query/graphql/schema/generate.go
Outdated
| isPrimary := (findDirective(field, "primary") != nil) | ||
| reltype := client.Relation_Type_ONE | ||
| if isPrimary { | ||
| reltype |= client.Relation_Type_Primary |
There was a problem hiding this comment.
question: If I understand this correctly we have:
Relation_Type_Primary RelationType = 128 // 0b1000 0000
Relation_Type_ONE RelationType = 1 // 0b0000 0001
Bitwise OR of them will be = 0b1000 0001 = 129
So now the relType will be set to 129 instead of 128 == client.Relation_Type_Primary ?
I guess you then do client.Relation_Type_Primary | Relation_Type_ONE to check?
There was a problem hiding this comment.
Yupp basically.
To check, you just do some bit arithmetic, eg: if (relType & client.Relation_Type_Primary) > 0 will check if the correct bit is set
There was a problem hiding this comment.
Reminds me of my embedded dev days haha. This works but just wondering what happens if someone defines a Relation_Type_ABC on client that might cause ambiguity to the binary bits.
query/graphql/schema/generate.go
Outdated
| // field | ||
| isPrimary := (findDirective(field, "primary") != nil) | ||
| reltype := client.Relation_Type_ONE | ||
| if isPrimary { |
There was a problem hiding this comment.
nitpick: limit scope of isPrimary to if block or remove completely.
Suggested change
| if isPrimary { | |
| if findDirective(field, "primary") != nil { |
nitpick: This could be even simpler if you just return bool instead from findDirective as you aren't using the returned value anyway (and it's the same value you put in, even if you do need it in future?).
Suggested change
| if isPrimary { | |
| if findDirective(field, "primary") { |
There was a problem hiding this comment.
That's a good suggestion :)
| Name: "published_id", | ||
| Kind: client.FieldKind_DocKey, | ||
| Typ: client.LWW_REGISTER, | ||
| RelationType: client.Relation_Type_INTERNAL_ID, |
There was a problem hiding this comment.
question: Can relation type ever be only Relation_Type_Primary or Relation_Type_Primary | Relation_Type_One ? if so suggesting adding those test cases too?
There was a problem hiding this comment.
They can't. Otherwise the finalize step of the relation manager would either error out, or hasn't been run, which case the finalized field would be false, which would cause an error.
Will think about defensive tests here, but not sure how to trigger it tbh.
There was a problem hiding this comment.
In that case tests look good to me.
| }, | ||
| }, | ||
| { | ||
| description: "Multiple types with relations (one-to-many)", |
There was a problem hiding this comment.
question: To confirm mostly for myself in the 1-N (one to many) case the 1 side will always be primary?
There was a problem hiding this comment.
It matters less for the one-to-many case. We arbitrarily set the one side to be the primary at the moment. Can't 100% remember why. Usually, the primary side is whichever holds the reference to the foreign key, which is technically the many side. So not sure. Will look into this, but its fine for now.
shahzadlone
reviewed
Jul 22, 2022
query/graphql/schema/generate.go
Outdated
| // set the primary relation bit on the relation type if the directive exists on the | ||
| // field | ||
| isPrimary := (findDirective(field, "primary") != nil) | ||
| reltype := client.Relation_Type_ONE |
There was a problem hiding this comment.
Suggested change
| reltype := client.Relation_Type_ONE | |
| relType := client.Relation_Type_ONE |
There was a problem hiding this comment.
Ops nvm I see fred had the same suggestion.
shahzadlone
approved these changes
Jul 22, 2022
There was a problem hiding this comment.
Neat stuff, just some non-blocking suggestions and lots of questions and thoughts mostly for myself.
Also would like the first word of the PR title to be Add instead of Added when you merge =D
LGTM!
For bonus marks maybe you can fix Relation_Type_Primary -> Relation_Type_PRIMARY as part of this PR?
jsimnz
force-pushed
the
jsimnz/feat/primary-directive
branch
from
bb21a62
to
a6c8f41
Compare
Also, added tests for relation directive as those were missing.
jsimnz
force-pushed
the
jsimnz/feat/primary-directive
branch
from
a6c8f41
to
a75f2f4
Compare
jsimnz
changed the title
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
…network#650) * Added primary directive support for schemas * Tests for primary directive
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.
Relevant issue(s)
Resolves #323
Description
Simple implementation, adds support for
@primarydirective on schema field definitions. During generate, we scan and manually applyclient.Relation_Type_Primaryif the directive is present. Only applies to one-to-one relationships. Change is minimal because theRelationManageralready handles the case for manually setting the Primary bit on the relationship metadata type. It only automatically sets it if neither side of the relationship has it already set.Also added a quick test (in the same test suite) for
@relation(name: "...")to manually set the names of relationships, as they were missing.Tasks
How has this been tested?
Added unit tests within the schema package for both
@primaryand@relationdirectives.Specify the platform(s) on which this was tested: