Don't convert a string to binary base64 unnecessarily #25586

rodireich · 2023-04-26T20:52:15Z

What

Mysql column that is encoded with any character set and collated (sorted) in a binary collation logic (e.g utf8mb4_bin) is today treated as a binary value and converted to base64 on detonation side.
This is not necessary as the value itself is still a simple string.
We need to treat it as a string and create one on destination.

How

Our code is checking incorrectly the .isBinary() property of a column when deciding how to convert it.
Remove this logic when copying to json.

🚨 User Impact 🚨

Any string value previously synced as base64 will remain so until a reset or a full refresh.
Newly synced values will appear as the original on source side ("abcd").

Resolves #9938 .

rodireich · 2023-04-26T20:53:21Z

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/4813268088
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/4813268088
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================= 22 passed, 6 skipped, 30 warnings in 18.49s ==================

github-actions · 2023-04-26T20:56:11Z

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

Run integration tests
Bump connector or module version
Add changelog
Publish the new version

❌ Sources (2)

Connector	Version	Changelog	Publish
`source-mysql`	`2.0.23`	✅	❌ (diff seed version)
`source-mysql-strict-encrypt`	`2.0.23`	🔵 (ignored)	🔵 (ignored)

See "Actionable Items" below for how to resolve warnings and errors.

✅ Destinations (0)

Connector	Version	Changelog	Publish

See "Actionable Items" below for how to resolve warnings and errors.

✅ Other Modules (0)

Actionable Items

(click to expand)

Category	Status	Actionable Item
Version	❌ mismatch	The version of the connector is different from its normal variant. Please bump the version of the connector.
	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
Changelog	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
	❌ changelog missing	There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish	⚠ not in seed	The connector is not in the cloud or oss registry, so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that you have added a metadata.yaml file and the expected registries are enabled.

prateekmukhedkar · 2023-05-01T05:53:19Z

...s/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSourceOperations.java

-          putString(json, columnName, resultSet, colIndex);
-        }
-      }
+      case CHAR, VARCHAR -> putString(json, columnName, resultSet, colIndex);


Do we need a test case for binary fields to ensure that conversion to string logic is taking place as expected?

This code change is putting us back to handling VARCHAR the same way under any charset or collation.

We don't have tests for variation of charset as far as I can tell, but the change itself is returning to the way this used to behave before checking for .isBinary() was added.

edgao · 2023-05-01T17:33:30Z

checking my understanding:

current behavior is that we discover these columns as string and read records as base64encode(select the_column)
new behavior is still discovering string, but no longer base64 encoding the data?

this sounds like a breaking change. Do we need any coordination to release this? Should we recommend that users run a reset after upgrading? (or they can manually rewrite their raw tables + rebuild normalized tables I guess, but that sounds complicated)

rodireich · 2023-05-01T23:02:29Z

What you describe @edgao is correct - that's the change.
But I thought of it as non breaking as old values that already synced to destination as base64 would not affect new values synced as a simple string.
Do you think we should flag this beforehand to customers so they can be aware and reset or re-run normalization ?

edgao · 2023-05-02T16:42:49Z

presumably there are some folks who need to manually base64-decode their data right now; we should at least tell them to stop doing that decode.

does our schema include anything like format: base64? I.e. can we continue reading base64 for existing users, but new users (or after schema refresh) start reading normal strings?

rodireich · 2023-05-06T00:53:33Z

does our schema include anything like format: base64

I don't think that's the case.
The column in the case above was marked correctly as a string but its value was treated like a binary.
Note that the only thing binary here was the column collation order. not any encoding or data.

I'm not sure what can be done other than notifying customers that may have reverse conversion on they destination.
I'm not sure we can even check for existence of such cloud connections as the type is string.

edgao · 2023-05-08T14:58:08Z

sorry, what does this mean?

the only thing binary here was the column collation order. not any encoding or data

rodireich · 2023-05-08T17:03:05Z

The case here is db with charset encoding of utf8,
and a collation (or sorting) order of utf8_bin.

CREATE TABLE `INV_LOT_LOC_ID` ( … ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin

The data is encoded in plain utf8 but when sorted for example in ORDER BY, binary logic is used which is comparing byte to byte values rather than go into all the intricacies of variable length multibyte utf8 characters.
For example utf8 aware sorting may treat ß as equal to ss, while binary sorting would place ß after z

Not to be confused with Binary encoding that has its own "binary" collation order which is unrelated to this case and will still get destination strings converted to base64 as it should.
See https://dev.mysql.com/doc/refman/5.7/en/charset-binary-collations.html

edgao

ah got it :/ that seems challenging to coordinate then, if we can't even recognize which workspaces are affected...

is our plan to just tell all source-mysql users something like "check your source DB for columns with COLLATE=utf8_bin and run a reset if you have them?" (or maybe "update your query to attempt base64_decode, and fallback to the raw value if that decode fails"?)

approving since I have no opinion on the code change, but I think the release plan needs some clarification. (destinations has no problems with this, since the schema was and will remain type: string)

rodireich · 2023-05-08T19:23:00Z

Thanks Ed
And this is largely the plan - alert users to check if their connection is affected. Thanks for suggesting fallback in case a decode fails. that should probably solve it for most.
I will coordinate with cloud and community support to send out a message.

rodireich · 2023-05-08T21:04:04Z

Airbyte technical support:
Once this change goes in, previously synced mysql-source connections that are syncing:
A database with any charset encoding which is set with an *_bin collation order (sorting)
For example hebrew charset with hebrew_bin collation.

Will see the following potentially breaking change:
Strings that were previously encoded on destination in base64 will now show up in their regular form 'abcd' rather than 'YWJjZA=='.
This means that a table that was previously synced and has VARCHAR columns converted to base64 will now have new values synced as simple strings.

What can be done:
a. Reset of table data on airbyte will get a fresh copy of all rows with now plain 'abcd' string.
b. If there's any script or procedure on destination that is consuming these base64 encoded columns, it should be updated to also be able to process plain strings (e.g if base64 decode failed use the string as is).

Note: the issue won't affect anybody who is running with default settings but only customers that intentionally created a binary collated DB or tables or columns.

cc: @erica-airbyte

erica-airbyte · 2023-05-08T21:06:27Z

Thanks @rodireich can you fill out the breaking changes doc?

rodireich · 2023-05-08T21:39:14Z

See breaking change doc for this change

amaliaroye · 2023-05-23T16:49:52Z

Hi @rodireich, is this still scheduled to be released tomorrow, May 24th?

rodireich · 2023-05-24T15:57:46Z

/connector-performance connector=connectors/source-mysql

rodireich · 2023-05-24T16:00:19Z

/connector-performance connector=connectors/source-mysql ref=master

* Don't convert a string to binary base64 unnecessarily * update doc * update doc

Don't convert a string to binary base64 unnecessarily

8269bed

rodireich requested a review from a team April 26, 2023 20:52

rodireich requested a review from a team as a code owner April 26, 2023 20:52

rodireich linked an issue Apr 26, 2023 that may be closed by this pull request

Source MySQL: support utf8_bin charset #9938

Closed

octavia-squidington-iii added area/connectors Connector related issues connectors/source/mysql labels Apr 26, 2023

update doc

da8f568

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 26, 2023

update doc

1f5562e

prateekmukhedkar reviewed May 1, 2023

View reviewed changes

edgao approved these changes May 8, 2023

View reviewed changes

prateekmukhedkar approved these changes May 8, 2023

View reviewed changes

rodireich added 2 commits May 24, 2023 07:05

Merge branch 'master' into 9938-source-mysql-support-utf8_bin-charset

3c0ae45

Bump version and update note

4674edf

rodireich enabled auto-merge (squash) May 24, 2023 14:11

octavia-squidington-iii added the connectors/source/mysql-strict-encrypt label May 24, 2023

rodireich merged commit 804087a into master May 24, 2023
23 checks passed

rodireich deleted the 9938-source-mysql-support-utf8_bin-charset branch May 24, 2023 14:33

nguyenaiden pushed a commit that referenced this pull request May 25, 2023

Don't convert a string to binary base64 unnecessarily (#25586)

c1e67df

* Don't convert a string to binary base64 unnecessarily * update doc * update doc

marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023

Don't convert a string to binary base64 unnecessarily (airbytehq#25586)

df8e376

* Don't convert a string to binary base64 unnecessarily * update doc * update doc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't convert a string to binary base64 unnecessarily #25586

Don't convert a string to binary base64 unnecessarily #25586

rodireich commented Apr 26, 2023 •

edited

rodireich commented Apr 26, 2023 •

edited by github-actions bot

github-actions bot commented Apr 26, 2023 •

edited

❌ Sources (2)

✅ Destinations (0)

✅ Other Modules (0)

Actionable Items

prateekmukhedkar May 1, 2023

rodireich May 1, 2023

edgao commented May 1, 2023

rodireich commented May 1, 2023

edgao commented May 2, 2023

rodireich commented May 6, 2023 •

edited

edgao commented May 8, 2023

rodireich commented May 8, 2023 •

edited

edgao left a comment

rodireich commented May 8, 2023

rodireich commented May 8, 2023 •

edited

erica-airbyte commented May 8, 2023

rodireich commented May 8, 2023 •

edited

amaliaroye commented May 23, 2023

rodireich commented May 24, 2023

rodireich commented May 24, 2023

Don't convert a string to binary base64 unnecessarily #25586

Don't convert a string to binary base64 unnecessarily #25586

Conversation

rodireich commented Apr 26, 2023 • edited

What

How

🚨 User Impact 🚨

rodireich commented Apr 26, 2023 • edited by github-actions bot

Build Passed

github-actions bot commented Apr 26, 2023 • edited

Affected Connector Report

❌ Sources (2)

✅ Destinations (0)

✅ Other Modules (0)

Actionable Items

prateekmukhedkar May 1, 2023

Choose a reason for hiding this comment

rodireich May 1, 2023

Choose a reason for hiding this comment

edgao commented May 1, 2023

rodireich commented May 1, 2023

edgao commented May 2, 2023

rodireich commented May 6, 2023 • edited

edgao commented May 8, 2023

rodireich commented May 8, 2023 • edited

edgao left a comment

Choose a reason for hiding this comment

rodireich commented May 8, 2023

rodireich commented May 8, 2023 • edited

erica-airbyte commented May 8, 2023

rodireich commented May 8, 2023 • edited

amaliaroye commented May 23, 2023

rodireich commented May 24, 2023

rodireich commented May 24, 2023

rodireich commented Apr 26, 2023 •

edited

rodireich commented Apr 26, 2023 •

edited by github-actions bot

github-actions bot commented Apr 26, 2023 •

edited

rodireich commented May 6, 2023 •

edited

rodireich commented May 8, 2023 •

edited

rodireich commented May 8, 2023 •

edited

rodireich commented May 8, 2023 •

edited