From 6ffbfd17663d406f05d627be8307a05c5113eee0 Mon Sep 17 00:00:00 2001 From: Andreas Skorczyk Date: Thu, 16 Dec 2021 11:12:07 +0100 Subject: [PATCH 1/2] Skip entries that don't match schema on import For some reason, it's possible that a table contains NULL entries, even if the schema doesn't allow for that. Currently, an import would then fail with a `Scan error`. As the entry anyway is not valid, we can also simply skip it and continue with the import as usual. This PR adds a `isNullableMismatch` function, which can check if a given error is due to a mismatch of a NULL entry and a NOT-NULL schema. If we detect this kind of issue while scanning a table, we don't need to fail, but can simply skip to the next entry. Fixes #115 --- model/Database.go | 66 ++++++++++++++- model/Database_test.go | 162 +++++++++++++++++++++++++++++++++++- model/manifest_test.go | 2 +- model/testdata/user_data.db | Bin 159744 -> 163840 bytes 4 files changed, 224 insertions(+), 6 deletions(-) diff --git a/model/Database.go b/model/Database.go index 1269bd3..135bf89 100644 --- a/model/Database.go +++ b/model/Database.go @@ -9,12 +9,15 @@ import ( "os" "path/filepath" "reflect" + "regexp" + "strconv" "sync" "time" "github.com/davecgh/go-spew/spew" "github.com/pkg/errors" "github.com/sergi/go-diff/diffmatchpatch" + log "github.com/sirupsen/logrus" // Register SQLite driver _ "github.com/mattn/go-sqlite3" @@ -394,7 +397,15 @@ func fetchFromSQLite(sqlite *sql.DB, modelType Model) ([]Model, error) { } mn, err := m.scanRow(rows) if err != nil { - return nil, errors.Wrap(err, "Error while scanning results from SQLite database") + // For some reason a row might contain NULL entries, even though the schema + // shouldn't allow this. Instead of failing the whole import, we can simply skip + // this entry as the data anyway wouldn't be valid. + if !isNullableMismatch(err, rows) { + return nil, errors.Wrap(err, "Error while scanning row") + } + log.Warnf("Nullable mismatch in %T at index %d detected. Skipping entry", m, i) + i++ + continue } result[mn.ID()] = mn i++ @@ -407,6 +418,53 @@ func fetchFromSQLite(sqlite *sql.DB, modelType Model) ([]Model, error) { return result, nil } +// isNullableMismatch checks if a given error is due to a NULL entry in a column that only allows +// non-NULL entries. If this is the case and no other schema mismatch is detected, true is returned. +func isNullableMismatch(err error, rows *sql.Rows) bool { + if err == nil || rows == nil { + return false + } + + re := regexp.MustCompile(`Scan error on column index (\d+), name "(\w+)": converting NULL to (\w+) is unsupported`) + matches := re.FindStringSubmatch(err.Error()) + if len(matches) != 4 { + return false + } + + index, err := strconv.ParseInt(matches[1], 0, 64) + if err != nil { + return false + } + + ct, err := rows.ColumnTypes() + if err != nil { + return false + } + + if len(ct) <= int(index) { + return false + } + + column := ct[index] + if column == nil { + return false + } + + if column.Name() != matches[2] { + return false + } + + if typeName, ok := dbTypeToGoType[column.DatabaseTypeName()]; !ok || typeName != matches[3] { + return false + } + + if n, _ := column.Nullable(); n { + return false + } + + return true +} + // getTableEntryCount returns the number of entries in a given table func getTableEntryCount(sqlite *sql.DB, tableName string) (int, error) { var count int @@ -625,3 +683,9 @@ func createEmptySQLiteDB(filename string) error { return nil } + +// maps a DatabaseTypeName to a go type name +var dbTypeToGoType = map[string]string{ + "INTEGER": "int", + "TEXT": "string", +} diff --git a/model/Database_test.go b/model/Database_test.go index 723d854..2d6fb77 100644 --- a/model/Database_test.go +++ b/model/Database_test.go @@ -148,7 +148,7 @@ func Test_fetchFromSQLite(t *testing.T) { bookmark, err := fetchFromSQLite(sqlite, &Bookmark{}) assert.NoError(t, err) - assert.Len(t, bookmark, 3) + assert.Len(t, bookmark, 4) assert.Equal(t, &Bookmark{2, 3, 7, 4, "Philippians 4", sql.NullString{String: "12 I know how to be low on provisions and how to have an abundance. In everything and in all circumstances I have learned the secret of both how to be full and how to hunger, both how to have an abundance and how to do without. ", Valid: true}, 0, sql.NullInt32{}}, bookmark[2]) inputField, err := fetchFromSQLite(sqlite, &InputField{}) @@ -173,7 +173,7 @@ func Test_fetchFromSQLite(t *testing.T) { tagMap, err := fetchFromSQLite(sqlite, &TagMap{}) assert.NoError(t, err) - assert.Len(t, tagMap, 3) + assert.Len(t, tagMap, 4) assert.Equal(t, &TagMap{2, sql.NullInt32{Int32: 0, Valid: false}, sql.NullInt32{Int32: 0, Valid: false}, sql.NullInt32{Int32: 2, Valid: true}, 2, 1}, tagMap[2]) userMark, err := fetchFromSQLite(sqlite, &UserMark{}) @@ -182,6 +182,160 @@ func Test_fetchFromSQLite(t *testing.T) { assert.Equal(t, &UserMark{2, 1, 2, 0, "2C5E7B4A-4997-4EDA-9CFF-38A7599C487B", 1}, userMark[2]) } +func Test_isNullableMismatch(t *testing.T) { + type args struct { + err error + rows func() *sql.Rows + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "Nullable mismatch with int", + args: args{ + err: fmt.Errorf(`Scan error on column index 2, name "NotNullable": converting NULL to int is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("INTEGER", nil)) + }, + }, + want: true, + }, + { + name: "Nullable mismatch with text", + args: args{ + err: fmt.Errorf(`Scan error on column index 1, name "NotNullable": converting NULL to string is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("NotNullable").OfType("TEXT", nil), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true)) + }, + }, + want: true, + }, + { + name: "No nullable mismatch", + args: args{ + err: fmt.Errorf(`Scan error on column index 1, name "Nullable": converting NULL to int is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("INTEGER", 3)) + }, + }, + want: false, + }, + { + name: "Empty", + }, + { + name: "Different error", + args: args{ + err: fmt.Errorf(`Scan error on column index 1, name "Nullable": converting mock to int is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("INTEGER", 3)) + }, + }, + }, + { + name: "No error", + args: args{ + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("INTEGER", 3)) + }, + }, + }, + { + name: "No rows", + args: args{ + err: fmt.Errorf(`Scan error on column index 1, name "Nullable": converting NULL to int is unsupported`), + }, + }, + { + name: "Column name not matching column index", + args: args{ + err: fmt.Errorf(`Scan error on column index 2, name "WrongName": converting NULL to int is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("INTEGER", 3)) + }, + }, + want: false, + }, + { + name: "Type mismatch", + args: args{ + err: fmt.Errorf(`Scan error on column index 2, name "NotNullable": converting NULL to string is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("INTEGER", 3)) + }, + }, + want: false, + }, + { + name: "Negative index", + args: args{ + err: fmt.Errorf(`Scan error on column index -5, name "NotNullable": converting NULL to string is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("INTEGER", 1), + sqlmock.NewColumn("Nullable").OfType("INTEGER", sql.NullInt32{}).Nullable(true), + sqlmock.NewColumn("NotNullable").OfType("TEXT", nil)) + }, + }, + want: false, + }, + { + name: "More columns in error than in rows - don't panic with index out of range", + args: args{ + err: fmt.Errorf(`Scan error on column index 2, name "NotNullable": converting NULL to int is unsupported`), + rows: func() *sql.Rows { + return mockSQLRows(t, + sqlmock.NewColumn("ID").OfType("int", 1), + sqlmock.NewColumn("Nullable").OfType("int", sql.NullInt32{}).Nullable(true)) + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.args.rows == nil { + tt.args.rows = func() *sql.Rows { return nil } + } + got := isNullableMismatch(tt.args.err, tt.args.rows()) + assert.Equal(t, tt.want, got) + }) + } +} + +func mockSQLRows(t *testing.T, columns ...*sqlmock.Column) *sql.Rows { + db, mock, err := sqlmock.New() + assert.NoError(t, err) + defer db.Close() + + mock.ExpectQuery("Placeholder").WillReturnRows(mock.NewRowsWithColumnDefinition(columns...)) + result, err := db.Query("Placeholder") + assert.NoError(t, err) + return result +} + func TestDatabase_importSQLite(t *testing.T) { db := Database{} @@ -191,12 +345,12 @@ func TestDatabase_importSQLite(t *testing.T) { // As we already test the correctness in Test_fetchFromSQLite, // it should be sufficient to just double-check the size of the slices. assert.Len(t, db.BlockRange, 5) - assert.Len(t, db.Bookmark, 3) + assert.Len(t, db.Bookmark, 4) assert.Len(t, db.InputField, 4) assert.Len(t, db.Location, 9) assert.Len(t, db.Note, 3) assert.Len(t, db.Tag, 3) - assert.Len(t, db.TagMap, 3) + assert.Len(t, db.TagMap, 4) assert.Len(t, db.UserMark, 5) path = filepath.Join("testdata", "error_playlistMedia.db") diff --git a/model/manifest_test.go b/model/manifest_test.go index bd9e4fe..05beb3a 100644 --- a/model/manifest_test.go +++ b/model/manifest_test.go @@ -14,7 +14,7 @@ var exampleManifest = &manifest{ CreationDate: time.Now().Format("2006-01-02"), UserDataBackup: userDataBackup{ LastModifiedDate: time.Now().Format("2006-01-02T15:04:05-07:00"), - Hash: "55e87dda924edb6a6c93871ee3c642119da4ade73fab05049bbd32ee5411dfeb", + Hash: "19f994bb2fcda04e213920ddee729337296d90cad108bf281c6f6bbf9f2e5fce", DatabaseName: "user_data.db", SchemaVersion: 8, DeviceName: "go-jwlm", diff --git a/model/testdata/user_data.db b/model/testdata/user_data.db index 6e82be9ecb5435a6b3d48af2572782ea5c09a7cd..d95baa66e455607634abb0e07a77a3b48c9b11df 100755 GIT binary patch delta 942 zcmZ`&T}YEr7(Va!e~$AzTB|wAT$6ORrJ{6`SX~G)%^$FfAVOCgu`pM-pg?dVqP#1F zK7o%1sp_CG0WWJp~ z3QXoJgs<=|zMd&R`%qsAS0MJi(67ePhTWj3ChJyq&`LP;Aj zCrr0CT{=%E*$Zh)dW)gLKkb8x|qKE*veM16HF0o@0fGue;0VFNXcTfH;Mt{<`$Rpzi7Q zLb}r$b)eqr$U=oR>p*48j@t8ytC3_pF^CPJ!i?gfCovQkVVuH_>z)pr(punRwK`Cx z7qO}Zg~kFNk`u^XiDYcgN%r_pxB?2>AT!pVB}j=kAGYo}(8kjKknXvJAeSv~7Md*E zL8^f)R9dMlC`~)~_6$Vs4#bC&;biQ_o=CwuD@CE2H5Yl71Ovr;Q}ND%wc;K$P%*-Y zozDV2i7Hzne2M?&IaI|*Zs;m9M44H+KFN()hj0h8YnYwikzG3@?cbek+3rPO(5QwU zMJmw#ePX3{n$RyU3vNrfZ-(25SyuMsCBB!;bA;!3p8rDNPy7Ita5oeg1*^(J*NO~O F`U@!8@ihPd delta 618 zcmZuu&ubGw6rMNP?d(jlJE;okTA^DjYH6sbcu~+w1quF&*h4{~jV6>xO_b!Ipf-Cb z;!Wvc%td<7p@blN>%~7ndk_T=3Z`2(x#R~G>>hNsA)wHK;l1~L-<$X5y;6ylzSCF3 zgHs5hWAgUT?CbbxbqY;RzptiOPiWYvBkmgYs{O^N)z4d?VctP}hj(g&A6;JS+}LMH zsEPP5-jvWczS3MWCDIK9y{fksL81 zNJp2Dv1%M(oBCH;Ve9HLeNJ!EBX|dIyt5#qDlZNZ;=EKL4$j&S$YVUgM6Mx28kjM$ z-jI!GyS{dECbe)a^^iIh3THuCE`*5VF&O$3Oe0Flh=1U3ji<(C{d2^R+z&Ux^UP-z z#_5vgYb&H&wI30uyajhbvEeWge||!V+4?An-7>*GC!V=5DI5=?BI(MaZ#*!?9S_1s zujcQiirIWFk&aukiNGZDg=_$g+onZgw`KknvEjifQE_2N3^!m@%zHqllZ&Y*i`hak zQOrEpEB*S&Ky1Q-{k>iFD5&6Bk{u^j>g$ml$_^IhQkXY1@eVd|NKf q<1sZ9EJX=He Date: Sun, 26 Dec 2021 13:33:00 +0100 Subject: [PATCH 2/2] fetchFromSQLite: Make clear what type errors This adds the actual type of the model that caused the error, which should make debugging a bit easier. --- model/Database.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/Database.go b/model/Database.go index 135bf89..4c66490 100644 --- a/model/Database.go +++ b/model/Database.go @@ -401,7 +401,7 @@ func fetchFromSQLite(sqlite *sql.DB, modelType Model) ([]Model, error) { // shouldn't allow this. Instead of failing the whole import, we can simply skip // this entry as the data anyway wouldn't be valid. if !isNullableMismatch(err, rows) { - return nil, errors.Wrap(err, "Error while scanning row") + return nil, errors.Wrapf(err, "Error while scanning row for %T", modelType) } log.Warnf("Nullable mismatch in %T at index %d detected. Skipping entry", m, i) i++ @@ -412,7 +412,7 @@ func fetchFromSQLite(sqlite *sql.DB, modelType Model) ([]Model, error) { } err = rows.Err() if err != nil { - return nil, errors.Wrap(err, "Error while scanning results from SQLite database") + return nil, errors.Wrapf(err, "Error while scanning results for %T from SQLite database", modelType) } return result, nil