-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: Allow users to add Views #2114
feat: Allow users to add Views #2114
Conversation
b8d74e1
to
86ce3ff
Compare
86ce3ff
to
aef1732
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2114 +/- ##
===========================================
+ Coverage 74.17% 74.35% +0.17%
===========================================
Files 248 252 +4
Lines 24845 25217 +372
===========================================
+ Hits 18428 18748 +320
- Misses 5149 5185 +36
- Partials 1268 1284 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
} | ||
// `schemas` will contain all versions of that name, as views cannot be updated atm this should | ||
// be fine for now | ||
schema = schemas[0] |
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: Should we try to ensure it's the latest version of the schema?
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 thought about that, but it felt like too much code right now. If it was really cheap to do so I would probably have added it in, but it is ultimately dead-code atm given that there can only be one version.
Better to wait until we actually implement something that makes this necessary (and testable), and then code it properly IMO.
var isEmbedded bool | ||
for _, definition := range collections { | ||
if t.Name() == definition.Schema.Name && definition.Description.Name == "" { | ||
isEmbedded = true |
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: You rather use this method than to use continue
with a loop tag? The block content is short enough that if would still read easily. Fine either way. Just curious.
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 see loop tags as a bit unusual and my brain has not get chosen to either ban them or always use them. Sometimes I use them sometimes I don't, I think I mostly only use them in tiny (~10 line or less) functions where it is hard for them to get lost.
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.
Looks good so far. Just have few minor comments
cli/errors.go
Outdated
@@ -27,6 +27,7 @@ var ( | |||
ErrNoLensConfig = errors.New("lens config cannot be empty") | |||
ErrInvalidLensConfig = errors.New("invalid lens configuration") | |||
ErrSchemaVersionNotOfSchema = errors.New(errSchemaVersionNotOfSchema) | |||
ErrViewAddMissingArgs = errors.New("please provide a base query and output SDL") |
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.
suggestion: looking at the text alone, it's to make anything out of it.
Would be nice to add some view-related context.
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.
Not a problem, will add :)
- Tweak error text
// } | ||
// | ||
// | ||
// A GQL SDL that matches its output type must also be provided. There can only be one `type` declaration, |
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.
nitpick: 2 spaces
Also on the next line I think it should be "schema-only"
And on the next line again 2 spaces
Also in one the last line 2 spaces
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.
- schema-only
client/descriptions.go
Outdated
@@ -30,6 +32,13 @@ type CollectionDescription struct { | |||
// The ID of the schema version that this collection is at. | |||
SchemaVersionID string | |||
|
|||
// BaseQuery contains the base query of this view, if this is collection is a view. |
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.
nitpick: "is this collection is"
Also 2 spaces on the next line
Should it be "Actor-defined"?
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.
Cheers :)
- "is this collection is" typo
Should it be "Actor-defined"?
I don't think so, why do you think so?
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 don't think so, why do you think so?
Otherwise it the sentence sound unnatural (at least to my non-native ears). Why then you used past tense "Actor defined"?
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.
Why then you used past tense "Actor defined"?
They are aggregates that have been (past tense), by an actor. Both with and without hyphen made sense to me, and I think I over-use special characters when writing, and so I try and avoid them.
Grammarly suggests using hyphens like you suggested here: https://www.grammarly.com/blog/hyphen/ though. I'm not sure it is a hard grammar rule in English though, Collins (respected author of dictionaries) keeps their usage simple: https://grammar.collinsdictionary.com/easy-learning/when-do-you-use-a-hyphen-in-english.
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.
good arguments, thanks. I'm not trying to say that it's wrong or not. Just giving you my perception.
My judgement based mostly on my own understanding. If I had to read it few time to make sure I understand, then some others will do as well.
s.Fields = make([]Selection, len(selectMap.Fields)) | ||
|
||
for i, field := range selectMap.Fields { | ||
fieldJson, err := json.Marshal(field) |
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: why do you need to marshal the field first? Can't you unmarshal directly from map[string]json.RawMessage
enough?
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.
selectJson
acts as a strongly typed version of map[string]json.RawMessage
with some compile time checks, and, most importantly, documentation. I prefer it over just going to map[string]json.RawMessage
.
client/request/select.go
Outdated
// We detect which concrete type each `Selection` object is by detecting | ||
// non-nillable fields, if the key is present it must be of that type. | ||
// They must be non-nillable as nil values may have their keys omitted from | ||
// the json. This als relies on the fields being unique. We may wish to change |
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.
nitpick: "This als"?
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.
Cheers lol
- fix (another) typo in doc
http/handler_store.go
Outdated
@@ -106,6 +106,25 @@ func (s *storeHandler) SetDefaultSchemaVersion(rw http.ResponseWriter, req *http | |||
rw.WriteHeader(http.StatusOK) | |||
} | |||
|
|||
func (s *storeHandler) CreateView(rw http.ResponseWriter, req *http.Request) { |
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: why is it called here CreateView
? Everywhere else it's AddView
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.
Everything was called CreateView
when I originally wrote it, then I realized AddView
fitted much more nicely with out existing funcs and I had to rename everything - I must have missed this function :)
- Rename to
AddView
return nil, nil, err | ||
// If the collection is not found, check to see if a schema of that name exists, | ||
// if so, this must be an embedded object | ||
schemas, err := store.GetSchemasByName(ctx, collectionName) |
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.
todo: you assume here that the error by GetCollectionByName
is due to the missing collection. But there can be other reasons, which are ignored here.
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, I couldn't actually find an easy, reliable way to check this (the current error returned is the horrible 'datastore not found error', which I do not consider a reliable). I figured not checking the error at all was the safest atm.
I'll re-visit this, but do you have any off-the-top-of-your-head suggestions in the meantime?
- Revisit mapper error check 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've added a todo and a new issue: #2146
request/graphql/schema/collection.go
Outdated
@@ -430,6 +471,13 @@ func getRelationshipName( | |||
} | |||
|
|||
func finalizeRelations(relationManager *RelationManager, definitions []client.CollectionDefinition) error { | |||
embeddedObjNames := map[string]any{} |
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.
todo: change to stronger typing map[string]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.
Cheers, don't know why I went for any
here lol
- struct{}
_count | ||
} | ||
}`, | ||
ExpectedError: "aggregate must be provided with a property to aggregate", |
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: I don't quite follow how it works. If the _count
is specified in SDL why can't it be used here?
If you can't override a reserved property why doesn't it throw an error inCreateView
?
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 #2113 linked in the description of the test func doc - _count
is not protected by us and is half-overrideable.
name | ||
numberOfBooks | ||
} | ||
}`, |
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.
nitpick: I've seen a lot in the tests that indentation is correct for raw strings. Would be nice to fix them (for other occurrences as well). I don't think lint will report 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.
Ah true, my bad, I'll try and fix the ones in this PR at least for now. Agree a linter would be nice long term.
- Fix test query indentation
// Properties above this line match the `Select` object and | ||
// are deserialized using the normal/default logic. | ||
// Properties below this line require custom logic in `UnmarshalJSON` | ||
// in order to be deserialized correctly. |
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.
suggestion: Although I like this comment, I find that overtime this might be hard to enforce / maintain. Perhaps in addition to this add a different prefix to variable names to the ones that require custom logic? i.e. CustomFields
instead of Fields
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.
The current solution for sure has it's flaws, but I'm not sure I like that suggestion. I'll add a task to re-visit and see how I feel about it again in a little bit :)
- Revisit this suggestion
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 still don't like the suggestion, and will leave as-is unless anyone continues this thread :)
planner/view.go
Outdated
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.
todo: Since a new "viewNode" is being added to the planner:
-
Must add it to the
allPlanNodeNames
variable in thetests/integration/explain.go
file for debug node, which dumps the entire plan node graph (including non-explainable nodes). -
Following up on (1) please do add a test for the debug explain under the folder
tests/integration/explain/debug
. -
discussion/question: Do we need to make
viewNode
explainable? any attributes that might be important to view for the simple explain (non-debug explain)? @jsimnz
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.
(3) Can be done in another PR, I can't see any reason for adding it to this.
question: Why do you think (1) and (2) need to be done in this PR? The tests pass atm, is production explain broken? Is there a gap in explain integration tests?
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.
Because Debug explain needs to be aware of all nodes in the plan graph, otherwise it likely skips the node (it needs to know all names of the valid plan nodes).
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 not required for this PR
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.
otherwise it likely skips the node
'It' being the production code or the test code?
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' being the production code or the test code?
Reading the code referenced, I assume the test code.
The test you asked for in (2), this would in effect just be testing that viewNode.Kind()
returns "viewNode"
no? Is there anything else that could go wrong?
note: Test has been added.
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 for (1) I meant that it would break the testing framework, as it needs to know what is and isn't a valid node since we can target specific nodes, all you need to do is add it to the list, it shouldn't break or fail anything.
(2) Yea once you test with @explain(type: debug)
on any of your view queries, all it would show is that "viewNode" is properly being populated and in the correct structure that you expect it to be in the full build graph.
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.
Nice, cheers Shahzad, sorry for the questions - is maybe the first time I'm adding an explain test and I want to make sure I understand it all :)
@@ -365,14 +409,23 @@ func (g *Generator) buildTypes( | |||
// will be reassigned before the thunk is run | |||
collection := c | |||
fieldDescriptions := collection.Schema.Fields | |||
isEmbeddedObject := collection.Description.Name == "" |
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.
suggestion: A brief note / concrete example here describing what is meant by an "embedded object" here would be nice.
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 do see embedded
object as a ubiquitous database/defra concept that needs no explanation against every variable declaration that mentions it. I'd be quite against doing this.
existingDefinitions[i] = existingCollections[i].Definition() | ||
} | ||
|
||
err = db.parser.SetSchema(ctx, txn, append(existingDefinitions, newDefinitions...)) |
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 would be much nicer to have a AddSchema
method on parser instead of having to query existing schemas to add a new one.
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.
Food for thought, but I'm not sure it is worth the hassle that would mean in the parser/gql code. That hassle would also increase the cost of adding any new query languages who would need to re-write their own flavour of that hassle in order to satisfy the interface. Performance isn't a concern in these functions.
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.
At the moment it's a hassle that is needed every time there is a call to SetSchema
. Don't you think it would be better to have that in just one location instead?
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.
Not every time, the existing are required when updating schema (and in the future, views). I also see replacing everything with a new, clean instance as safer than mutating 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.
Don't you think it would be better to have that in just one location instead?
We can do this without hosting it inside the Parser interface.
A schemaFromAstDefinition function will be added shortly
This function will be called using an interface host, not just a object host soon.
Relying on schema only will make it easier in the near future when we start working with embedded (schema-only) types from complex views. I think I managed to remove any bits of code that reference stuff on fields that is only accidentally on the schema object, so hopefully this should not complicate any future work where stuff current on schema fields gets moved to collection fields.
1388bfe
to
1484964
Compare
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.
LGTM 🙂 as long as all the conversations are 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.
Thanks for adding the Debug Explain tests. LGTM
Bug bash issue: #2182 |
Bug bash issue: #2183 |
Bug bash issue: #2189 |
Bug bash issue: #2190 |
Bug bash issue: #2191 |
Bug bash issue: #2201 |
Bug bash issue: #2206 |
Bug bash issue: #2212 |
Bug bash issue: #2218 |
Bug bash issue: #2219 |
## Relevant issue(s) Resolves sourcenetwork#2073 ## Description Allows users to add Views. Defined views and embedded schema types are visible when calling GetCollections/Schema etc.
## Relevant issue(s) Resolves sourcenetwork#2073 ## Description Allows users to add Views. Defined views and embedded schema types are visible when calling GetCollections/Schema etc.
Relevant issue(s)
Resolves #2073
Description
Allows users to add Views.
Defined views and embedded schema types are visible when calling GetCollections/Schema etc.
Does not provide a means to delete or update them.
Does not allow users to create documents via views.
Does not cache view results.
Does not allow users to define Lens transforms for their views.
Does not allow syncing of results over P2P.
Note: The tests are a bit light for this, covering the very basics only. I would like to add some more complex testing in another PR - as views are as complex as regular queries, I think it would be best to add some magic to the test framework to auto test all normal queries via auto-generated views (a new test-matrix dimension, only needs to run against mem-store, one OS, save mutation type, etc), otherwise to properly test we'll end up duplicating most of our integration test suite. We'll still want some dedicated view tests, for view-specific stuff, those I have added, as well as a few important ones (such as basic filter and aggregate tests) that could be ripped out once that test magic is in place.
I have manually tested the openapi stuff (on linux mint).