feat: Allow field indexing by name in PatchSchema #1810
Conversation
No point doing the same work twice, and it is about to gain an extra usage
AndrewSisley
added
feature
The old way was silly, and hard to use.
AndrewSisley
force-pushed
the
1169-field-index-substitution
branch
from
260e5c7
to
b1e5038
Compare
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1810 +/- ##
===========================================
- Coverage 75.72% 75.69% -0.03%
===========================================
Files 209 209
Lines 22211 22263 +52
===========================================
+ Hits 16819 16851 +32
- Misses 4227 4242 +15
- Partials 1165 1170 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
db/schema.go
Outdated
|
|
||
| desc := collectionsByName[splitPath[schemaNamePathIndex]] | ||
| var index string | ||
| if index == "" { |
There was a problem hiding this comment.
todo: This if will always be true. You should probably remove it.
There was a problem hiding this comment.
Cheers, I moved this block around a bit and missed this silliness.
- Remove extra check
db/schema.go
Outdated
| @@ -171,23 +181,82 @@ func substituteSchemaPatch( | |||
| patch jsonpatch.Patch, | |||
| collectionsByName map[string]client.CollectionDescription, | |||
| ) (jsonpatch.Patch, error) { | |||
| fieldIndexesByName := make(map[string]map[string]int, len(collectionsByName)) | |||
There was a problem hiding this comment.
suggestion: The naming of this and colFieldsIndexesByName makes understanding the added code bellow confusing. I would suggest a rename.
fieldIndexesByName -> fieldIndexesByCollection
colFieldIndexesByName -> fieldIndexesByName
There was a problem hiding this comment.
I like that, will rename throughout - thanks Fred.
- Rename map
db/schema.go
Outdated
| for colName, col := range collectionsByName { | ||
| colFieldIndexesByName := make(map[string]int, len(col.Schema.Fields)) | ||
| fieldIndexesByName[colName] = colFieldIndexesByName | ||
| for i, field := range col.Schema.Fields { | ||
| colFieldIndexesByName[field.Name] = i | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: This can be simplified to
for colName, col := range collectionsByName {
fieldIndexesByCollection[colName] = make(map[string]int, len(col.Schema.Fields))
for i, field := range col.Schema.Fields {
fieldIndexesByCollection[colName][field.Name] = i
}
}There was a problem hiding this comment.
It is one less line, but an extra call. I prefer the current, there is actually less to read even though it takes up more eye-space.
db/schema.go
Outdated
| } else { | ||
| fieldIndexesByName[desc.Name] = map[string]int{ | ||
| // The DocKey field is always at the zero index and we need to account for it | ||
| request.KeyFieldName: 0, | ||
| } | ||
| } |
There was a problem hiding this comment.
question: If the provided collection doesn't exist, shouldn't we return an error here?
There was a problem hiding this comment.
This is me falling into the trap of coding for the future and tolerating dead code to do so.
I was concerned about when PatchSchema gains the ability to add new collections, however this block sits within an isFieldOrInner check, and so when the time comes it would be a poor and surprising location to do this.
I will just delete the else block.
- remove dead code
There was a problem hiding this comment.
What about the option to return an error in this isFieldOrInner block? I understand potentially adding schemas from a patch operation but adding a field or changing the kind of a field in a schema that doesn't exist should probably be an error. What do you think?
There was a problem hiding this comment.
Returning an error here would be dead code, there would be no way to reach it. If/when we allow new schema to be added via PatchSchema then we can add some logic somewhere to handle it, it would likely be located somewhere else.
## Relevant issue(s) Resolves sourcenetwork#1169 ## Description Allow field indexing by name in PatchSchema.
Relevant issue(s)
Resolves #1169
Description
Allow field indexing by name in PatchSchema.