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

Ban the connect attr interpolateParams for MySQL 8 Vis dbs #5441

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

tdeebswihart
Copy link
Contributor

What changed?

Attempting to configure a MySQL 8 Visibility database with go-sql-driver/mysql's interpolateParams option will now fail.

Why?

There are a number of reasons why this doesn't work right now so we're going to prevent it from happening while we discuss how much we should invest in fixing it. The current work to fix this is in #5428

How did you test it?

Added a new test

Potential risks

There should be none. Any user who has currently set up their database this way will already have encountered exciting errors like Cannot create a JSON value from a string with CHARACTER SET 'binary' and Unable to parse ExecutionStatus value from DB.

Is hotfix candidate?

That's up for discussion

Copy link
Collaborator

@rodrigozhou rodrigozhou left a comment

Choose a reason for hiding this comment

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

Please fix lints.

common/persistence/sql/sqlplugin/mysql/session/session.go Outdated Show resolved Hide resolved
@tdeebswihart tdeebswihart force-pushed the tds/prevent-mysql8-vis-interpolateParams branch from b9fa8cc to 302d324 Compare February 29, 2024 00:28
There are a number of reasons why this doesn't work right now so we're
going to prevent it from happening while we discuss whether we should
invest in fixing it. The current work to fix this is in #5428
@tdeebswihart tdeebswihart force-pushed the tds/prevent-mysql8-vis-interpolateParams branch from 302d324 to 768fd95 Compare March 6, 2024 22:57
@tdeebswihart tdeebswihart enabled auto-merge (squash) March 8, 2024 19:52
@tdeebswihart tdeebswihart merged commit cdbee39 into main Mar 8, 2024
42 checks passed
@tdeebswihart tdeebswihart deleted the tds/prevent-mysql8-vis-interpolateParams branch March 8, 2024 20:03
stephanos pushed a commit to stephanos/temporal that referenced this pull request Mar 21, 2024
…o#5441)

## What changed?
Attempting to configure a MySQL 8 Visibility database with
go-sql-driver/mysql's `interpolateParams` option will now fail.

## Why?

There are a number of reasons why this doesn't work right now so we're
going to prevent it from happening while we discuss how much we should
invest in fixing it. The current work to fix this is in
temporalio#5428

## How did you test it?
Added a new test

## Potential risks
There should be none. Any user who has currently set up their database
this way will already have encountered exciting errors like `Cannot
create a JSON value from a string with CHARACTER SET 'binary'` and
`Unable to parse ExecutionStatus value from DB`.

## Is hotfix candidate?

That's up for discussion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants