[MSSQL, PostgreSQL] Resolve case sensitivity mismatch in id_columns transformation #3885
Conversation
| else: | ||
| id_columns_str = f"{id_columns_str}_{table}" | ||
| id_columns = [f"{id_columns_str}_{column}" for column in id_columns] | ||
| id_columns = [ |
There was a problem hiding this comment.
I think what needs to happen is that you have to pass both the fetch-from-db side and the configured-from-config-columns through the same transformation.
The other side puts this through map_column_names while here and in the postgres connector we have 2 different pieces of code that try to do the same, which makes things a bit more confusing and brittle (as someone might not understand that these are to be fully in sync and make small adjustments).
Based on this, I say we pass the columns here through the map_column_names function as well - but I might be missing something.
Apmats
left a comment
Apmats
left a comment
There was a problem hiding this comment.
I think this functionally is correct. I have a comment about reusing a function that would be good to address.
Good job on tracking this down!
artem-shelkovnikov
left a comment
artem-shelkovnikov
left a comment
There was a problem hiding this comment.
Can you change functional test so that it would trigger this bug and verify that the outcome is correct?
dennis-tismenko
force-pushed
the
dennis/fix-id-columns-case-sensitivity
branch
from
57dba9b to
af2a804
Compare
Closes #3884
This PR fixes a case sensitivity bug in the PostgreSQL and MSSQL connectors when using
id_columnsin advanced sync rules:.lower()to theid_columnstransformation.lower()andsorted()to matchmap_column_names()behaviourChecklists
Pre-Review Checklist
config.yml.example)v7.13.2,v7.14.0,v8.0.0)Release Note
PostgreSQL/MSSQL connectors: Fixed a bug where using
id_columnsin advanced sync rules with mixed-case table or column names caused all documents to receive the same_id, resulting in document overwrites and only 1 document being indexed instead of the expected count.