-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🐛 source-mysql Support special chars in dbname #34580
Changes from all commits
f875c01
247a775
b3317ae
05277a3
d81db6b
1adb231
8f037ea
5451425
5b54342
7b0b608
c7b8682
09ed4ff
2efa38b
c2532a1
68a2c88
e1fdf69
2218719
27c2ee5
3168937
263adb9
b0ba72a
173d308
93c536d
53ac16b
e55f417
d43eb6f
5198b54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
version=0.20.2 | ||
version=0.20.3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
|
||
import com.fasterxml.jackson.databind.JsonNode; | ||
import io.airbyte.protocol.models.v0.ConfiguredAirbyteCatalog; | ||
import io.debezium.spi.common.ReplacementFunction; | ||
import java.util.Optional; | ||
import java.util.Properties; | ||
|
||
|
@@ -76,15 +77,42 @@ public Properties getDebeziumProperties( | |
props.setProperty("max.queue.size.in.bytes", BYTE_VALUE_256_MB); | ||
|
||
// WARNING : Never change the value of this otherwise all the connectors would start syncing from | ||
// scratch | ||
props.setProperty(TOPIC_PREFIX_KEY, getName(config)); | ||
// scratch. | ||
props.setProperty(TOPIC_PREFIX_KEY, sanitizeTopicPrefix(getName(config))); | ||
|
||
// includes | ||
props.putAll(getIncludeConfiguration(catalog, config)); | ||
|
||
return props; | ||
} | ||
|
||
public static String sanitizeTopicPrefix(final String topicName) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit : I think you could simplify using regex: `public static String sanitizeTopicPrefix(final String topicName) {
} but will leave it to you, I haven't really tested this (ChatGPT generated 😅 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I borrowed it from the following and there is no good way to directly use them though: And I guess we want to keep them the same. I'll leave some breadcrumb in the comment for better readability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akashkulk I do prefer the replaceAll as well. It's simpler, and we'll use it infrequently enough that perf don't matter |
||
StringBuilder sanitizedNameBuilder = new StringBuilder(topicName.length()); | ||
boolean changed = false; | ||
|
||
for (int i = 0; i < topicName.length(); ++i) { | ||
char c = topicName.charAt(i); | ||
if (isValidCharacter(c)) { | ||
sanitizedNameBuilder.append(c); | ||
} else { | ||
sanitizedNameBuilder.append(ReplacementFunction.UNDERSCORE_REPLACEMENT.replace(c)); | ||
changed = true; | ||
} | ||
} | ||
|
||
if (changed) { | ||
return sanitizedNameBuilder.toString(); | ||
} else { | ||
return topicName; | ||
} | ||
} | ||
|
||
// We need to keep the validation rule the same as debezium engine, which is defined here: | ||
// https://github.com/debezium/debezium/blob/c51ef3099a688efb41204702d3aa6d4722bb4825/debezium-core/src/main/java/io/debezium/schema/AbstractTopicNamingStrategy.java#L178 | ||
private static boolean isValidCharacter(char c) { | ||
return c == '.' || c == '_' || c == '-' || c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' || c >= '0' && c <= '9'; | ||
} | ||
|
||
protected abstract Properties getConnectionConfiguration(final JsonNode config); | ||
|
||
protected abstract String getName(final JsonNode config); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Copyright (c) 2023 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.integrations.source.mysql; | ||
|
||
import io.airbyte.integrations.source.mysql.MySQLTestDatabase.BaseImage; | ||
import io.airbyte.integrations.source.mysql.MySQLTestDatabase.ContainerModifier; | ||
|
||
public class CdcMysqlSourceWithSpecialDbNameTest extends CdcMysqlSourceTest { | ||
|
||
public static final String INVALID_DB_NAME = "invalid@name"; | ||
|
||
@Override | ||
protected MySQLTestDatabase createTestDatabase() { | ||
return MySQLTestDatabase.inWithDbName(BaseImage.MYSQL_8, INVALID_DB_NAME, ContainerModifier.INVALID_TIMEZONE_CEST, ContainerModifier.CUSTOM_NAME) | ||
.withCdcPermissions(); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any potential re-sync issues with existing syncs? I assume that because in these cases syncs are already in a broken state it shouldn't be an issue, but wanted to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I'm thinking too. The reformatting code is just borrowed from debezium sanitize code so I would expect them to recognize invalid name in the same way.