chore(compas-indexes): store real indexes and in-progress indexes as separate lists #6300
Conversation
| if (serverSupportsHideIndex(serverVersion)) { | ||
| actions.unshift( | ||
| if ( | ||
| index.compassIndexType === 'regular-index' && |
There was a problem hiding this comment.
This was a bug - we showed the hide button for in-progress indexes.
| @@ -58,6 +74,19 @@ const IndexActions: React.FunctionComponent<IndexActionsProps> = ({ | |||
| ); | |||
| } | |||
|
|
|||
| // you can only drop regular indexes or failed inprogress indexes | |||
There was a problem hiding this comment.
This was also a bug - you could drop an in progress index that was really still in progress and that just wouldn't work because the index creation isn't failed and it isn't a real index yet either.
|
|
||
| // TODO: this should be its own function, not part of dropIndex |
There was a problem hiding this comment.
There was actually momentarily a bug here. Because I'm not merging the in-progress indexes into indexes, the code:
const index = indexes.find((x) => x.name === indexName);
if (!index) {
return;
}
which ran first meant that you could never get to the check to see if the thing is an in-progress index. Which wasn't as obvious to find and I think if the two things weren't both rolled into dropIndex() then it would have been easier to spot, more likely to be tested, etc.
| type: React.ReactNode; | ||
| size: React.ReactNode; | ||
| usage: React.ReactNode; | ||
| usageCount: React.ReactNode; |
There was a problem hiding this comment.
We were changing usageCount into usage back into usageCount..
| @@ -95,7 +95,9 @@ describe('Indexes Component', function () { | |||
| }, | |||
| }); | |||
| expect(screen.getByTestId('indexes-toolbar')).to.exist; | |||
| // TODO: actually check for the error | |||
| expect(screen.getByTestId('indexes-error').textContent).to.equal( | |||
There was a problem hiding this comment.
Drive-by: I spotted the TODO when I audited the new TODOs I'm adding in this PR.
packages/compass-indexes/src/components/regular-indexes-table/index-actions.tsx
Outdated
Show resolved
Hide resolved
| @@ -32,7 +32,7 @@ export type IndexDefinition = { | |||
| | 'columnstore'; | |||
| cardinality: 'single' | 'compound'; | |||
| properties: ('unique' | 'sparse' | 'partial' | 'ttl' | 'collation')[]; | |||
| extra: Record<string, string | number | Record<string, any>>; | |||
| extra: Record<string, string | number | boolean | Record<string, any>>; | |||
There was a problem hiding this comment.
This was just wrong. index.extra.hidden is true if it is set.
packages/compass-indexes/src/components/regular-indexes-table/regular-indexes-table.tsx
Outdated
Show resolved
Hide resolved
packages/compass-indexes/src/components/regular-indexes-table/regular-indexes-table.tsx
Outdated
Show resolved
Hide resolved
packages/compass-indexes/src/components/regular-indexes-table/regular-indexes-table.tsx
Outdated
Show resolved
Hide resolved
Extracted out of #6290
The basic idea is to not merge real indexes and in-progress indexes in the store. That's error prone because you can't really merge them together. All we have to go on is index name and if you override either the in progress details or the real index details it leads to bugs either way. We poll the real details now anyway, so it is eventually consistent as it is. With rolling indexes this just gets worse.
Furthermore, the types aren't really very compatible. We have an index name and little more for in-progress indexes at the moment and for rolling indexes we'll have a name and a type. If we want type and properties for a regular in-progress index we have to infer that based on the input ourselves and in future if we want properties for rolling indexes we have to map those from atlas' response type ourselves. Similar for any other field we might display in the table. Things like size and usage count can only be zero in those cases too. So I decided to have three types (two in this PR) and corresponding lists, then we concatenate them together in the table.
This happens to fit redux's recommendation to not store derived state in the store.
The rest of the changes are caused by that basic decision or just drive-by fixes of bugs I spotted while working on it. Partially caught by having narrower types for regular and in progress indexes.