Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🪟 🎉 Clear user-defined primary key(s) and cursor field(s) if the field is removed #20203

Merged
merged 16 commits into from Dec 12, 2022

Conversation

teallarson
Copy link
Contributor

@teallarson teallarson commented Dec 7, 2022

What

closes #19387

Clears the primary key and/or cursor properties on the stream.config object if any of the fields referenced there are removed during a refresh.

How

  • useInitialValues (in formConfig) identifies if there is a breaking change on the connection. If there is, it will traverse the diff and pull out the transforms for the field(s) that were removed
  • the transforms get sent along to calculateInitialValues which is already used to calculate and override config fields
  • as each stream is evaluated, if it is included in the list of streams with broken changes, a cleanup function is called that will find a match on either a primary key (or a partial of a compound primary key) or cursor field. If a match is found for the removed field, we clear that property on the config object.

Recommended reading order

  1. calculateInitialCatalog.test.tsx (for expected behaviors)
  2. formConfig.tsx
  3. calculateInitialCatalog.ts

Testing

This is not behind the feature flag, but should not affect anything negatively. Open to discussion if we'd rather keep it behind the flag. Either way, useful to test both with REACT_APP_AUTO_DETECT_SCHEMA_CHANGES as true and as false

At bare minimum:

  • removing fields for user-configured primary keys and cursors
  • adding and removing fields for source-defined primary keys and cursors

These are also covered by unit tests.

Next steps

These files are due for a refactor.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 7, 2022
@teallarson teallarson changed the title 🪟 🎉 Clear primary key(s) and cursor field(s) if the field is removed 🪟 🎉 Clear user-defined primary key(s) and cursor field(s) if the field is removed Dec 7, 2022
if (streamNode.config?.primaryKey && removedFieldPaths?.length) {
// if any of the field paths in the primary key match any of the field paths that were removed, clear the entire primaryKey property
if (
streamNode.config.primaryKey.some((key) => removedFieldPaths.some((removedPath) => isEqual(key, removedPath)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible that lodash intersection could tidy this up, but i haven't figured out how

}

// if the stream is new since a refresh, verify cursor and get optimal sync modes
const isStreamNew = newStreamDescriptors?.some(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no changes to this logic, just cleaning up some naming here for clarity

@teallarson teallarson marked this pull request as ready for review December 7, 2022 23:23
@teallarson teallarson requested review from a team as code owners December 7, 2022 23:23
: [];

// if we're in edit or readonly mode and the stream is not new, check for breaking changes then return
if (isNotCreateMode && !isStreamNew) {
Copy link
Contributor Author

@teallarson teallarson Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that this check happens on readonly connections. Presumably readonly connections wouldn't ever have a breaking change so should return immediately when clearBreakingFieldChanges() is called. It's not the only place we're doing something like this. I have proposed some cleanup in #20233.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this with @tealjulia over Slack. We agree that it could be improved but it would require a lot of work outside of the scope of solving the current problem.

: [];

// if we're in edit or readonly mode and the stream is not new, check for breaking changes then return
if (isNotCreateMode && !isStreamNew) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this with @tealjulia over Slack. We agree that it could be improved but it would require a lot of work outside of the scope of solving the current problem.

teallarson and others added 2 commits December 9, 2022 15:55
…itialCatalog.ts

Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>
…itialCatalog.ts

Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>
@teallarson teallarson merged commit 21e7290 into master Dec 12, 2022
@teallarson teallarson deleted the teal/clear-removed-source-defined-fields branch December 12, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clear invalid primary key or cursor when field is removed after schema refresh
4 participants