馃獰 馃Ч Match sync catalog items by name and namespace, not their index #20387
Conversation
octavia-squidington-iv
added
area/platform
teallarson
changed the title
airbyte-webapp/src/components/connection/CatalogTree/CatalogTree.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/components/connection/CatalogTree/CatalogTree.tsx
Outdated
Show resolved
Hide resolved
| return stream.config?.selected !== initialValues.syncCatalog.streams[idx].config?.selected; | ||
| streams.filter((stream) => { | ||
| const matchingInitialValue = initialValues.syncCatalog.streams.find( | ||
| (initialStream) => |
There was a problem hiding this comment.
We also match by id (which is the stringified index of the item in the array) a little higher up, and in the BulkEditService. TBD whether those should also be changed to use name & namespace as matchers. I am unsure if either would get called in a case where using the index would be unreliable.
There was a problem hiding this comment.
I had the same thought as well when I was looking at this. I wonder if we can trust the ID, but in this solution we know we can trust name + namespace as a matching criteria so this is less risky.
There was a problem hiding this comment.
Do you think it's worth making a ticket to audit other uses of index/stream id? I'm not sure they're a problem, but I'm not sure they're not a problem.
There was a problem hiding this comment.
I think it's good to track this in a ticket. If the id proves to be trustworthy, we can simplify a lot of checks in our code.
There was a problem hiding this comment.
Ope. I hit merge. The id is the item's index in the array converted to a string, so it would presumably have the same problems.
| return stream.config?.selected !== initialValues.syncCatalog.streams[idx].config?.selected; | ||
| streams.filter((stream) => { | ||
| const matchingInitialValue = initialValues.syncCatalog.streams.find( | ||
| (initialStream) => |
There was a problem hiding this comment.
I had the same thought as well when I was looking at this. I wonder if we can trust the ID, but in this solution we know we can trust name + namespace as a matching criteria so this is less risky.
What
Match by stream name and namespace instead of by syncCatalog index.
Fixes a bug when editing a connection where adding or removing a stream from the source, refreshing the source schema, then canceling the changes in the form would throw an error in
<CatalogTree/>How
Name and namespace are more stable matchers than index. The array may change based on refreshes, making it not a stable reference.