Update web backend get connection to properly handle field selection and schema refresh #20323
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and schema refresh with newly-discovered fields
octavia-squidington-iv
added
area/platform
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pmossman
reviewed
Dec 9, 2022
| @@ -357,7 +360,7 @@ public WebBackendConnectionRead webBackendGetConnection(final WebBackendConnecti | |||
| connection.setStatus(refreshedCatalog.get().getConnectionStatus()); | |||
| } else if (catalogUsedToMakeConfiguredCatalog.isPresent()) { | |||
| // reconstructs a full picture of the full schema at the time the catalog was configured. | |||
| syncCatalog = updateSchemaWithDiscovery(configuredCatalog, catalogUsedToMakeConfiguredCatalog.get()); | |||
| syncCatalog = updateSchemaWithDiscovery(configuredCatalog, catalogUsedToMakeConfiguredCatalog.get(), catalogUsedToMakeConfiguredCatalog.get()); | |||
There was a problem hiding this comment.
Why are we just passing the same catalogUsedToMakeConfiguredCatalog.get() in twice?
There was a problem hiding this comment.
Oh hm, I think I see why but it's confusing, maybe in the case where there isn't a refreshed catalog, we should call a different, simpler method that takes advantage of the fact that the original discovered catalog is equal to the discovered catalog?
There was a problem hiding this comment.
Yeah I can see how this is confusing.
It wouldn't actually simplify much to have two different methods - essentially it's mostly saying "reconstruct it by looking at what we persisted versus the latest catalog". This PR adds a third element: (1) what we persisted; (2) the whole catalog when we persisted it; (3) the latest catalog. It's just that in the case where the catalog hasn't been rediscovered, (2) and (3) are actually the same. But otherwise the logic stays the same.
I do agree that it stands out as weird to pass the same thing twice like that, so I added a helper method in the hopes of better readability.
WDYT?
There was a problem hiding this comment.
Nice! Makes sense, the helper makes it much clearer
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
pmossman
reviewed
Dec 9, 2022
| originalDiscoveredStream.getStream().getJsonSchema().findPath("properties").fieldNames() | ||
| .forEachRemaining((name) -> originallyDiscovered.add(name)); | ||
| stream.getJsonSchema().findPath("properties").fieldNames().forEachRemaining((name) -> refreshDiscovered.add(name)); | ||
| // We include a selected field if it: |
There was a problem hiding this comment.
great comments, would be hard to follow without them!
pmossman
reviewed
Dec 9, 2022
| final WebBackendConnectionRead result = testWebBackendGetConnection(true, connectionRead, | ||
| operationReadList); | ||
|
|
||
| // We expect the discovered catalog with two fields selected: the one that was originally selected, |
There was a problem hiding this comment.
Also really appreciate the comments describing how the test works!
pmossman
approved these changes
Dec 9, 2022
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
mfsiega-airbyte
temporarily deployed
to
more-secrets
GitHub Actions
Inactive
|
This change broke the "Refresh Schema" button for a lot of connections on my install. This is because of this comment, some lines below the change, the source_catalog_id is empty for those connections: There should be a check to see if |
37 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Make the web backend handler properly handle field selection when there is a schema refresh.
How
Only include a selected field if:
is in the newly discovered schemaANDit was either originally selected OR not in the originally discovered schema at all.TODO before merging: add another unit test for the case where a field is removed in the newly-discovered schema.
Recommended reading order
Reading the new unit test might make the intended logic clearer, and therefore easier to review the actual code change.