feat: Change GetCollectionBySchemaFoo funcs to return many #1984
Conversation
AndrewSisley
added
feature
Is always nil
Conceptually many collections can share the same schema. The implementation of this func can change later once things are stored correctly to actually allow this.
Conceptually many collections can share the same schema. The implementation of this func can change later once things are stored correctly to actually allow this.
AndrewSisley
force-pushed
the
1963-detangle-col-fetch-funcs
branch
from
d4be441 to
44ee5f4
Compare
planner/commit.go
Outdated
|
|
||
| // WARNING: This will become incorrect once we allow multiple collections to share the same schema | ||
| // todo: link to tickets. | ||
| // We should instead fetch the collection be global collection ID: |
There was a problem hiding this comment.
I managed to leave half the todo text in there too..., thanks for spotting Fred :)
- Fix warning text
cli/collection.go
Outdated
| if name != "" { | ||
| fetchedCols := cols | ||
| cols = nil | ||
| for _, c := range fetchedCols { | ||
| if c.Name() == name { | ||
| cols = append(cols, c) | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
thought: This seems superfluous. If name != "", there will be exactly one collection anyways.
thought (out-of-scope): We should eventually ensure that the flags that are part of the switch statement are mutually exclusive.
There was a problem hiding this comment.
This seems superfluous. If name != "", there will be exactly one collection anyways.
There will be one collection, but the user may have requested it at a specific schema version (by providing both a name, and schema version param). So we need to fetch by schema version and then filter by name.
I'll add a comment.
- Add comment
We should eventually ensure that the flags that are part of the switch statement are mutually exclusive.
I disagree with this, for the use case above.
Codecov ReportAttention:
@@ Coverage Diff @@
## develop #1984 +/- ##
===========================================
- Coverage 74.46% 74.18% -0.29%
===========================================
Files 246 246
Lines 24309 24426 +117
===========================================
+ Hits 18101 18118 +17
- Misses 5010 5095 +85
- Partials 1198 1213 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
cli/collection.go
Outdated
| // Multiple params may have been specified, and in some cases both are needed. | ||
| // For example if a schema version and a collection name have been provided, | ||
| // we need to ensure tha a collection at the requested version is returned. | ||
| // Likewise we need to ensure that if a name and schema id are provided, but | ||
| // there are none matching both, that nothing is returned. |
There was a problem hiding this comment.
thought: I don't think this comment reflect what happens in the switch statement. Only one of the three will ever be used.
There was a problem hiding this comment.
Yes, but that is due to how they interact.
If name and another param is provided, the other param will be used and name applied after.
If schema version is provided, schema does not matter. We could check that the schema version fetched is actually of the given schema though in case someone provides impossible params.
There was a problem hiding this comment.
Ah yes ok I see what you mean 👍
There was a problem hiding this comment.
// we need to ensure **that** a collection
There was a problem hiding this comment.
Ah yes ok I see what you mean
Am adding an error for that schema-schema version id clash
- add error
we need to ensure that a collection
Cheers, will change :)
- collection name
…work#1984) ## Relevant issue(s) Resolves sourcenetwork#1963 ## Description Changes GetCollectionBySchemaFoo function signatures to return many Collections, instead of single values. Conceptually these functions were broken and relied on the fact that we cannot yet link multiple collections to the same schema(version)(s). This changes their signatures and the code that calls them to reflect what they should do long term. The code that calls these functions now hosts the 'broken' code. These will still need to change once we add global IDs. The implementation of these functions will change later in sourcenetwork#1964.
Relevant issue(s)
Resolves #1963
Description
Changes GetCollectionBySchemaFoo function signatures to return many Collections, instead of single values.
Conceptually these functions were broken and relied on the fact that we cannot yet link multiple collections to the same schema(version)(s). This changes their signatures and the code that calls them to reflect what they should do long term.
The code that calls these functions now hosts the 'broken' code. These will still need to change once we add global IDs.
The implementation of these functions will change later in #1964.