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

Fix bug affecting sanitization version of sync tables #15885

Merged
merged 4 commits into from
Oct 7, 2020
Merged

Fix bug affecting sanitization version of sync tables #15885

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54f424f
Fix bug affecting sanitization version of sync tables
jgoizueta Oct 6, 2020
1200543
Update NEWS
jgoizueta Oct 6, 2020
6d1a878
Choose a more idiomatic phrasing
jgoizueta Oct 6, 2020
7775556
Revert "Choose a more idiomatic phrasing"
jgoizueta Oct 7, 2020
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
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Development
- None yet

### Features
* Fix column sanitization for connector syncs ([15885](https://github.com/CartoDB/cartodb/pull/15885))
* Load config files as ERB templates to allow reading ENV values ([15881](https://github.com/CartoDB/cartodb/pull/15881))

### Bug fixes / enhancements
Expand Down
3 changes: 1 addition & 2 deletions app/models/synchronization/adapter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,7 @@ def import_cleanup(schema_name, table_name)
# but is kept because there may be existing syncs for which this double sanitization
# (version 1 sanitization which wasn't idemponent) had the effect of altering some
# satinizated names (e.g. __1 -> _1).
table = ::Table.new(name: @table_name, user_id: @user.id)
# we could as well: table = Carto::UserTable.find(@user.tables.where(name: @table_name).first.id).service
table = Carto::UserTable.find(@user.tables.where(name: @table_name).first.id).service
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace:

@user.tables.where(name: @table_name).first.id

per:

@user.tables.find_by(name: @table_name).id

It's a bit more idiomatic 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can affect some users with sync tables and break their visualizations, since column names where uppercase letters have been replaced by underscores will change. But we can prevent the users we know about (in the clubhouse ticket) and I think the alternative, to create a new sanitization version and preserve the odd behaviour in the current one, is worse.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thougt @user.tables returned an AR relationship, but per what I see in the CI log I think i was wrong:

Step #9: undefined method `find_by' for #<#<Class:0x000055f7aaf4eaf0>:0x000055f7aa1d9460>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had just found that: I tested it in a console and it worked because I had an AR Carto::User, but the tests seem to use some Sequel ::User, and I guess we should cover all bases.

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'm going to leave the original @user.tables.where(name: @table_name).first.id even if ugly because it seems to work regardless of @user being AR or Sequel, and because we may have more customers affected

table.sanitize_columns(table_name: table_name, database_schema: schema_name, connection: user_database)

# When tables are created using ogr2ogr they are added a ogc_fid or gid primary key
Expand Down