fix: Error if group select contains non-group-by fields #898
Conversation
AndrewSisley
added
bug
client/errors.go
Outdated
| @@ -13,24 +13,30 @@ package client | |||
| import "github.com/sourcenetwork/defradb/errors" | |||
|
|
|||
| const ( | |||
| errFieldNotExist string = "The given field does not exist" | |||
| errFieldNotExist string = "The given field does not exist" | |||
| errSelectOfNonGroupField string = "Cannot select a non-group-by field at group-level" | |||
There was a problem hiding this comment.
suggestion: The linter will flag this when we merge #901. If you don't mind changing Cannot to cannot that would be great.
There was a problem hiding this comment.
Not a problem
- lowercase error
| switch typedChildSelection := childSelection.(type) { | ||
| case *Select: | ||
| result = append(result, typedChildSelection.validateShallow()...) | ||
| default: |
There was a problem hiding this comment.
question: Do you foresee having more than *Select in that switch statement? If not
if typedChildSelection, ok := childSelection.(*Select); ok {
result = append(result, typedChildSelection.validateShallow()...)
}would be more appropriate.
There was a problem hiding this comment.
Yeah, same code block could handle commit, create, update etc - I prefer the switch here
| name | ||
| _group { | ||
| age | ||
| } | ||
| } |
There was a problem hiding this comment.
thought: I'm surprised that this worked before. I guess this PR helps with flagging stuff like this.
There was a problem hiding this comment.
in develop branch these fields just yield the value of the last record in the group, as explain tests dont assert the actual values returned these never failed - explain tests do however check for errors, which is why they started to fail in this branch (before fixing them)
These tests are already covered by existing integration tests and can be removed without loss of coverage.
I really dont think we support this anyway, and I cant see how the feature would work (updating a list of docs, with a provided list of docs)
These tests contained invalid queries and will fall foul of the soon-to-be-added validation rule
AndrewSisley
force-pushed
the
sisley/fix/I680-group-validation
branch
from
7e17fa5
to
b5cce32
Compare
AndrewSisley
force-pushed
the
sisley/fix/I680-group-validation
branch
from
b5cce32
to
2539e39
Compare
Codecov Report
@@ Coverage Diff @@
## develop #898 +/- ##
===========================================
+ Coverage 59.68% 59.78% +0.10%
===========================================
Files 156 156
Lines 17306 17358 +52
===========================================
+ Hits 10329 10378 +49
- Misses 6042 6044 +2
- Partials 935 936 +1
|
…k#898) * Remove unit tests with existing int. tests These tests are already covered by existing integration tests and can be removed without loss of coverage. * Migrate create empty unit test * Migrate invalid mutation test * Remove unsupported unit test I really dont think we support this anyway, and I cant see how the feature would work (updating a list of docs, with a provided list of docs) * Migrate underscored schema name test * Correct explain query tests These tests contained invalid queries and will fall foul of the soon-to-be-added validation rule * Error on select of fields not in group
Relevant issue(s)
Resolves #680
Description
Errors if group select contains non-group-by fields instead of yielding the last doc in the group's values.
Also migrates some parse unit tests to integration tests, as an internal func that it was dependent on change its signature.
Deliberately avoids complicating things too much RE validation, it is easy to move off the parser.Select object if/when we add similar validation for other query types.