Source TikTok Marketing : Access token is null #14299 #14461

alexandr-shegeda · 2022-07-06T17:52:56Z

What

After recent changes introduced in this PR logic related to path recalculation for JSON nodes we faced an issue for connectors that contain oneOf elements that have the same field names. So, for described case getSortedSecretPaths() returned duplicates, which lead to incorrect postprocessing during secret replacement.

How

Made getSortedSecretPaths() return a unique values.

🚨 User Impact 🚨

Without these changes, users are not able to configure a proper connection using multiple auth options on spec.

Pre-merge Checklist

Updating a connector

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Unit

Before fix:

After fix:

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

… when we have multiple options with the same key on spec

…ave a unique path values

grishick · 2022-07-06T18:59:47Z

because this change affects many connectors, please test that it does not break any GA and Beta connectors and also verify that it does not break jobs after a user upgrades to a platform version

sherifnada · 2022-07-07T22:06:59Z

...ig-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHelpers.java

@@ -178,6 +178,7 @@ public static List<String> getSortedSecretPaths(final JsonNode spec) {
            .anyMatch(field -> field.getKey().equals(JsonSecretsProcessor.AIRBYTE_SECRET_FIELD)))
        .stream()
        .map(JsonPaths::mapJsonSchemaPathToJsonPath)
+        .distinct()


can you add a unit test for this?

added unit tests. will attach screenshots with failed tests without this change

can you add in the javadoc comment that it will not return duplicates?

added javadoc

kimerinn · 2022-07-08T00:13:17Z

TikTok works well

kimerinn · 2022-07-08T00:19:45Z

BigQuery also

kimerinn · 2022-07-08T08:50:56Z

GitHub

cgardens

We need tests here that show the problem and then how this fixes it.

added unit tests

…acement' into osheg/bug-with-json-secrets-replacement

alexandr-shegeda · 2022-07-13T10:13:21Z

We need tests here that show the problem and then how this fixes it.

hi @cgardens, I have added unit tests for the described case, and also updated the description with failed unit tests without the fix.

cgardens

thanks! the test is great.

can you point to what line of code causes things to work in an unexpected way when there are duplicates? this is still not intuitive to me, and i want to understand where the brittleness is coming from.

cgardens · 2022-07-13T15:39:37Z

...ig-persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHelpers.java

@@ -178,6 +178,7 @@ public static List<String> getSortedSecretPaths(final JsonNode spec) {
            .anyMatch(field -> field.getKey().equals(JsonSecretsProcessor.AIRBYTE_SECRET_FIELD)))
        .stream()
        .map(JsonPaths::mapJsonSchemaPathToJsonPath)
+        .distinct()


can you add in the javadoc comment that it will not return duplicates?

alexandr-shegeda · 2022-07-13T16:29:11Z

thanks! the test is great.

can you point to what line of code causes things to work in an unexpected way when there are duplicates? this is still not intuitive to me, and i want to understand where the brittleness is coming from.

@cgardens in case SecretsHelpers#getSortedSecretPaths return duplicate path
then inside of SecretsHelpers#internalSplitAndUpdateConfig we will replace incoming config value with the secret, and then after the second run of method JsonPaths.replaceAt()
it will not find a value of the original field, because it was replaced with {"_secret" : "..."}, so we will just erase the secret value as it showed on this screenshot

updated javadoc

supertopher

Based on local testing and my understanding of the problem space this looks good to go.

Tik Tok access token are broken so we hope to see a more different error here.

cgardens

Thank, @alexandr-shegeda ! That explanation makes sense to me too. I think we are good to go on this.

benmoriceau · 2022-07-13T19:25:06Z

thanks! the test is great.

can you point to what line of code causes things to work in an unexpected way when there are duplicates? this is still not intuitive to me, and i want to understand where the brittleness is coming from.

@cgardens The issue is because this duplicated are secrets. Here is what happens:

We have 2 paths for the secret that are deplicated (because they are in a oneOf)
For each path we will try to sanitize the config in order to hide the secret and store in the secret manager
The first one works well, store the secret in the secret store and replace the value by a coordiante
The second on can read the secret value because it has been replace by a coordinate but doesn't error out and replace the first coordinate by a new coordinate.
Then when we try to retrieve the secret value, it is invalid.
Does it makes sense?

Edit: Sorry I didn't saw that the explaination was post already

supertopher

triple approve?

alexandr-shegeda added 2 commits July 6, 2022 19:36

Replaced List with Set in order to avoid credentials loosing for case…

12c100a

… when we have multiple options with the same key on spec

returned back List to save the order of fields, added distinct() to h…

faf0e63

…ave a unique path values

alexandr-shegeda linked an issue Jul 6, 2022 that may be closed by this pull request

Source TikTok Marketing : Access token is null #14299

Closed

alexandr-shegeda requested review from cgardens and grishick July 6, 2022 17:58

clean up

b090bd3

alexandr-shegeda temporarily deployed to more-secrets July 6, 2022 18:02 Inactive

sherifnada reviewed Jul 7, 2022

View reviewed changes

Merge branch 'master' into osheg/bug-with-json-secrets-replacement

da14684

kimerinn temporarily deployed to more-secrets July 8, 2022 09:52 Inactive

cgardens reviewed Jul 8, 2022

View reviewed changes

alexandr-shegeda added 3 commits July 13, 2022 13:04

[14299] Source TikTok Marketing : Access token is null

38c0a3c

added unit tests

Merge remote-tracking branch 'origin/osheg/bug-with-json-secrets-repl…

f2b5765

…acement' into osheg/bug-with-json-secrets-replacement

Merge branch 'master' into osheg/bug-with-json-secrets-replacement

eb90680

alexandr-shegeda temporarily deployed to more-secrets July 13, 2022 10:08 Inactive

cgardens reviewed Jul 13, 2022

View reviewed changes

[14299] Source TikTok Marketing : Access token is null

4955a0f

updated javadoc

alexandr-shegeda temporarily deployed to more-secrets July 13, 2022 16:44 Inactive

alexandr-shegeda mentioned this pull request Jul 13, 2022

Source TikTok Marketing : Access token is null #14299

Closed

benmoriceau requested review from cgardens July 13, 2022 19:09

Format

fb73ca1

benmoriceau requested review from gosusnp and lmossman July 13, 2022 19:14

benmoriceau requested a review from supertopher July 13, 2022 19:14

benmoriceau temporarily deployed to more-secrets July 13, 2022 19:16 Inactive

supertopher approved these changes Jul 13, 2022

View reviewed changes

benmoriceau requested review from supertopher and sherifnada July 13, 2022 19:18

cgardens approved these changes Jul 13, 2022

View reviewed changes

supertopher approved these changes Jul 13, 2022

View reviewed changes

gosusnp approved these changes Jul 13, 2022

View reviewed changes

benmoriceau approved these changes Jul 13, 2022

View reviewed changes

alexandr-shegeda merged commit c308c2f into master Jul 13, 2022

alexandr-shegeda deleted the osheg/bug-with-json-secrets-replacement branch July 13, 2022 19:29

octavia-squidington-iii mentioned this pull request Jul 13, 2022

Bump Airbyte version from 0.39.35-alpha to 0.39.36-alpha #14690

Merged

drrest mentioned this pull request Jul 14, 2022

🎉 Source Notion: add OAuth authorization for source-notion connector #14706

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source TikTok Marketing : Access token is null #14299 #14461

Source TikTok Marketing : Access token is null #14299 #14461

alexandr-shegeda commented Jul 6, 2022 •

edited

Loading

grishick commented Jul 6, 2022

sherifnada Jul 7, 2022

alexandr-shegeda Jul 13, 2022

cgardens Jul 13, 2022

alexandr-shegeda Jul 13, 2022

kimerinn commented Jul 8, 2022 •

edited

Loading

kimerinn commented Jul 8, 2022

kimerinn commented Jul 8, 2022

cgardens left a comment

alexandr-shegeda commented Jul 13, 2022

cgardens left a comment

cgardens Jul 13, 2022

alexandr-shegeda commented Jul 13, 2022

supertopher left a comment

cgardens left a comment

benmoriceau commented Jul 13, 2022 •

edited

Loading

supertopher left a comment

Source TikTok Marketing : Access token is null #14299 #14461

Source TikTok Marketing : Access token is null #14299 #14461

Conversation

alexandr-shegeda commented Jul 6, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Airbyter

Tests

grishick commented Jul 6, 2022

sherifnada Jul 7, 2022

Choose a reason for hiding this comment

alexandr-shegeda Jul 13, 2022

Choose a reason for hiding this comment

cgardens Jul 13, 2022

Choose a reason for hiding this comment

alexandr-shegeda Jul 13, 2022

Choose a reason for hiding this comment

kimerinn commented Jul 8, 2022 • edited Loading

kimerinn commented Jul 8, 2022

kimerinn commented Jul 8, 2022

cgardens left a comment

Choose a reason for hiding this comment

alexandr-shegeda commented Jul 13, 2022

cgardens left a comment

Choose a reason for hiding this comment

cgardens Jul 13, 2022

Choose a reason for hiding this comment

alexandr-shegeda commented Jul 13, 2022

supertopher left a comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

benmoriceau commented Jul 13, 2022 • edited Loading

supertopher left a comment

Choose a reason for hiding this comment

alexandr-shegeda commented Jul 6, 2022 •

edited

Loading

kimerinn commented Jul 8, 2022 •

edited

Loading

benmoriceau commented Jul 13, 2022 •

edited

Loading