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

bugfix: error image when use null value as image query condition in insert on duplicate #704 #725

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AsterZephyr
Copy link

What this PR does:
This PR addresses the SQL generation and parameter handling issues in the MySQL Insert On Duplicate Key Update undo log builder. It improves the handling of primary key conditions, parameter type safety, and fixes spacing issues in SQL generation.

Which issue(s) this PR fixes:
Fixes #704

Special notes for your reviewer:

  • The changes focus on improving internal logic and maintaining backward compatibility.
  • Although extensive changes were made, they are confined to the MySQL undo log builder and should not affect other components.
  • The original code had deficiencies in SQL generation and parameter handling, which could lead to errors when dealing with complex MySQL Insert On Duplicate Key Update scenarios. Refactoring improves code readability and maintainability.
  • To support more edge cases and complex multi-row insert scenarios, the existing logic needed to be extended and optimized.
  • The previous implementation had some known bugs, and this modification aims to resolve these issues to ensure correct functionality.
  • Although performance has not been fully optimized, improving logic and reducing unnecessary checks can enhance performance to some extent.
  • Improved handling of parameter types ensures consistency and safety across different database operations.

Through these improvements, I aim to provide a more robust and efficient solution to meet the project's development needs. If you encounter any issues with the refactoring, please feel free to contact me.

Does this PR introduce a user-facing change?:

NONE

@github-actions github-actions bot added the coding label Dec 2, 2024
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

if len(paramMap) == 0 || len(metaData.Indexs) == 0 {
return "", nil, nil
}
hasPK := false
Copy link
Member

Choose a reason for hiding this comment

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

use goto

Copy link
Contributor

Choose a reason for hiding this comment

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

use goto

goto 不太好理解?

Copy link
Author

Choose a reason for hiding this comment

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

use goto

goto 不太好理解?

那这里还需要改为goto吗

Copy link
Contributor

Choose a reason for hiding this comment

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

不要使用 goto 吧


// Reset primary keys map
u.BeforeImageSqlPrimaryKeys = make(map[string]bool)

pkIndexMap := u.getPkIndex(insertStmt, metaData)
var pkIndexArray []int
Copy link
Member

Choose a reason for hiding this comment

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

use make and cap

for i := 0; i < len(insertRows); i++ {
var rowConditions []string
var rowArgs []driver.Value
usedParams := make(map[string]bool)
Copy link
Member

Choose a reason for hiding this comment

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

add cap

@luky116
Copy link
Contributor

luky116 commented Dec 5, 2024

把单测也加上

// Test case for null unique index
{
execCtx: &types.ExecContext{
Query: "insert into t_unique(id, a, b) values(1, NULL, 2) on duplicate key update b = 5",
Copy link
Contributor

Choose a reason for hiding this comment

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

values(1, NULL, 2) 换成占位符 ?

{
execCtx: &types.ExecContext{
Query: "insert into t_unique(id, a, b) values(1, NULL, 2) on duplicate key update b = 5",
MetaDataMap: map[string]types.TableMeta{"t_unique": tableMeta1},
Copy link
Contributor

Choose a reason for hiding this comment

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

MetaDataMap 会在后面使用,确保这个 key 被用到,可以 debug 看看是否走到了你预想的步骤

{
execCtx: &types.ExecContext{
Query: "insert into t_unique(id, b) values(NULL, 2) on duplicate key update b = 5",
MetaDataMap: map[string]types.TableMeta{"t_unique": tableMeta1},
Copy link
Contributor

Choose a reason for hiding this comment

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

t_unique 改为 t_user

Code-Fight
Code-Fight previously approved these changes Dec 21, 2024
Copy link
Contributor

@Code-Fight Code-Fight left a comment

Choose a reason for hiding this comment

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

LGTM

@luky116
Copy link
Contributor

luky116 commented Dec 21, 2024

单纯从语义来看,这样会导致出问题。如果他的联合索引是 "id", "name", "age",那么他生成的 SELECT SQL 也应该包含这三字段,如果使用其中部分字段,生成的 SELECT 没办法保证获取数据的唯一性

image

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

Successfully merging this pull request may close these issues.

[seata-java] bugfix: error image when use null value as image query condition in insert on duplicate
4 participants