refactor: Rework definition validation #2720
Conversation
AndrewSisley
added
area/schema
AndrewSisley
changed the title
AndrewSisley
force-pushed
the
2537-definition-validator-refactor
branch
from
ea8c8c6
to
cbf2d2c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2720 +/- ##
===========================================
+ Coverage 77.95% 78.04% +0.09%
===========================================
Files 310 310
Lines 23113 23231 +118
===========================================
+ Hits 18016 18129 +113
- Misses 3716 3718 +2
- Partials 1381 1384 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| oldState := newDefinitionState(oldCols, map[string]client.SchemaDescription{}) | ||
|
|
||
| for _, validator := range updateValidators { | ||
| err := validator(ctx, db, newState, oldState) |
There was a problem hiding this comment.
question: would it be possible to return all validator errors? It would be helpful to know all of the problems with a schema / collection.
There was a problem hiding this comment.
Was this what you meant in #2568 (I wasn't sure if that ticket was for this, or some other validation somewhere)?
It should be fine and easy to do, although the resultant errors might be quite noisy (some errors will likely result in many other errors being returned).
Can we do this in another PR/issue if we want it? Then we can test it properly (I want to avoid changing the tests as much as possible in this refactor PR)
There was a problem hiding this comment.
Was this what you meant in #2568 (I wasn't sure if that ticket was for this, or some other validation somewhere)?
Yeah I was referencing that issue.
Can we do this in another PR/issue if we want it? Then we can test it properly (I want to avoid changing the tests as much as possible in this refactor PR)
Yeah that's fine with me. I think its purely a user experience issue and not relevant to this PR.
internal/db/collection_define.go
Outdated
| return nil, err | ||
| } | ||
| for i, def := range newDefinitions { | ||
| schema := def.Schema |
There was a problem hiding this comment.
todo: Since schema is not used before being reassigned, please use def.Schema directly in CreateSchemaVersion bellow and instanciate schema at that time.
There was a problem hiding this comment.
Nice spot, removing.
- sort out
schemavars
internal/db/collection_define.go
Outdated
| } | ||
| for i, def := range newDefinitions { | ||
| schema := def.Schema | ||
| txn := mustGetContextTxn(ctx) |
There was a problem hiding this comment.
todo: mustGetContextTxn should be called just once, prior to getAllActiveDefinitions above.
There was a problem hiding this comment.
Will move it to just after getAllActiveDefinitions
- Move txn declaration(s)
AndrewSisley
force-pushed
the
2537-definition-validator-refactor
branch
3 times, most recently
from
17c30d1
to
603b016
Compare
It doesn't make sense to handle these one by one, especially long term where a mix of multiple collections and schemas may be patched in the same call and validation must only take into account the final result. A happy and necessary side effect of this commit is the schema version id is now set before validating new collections - this will simplify the new validation framework.
This means that we can validate that they have been set correctly, as well as simplifying some rules.
The new types will be applied to schema updates soon too. A handful of tests have changed as the order in which rules execute has changed.
A handful of tests have changed due to the order of rule execution changing.
They are mapped and interacted with by users by name, so this rule can never fail - changing the name would trip up other rules though
A handful of tests have changed due to changes in the order in which rules are executed
Is now handled by the (existing) validation rules.
Sorry, I spotted late that all the definitions were being validated on ever iteration
AndrewSisley
force-pushed
the
2537-definition-validator-refactor
branch
from
603b016
to
a55f317
Compare
Relevant issue(s)
Resolves #2537
Description
Reworks definition validation, standardizing the rule signatures, allowing rule reuse across different contexts, and hopefully improving their readability. Performance of the rules will have decreased slightly, but on col/schema update that is unimportant, performance of
createCollections(called when creating via SDL docs) has probably improved slightly due to a reduction in datastore calls.This is largely in preparation for #2401 and the removal/simplification of
SetActiveVersion.