Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Fix inspector column types #661
Conversation
The given `columnLists` may not contain all columns available in the
given table. This patch prevents the code to fail. Before this patch the
code was panicking whenever it was trying to set a value into a `nil`
object, e.g. `columnList.GetColumn('non-existant').Type = SomeType`.
For some reason the application was not completely failing but as a
side-effect any column after the non-existant column would never get its
Type properly set.
There is no need to call `applyColumnTypes` more than once for the same `databaseName` and `tableName`, it is just move the additional `columnList` to the first call.
|
Thank you for this PR. Can you please clarify inline about the changes you made? Why the removal of Why the reverse of |
| @@ -173,8 +173,7 @@ func (this *Inspector) inspectOriginalAndGhostTables() (err error) { | |||
| // This additional step looks at which columns are unsigned. We could have merged this within | |||
| // the `getTableColumns()` function, but it's a later patch and introduces some complexity; I feel | |||
| // comfortable in doing this as a separate step. | |||
| this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns) | |||
| this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, &this.migrationContext.UniqueKey.Columns) | |||
| this.applyColumnTypes(this.migrationContext.DatabaseName, this.migrationContext.OriginalTableName, this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns, &this.migrationContext.UniqueKey.Columns) | |||
esnunes
Nov 1, 2018
Author
Contributor
This is just a small improvement, instead of calling applyColumnTypes twice for the same databaseName and tableName, I've moved the &this.migrationContext.UniqueKey.Columns to this call.
This is just a small improvement, instead of calling applyColumnTypes twice for the same databaseName and tableName, I've moved the &this.migrationContext.UniqueKey.Columns to this call.
| } | ||
| if strings.Contains(columnType, "mediumint") { | ||
| for _, columnsList := range columnsLists { | ||
| columnsList.GetColumn(columnName).Type = sql.MediumIntColumnType |
esnunes
Nov 1, 2018
Author
Contributor
This code fails when columnsList.GetColumn(columnName) returns nil. This is going to happen in this.migrationContext.MappedSharedColumns and this.migrationContext.SharedColumns due to the fact they they only have the intersection of original and ghost tables. If you add a new column by the end of the column list this problem won't be noticed as the code is going to fail when setting the type of the new column (which is not used during copy of data), but if you alter table and add a column in the second position, all other columns will have the default Type (0) because the code will not iterate over all of them.
This code fails when columnsList.GetColumn(columnName) returns nil. This is going to happen in this.migrationContext.MappedSharedColumns and this.migrationContext.SharedColumns due to the fact they they only have the intersection of original and ghost tables. If you add a new column by the end of the column list this problem won't be noticed as the code is going to fail when setting the type of the new column (which is not used during copy of data), but if you alter table and add a column in the second position, all other columns will have the default Type (0) because the code will not iterate over all of them.
| columnsList.SetUnsigned(columnName) | ||
| for _, columnsList := range columnsLists { | ||
| column := columnsList.GetColumn(columnName) | ||
| if column == nil { |
esnunes
Nov 1, 2018
Author
Contributor
The for loop was changed in order to prevent accessing a column that is not available in the columnsList object.
The for loop was changed in order to prevent accessing a column that is not available in the columnsList object.
esnunes
Nov 1, 2018
Author
Contributor
This code ignores columns that are not available in the columnsList and continues iterating over the next columns.
This code ignores columns that are not available in the columnsList and continues iterating over the next columns.
|
Hi @shlomi-noach , thanks for you fast reply. I've added some inline comments, I hope they make clear the intent of this PR. Let me know if you need any further clarification. This is a big issue for me as it prevented me to use |
|
Thank you for replying. I'll take this change to testing. It'll like take a few days or over a week for me to respond to this PR, given my current workload, sorry for the expected delay. |
|
No worries and thanks for the heads up regarding the time to release. |
|
Hey @shlomi-noach |
|
sent to production testing |
|
Meanwhile sorry for the delay. |
|
This looks good in testing and I'll merge this PR early next week. |
shlomi-noach
merged commit 17233fc
into
github:master
Issue
Related issue: #660
Description
This PR fix an issue in
Inspector.applyColumnTypes. The function fails when acolumnListdoes not contain all table columns. For some reason the code fails silently, so any other column after the first failure will be identified as type0(defaultintvalue).As a side-effect when copying rows from original table to
gh-osttemporary one, it may fail due to bad type conversion as describe in the related issue.Notes
I've considered adding tests but they would require extract the column type logic to a different function or add an external library to mock
sql.DB.