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.

Sign up for GitHub

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

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

Merged
merged 11 commits into from
Aug 6, 2018
Merged

ddl:In non-strict sql mode, fix column type is text /blob/json default value null not processed. #7230

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fbbdde1
in non-strict SQL mode,fix column type is text /blob/json default val…
ciscoxll Aug 1, 2018
7a57b63
update comments
ciscoxll Aug 1, 2018
dbc1ac1
address comments
ciscoxll Aug 1, 2018
cc0babc
address comments
ciscoxll Aug 1, 2018
40aad3e
address comments
ciscoxll Aug 2, 2018
a3ab51a
Merge branch 'master' into sql-mode-compatible
ciscoxll Aug 2, 2018
eaa5850
address comments
ciscoxll Aug 3, 2018
69ae665
address comments
ciscoxll Aug 3, 2018
3b25e37
add testCheckColumnDefaultValue test
ciscoxll Aug 3, 2018
2226cb5
address comments
ciscoxll Aug 6, 2018
8916387
Merge branch 'master' into sql-mode-compatible
winkyao Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions ddl/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2145,6 +2145,50 @@ func (s *testDBSuite) TestYearTypeCreateTable(c *C) {
c.Assert(mysql.HasUnsignedFlag(yearCol.Flag), IsFalse)
}

func (s *testDBSuite) TestCheckColumnCantHaveDefaultValue(c *C) {
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("use test;")
s.tk.MustExec("drop table if exists text_default_text;")
s.testErrorCode(c, "create table text_default_text(c1 text not null default '');", tmysql.ErrBlobCantHaveDefault)
s.testErrorCode(c, "create table text_default_text(c1 text not null default 'scds');", tmysql.ErrBlobCantHaveDefault)

s.tk.MustExec("drop table if exists text_default_json;")
s.testErrorCode(c, "create table text_default_json(c1 json not null default '');", tmysql.ErrBlobCantHaveDefault)
s.testErrorCode(c, "create table text_default_json(c1 json not null default 'dfew555');", tmysql.ErrBlobCantHaveDefault)

s.tk.MustExec("drop table if exists text_default_blob;")
s.testErrorCode(c, "create table text_default_blob(c1 blob not null default '');", tmysql.ErrBlobCantHaveDefault)
s.testErrorCode(c, "create table text_default_blob(c1 blob not null default 'scds54');", tmysql.ErrBlobCantHaveDefault)

s.tk.MustExec("set sql_mode='';")
s.tk.MustExec("drop table if exists text_default_text;")
s.tk.MustExec("create table text_default_text(c1 text not null default '');")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test about show create table text_default_text and show create table test_text_default_value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@winkyao Done.

s.tk.MustQuery(`show create table text_default_text`).Check(testutil.RowsWithSep("|",
""+
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line?

"text_default_text CREATE TABLE `text_default_text` (\n"+
" `c1` text NOT NULL\n"+
") ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin",
))

s.tk.MustExec("drop table if exists text_default_blob;")
s.tk.MustExec("create table text_default_blob(c1 blob not null default '');")
s.tk.MustQuery(`show create table text_default_blob`).Check(testutil.RowsWithSep("|",
""+
"text_default_blob CREATE TABLE `text_default_blob` (\n"+
" `c1` blob NOT NULL\n"+
") ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin",
))

s.tk.MustExec("drop table if exists text_default_json;")
s.tk.MustExec("create table text_default_json(c1 json not null default '');")
s.tk.MustQuery(`show create table text_default_json`).Check(testutil.RowsWithSep("|",
""+
"text_default_json CREATE TABLE `text_default_json` (\n"+
" `c1` json NOT NULL DEFAULT 'null'\n"+
") ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin",
))
}

func (s *testDBSuite) TestCharacterSetInColumns(c *C) {
s.tk = testkit.NewTestKit(c, s.store)
s.tk.MustExec("drop database if exists varchar_test;")
Expand Down
29 changes: 22 additions & 7 deletions ddl/ddl_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,24 @@ func buildColumnAndConstraint(ctx sessionctx.Context, offset int,

// checkColumnCantHaveDefaultValue checks the column can have value as default or not.
// Now, TEXT/BLOB/JSON can't have not null value as default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment... in strict mode, TEXT/BLOB/JSON can't have not null value as default.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, this function signature is very strange:
why checkColumnCantHaveDefaultValue return a boolean value hasDefaultValue

Copy link
Contributor

Choose a reason for hiding this comment

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

Do agree with tiancaiamao.

func checkColumnCantHaveDefaultValue(col *table.Column, value interface{}) (err error) {
func checkColumnCantHaveDefaultValue(ctx sessionctx.Context, col *table.Column, value interface{}) (bool, error) {
hasDefaultValue := true
if value != nil && (col.Tp == mysql.TypeJSON ||
col.Tp == mysql.TypeTinyBlob || col.Tp == mysql.TypeMediumBlob ||
col.Tp == mysql.TypeLongBlob || col.Tp == mysql.TypeBlob) {
if !ctx.GetSessionVars().SQLMode.HasStrictMode() && value == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen when SQL mode change, create a table with SQLMode A and use it wit SQLMode B ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In non-strict SQL mode, if the default value of the column is an empty string, can ignore it, the table can be created successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand.

// In non-strict SQL mode, if the default value of the column is an empty string, can ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/can ignore it/the default value can be ignored

if col.Tp == mysql.TypeBlob || col.Tp == mysql.TypeLongBlob && value == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

&& value == "" is redundant because it has been tested at line 261

hasDefaultValue = false
}
sc := ctx.GetSessionVars().StmtCtx
sc.AppendWarning(errBlobCantHaveDefault.GenByArgs(col.Name.O))
return hasDefaultValue, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

You can return a specific error here, then remove hasDefaultValue. You can test the specific error in the caller to set the hasDefaultValue.

}
// TEXT/BLOB/JSON can't have not null default values.
return errBlobCantHaveDefault.GenByArgs(col.Name.O)
return hasDefaultValue, errBlobCantHaveDefault.GenByArgs(col.Name.O)
}
return nil
return hasDefaultValue, nil
}

// isExplicitTimeStamp is used to check if explicit_defaults_for_timestamp is on or off.
Expand Down Expand Up @@ -326,11 +336,14 @@ func columnDefToCol(ctx sessionctx.Context, offset int, colDef *ast.ColumnDef, o
if err != nil {
return nil, nil, ErrColumnBadNull.Gen("invalid default value - %s", err)
}
if err = checkColumnCantHaveDefaultValue(col, value); err != nil {
if hasDefaultValue, err = checkColumnCantHaveDefaultValue(ctx, col, value); err != nil {
return nil, nil, errors.Trace(err)
}
col.DefaultValue = value
hasDefaultValue = true
// In non-strict SQL mode, if the column type is json and the default value is null, it is initialized to an empty array.
if col.Tp == mysql.TypeJSON && value == "" {
col.DefaultValue = `null`
}
removeOnUpdateNowFlag(col)
case ast.ColumnOptionOnUpdate:
// TODO: Support other time functions.
Expand Down Expand Up @@ -1462,11 +1475,13 @@ func setDefaultAndComment(ctx sessionctx.Context, col *table.Column, options []*
if err != nil {
return ErrColumnBadNull.Gen("invalid default value - %s", err)
}
if err = checkColumnCantHaveDefaultValue(col, value); err != nil {
if hasDefaultValue, err = checkColumnCantHaveDefaultValue(ctx, col, value); err != nil {
return errors.Trace(err)
}
col.DefaultValue = value
hasDefaultValue = true
if col.Tp == mysql.TypeJSON && value == "" {
col.DefaultValue = `null`
}
case ast.ColumnOptionComment:
err := setColumnComment(ctx, col, opt)
if err != nil {
Expand Down