Qovery / Replibyte

Seed your development database with real data ⚡️
https://www.replibyte.com
GNU General Public License v3.0
4.17k stars 129 forks source link

Fix escape quote check #182

Closed danstewart closed 2 years ago

danstewart commented 2 years ago

Fixes https://github.com/Qovery/Replibyte/issues/180

evoxmusic commented 2 years ago

Thank you @danstewart for the contribution. Can you provide a test to validate your fix?

danstewart commented 2 years ago

Hey @evoxmusic, test added :slightly_smiling_face: I've also restored the original logic, I wasn't aware you could escape quotes by doubling them up.

I'm not sure why my first version didn't break the tests though :thinking:

danstewart commented 2 years ago

Hey @evoxmusic, trying to look into why the tests are failing but I can't get them to pass locally on main, am I missing something?

$ docker-compose -f docker-compose-dev.yml up
$ cargo build --release --all-features
$ cargo test --all-features
# ...
failures:
    destination::docker::tests::handle_containers
    destination::mongodb::tests::connect
    destination::mongodb_docker::tests::connect
    destination::mysql_docker::tests::connect
    destination::postgres::tests::connect
    destination::postgres_docker::tests::connect
    source::mongodb::tests::connect
    source::mongodb::tests::list_rows
    source::postgres::tests::connect
    source::postgres::tests::subset_options
evoxmusic commented 2 years ago
  1. Can you show the errors?
  2. Do you have the following binaries on your local machine?
danstewart commented 2 years ago

Thanks, looks like it was just missing dependencies, I've got the tests passing on main now. Will look into why they're failing on this branch.

danstewart commented 2 years ago

Got the tests passing now, but I'm still working on this.

I think the other condition is wrong too: if (query.len() > next_idx) && &query[next_idx..next_idx] == "'" {, a slice with the same index twice returns an empty slice.

When I fix it some other tests start failing though, so I need to do some more digging around.

For reference the errors for the first change were:

---- source::mysql::tests::list_rows stdout ----
thread 'source::mysql::tests::list_rows' panicked at 'byte index 4172 is not a char boundary; it is inside 'ç' (bytes 4171..4173) of `LOCK TABLES `city` WRITE;
/*!40000 ALTER TABLE `city` DISABLE KEYS */;
INSERT INTO `city` (`ID`, `Name`, `CountryCode`, `District`, `Population`) VALUES (1,'Kabul','AFG','Kabol',1780000);
INSERT INTO `city` (`ID`, `Name`, `CountryCode`, `District`, `Popula`[...]', dump-parser/src/utils.rs:209:43

---- source::postgres::tests::connect stdout ----
thread 'source::postgres::tests::connect' panicked at 'byte index 821 is not a char boundary; it is inside 'í' (bytes 820..822) of `INSERT INTO public.customers (customer_id, company_name, contact_name, contact_title, address, city, region, postal_code, country, phone, fax) VALUES ('ALFKI', 'Alfreds Futterkiste', 'Maria Anders', 'Sales Representative', 'Obere Str. 57', 'Berlin', NULL, `[...]', dump-parser/src/utils.rs:207:53

---- source::postgres::tests::list_rows_and_hide_last_name stdout ----
thread 'source::postgres::tests::list_rows_and_hide_last_name' panicked at 'byte index 821 is not a char boundary; it is inside 'í' (bytes 820..822) of `INSERT INTO public.customers (customer_id, company_name, contact_name, contact_title, address, city, region, postal_code, country, phone, fax) VALUES ('ALFKI', 'Alfreds Futterkiste', 'Maria Anders', 'Sales Representative', 'Obere Str. 57', 'Berlin', NULL, `[...]', dump-parser/src/utils.rs:207:53

---- source::postgres::tests::list_rows stdout ----
thread 'source::postgres::tests::list_rows' panicked at 'byte index 821 is not a char boundary; it is inside 'í' (bytes 820..822) of `INSERT INTO public.customers (customer_id, company_name, contact_name, contact_title, address, city, region, postal_code, country, phone, fax) VALUES ('ALFKI', 'Alfreds Futterkiste', 'Maria Anders', 'Sales Representative', 'Obere Str. 57', 'Berlin', NULL, `[...]', dump-parser/src/utils.rs:207:53

---- source::postgres::tests::skip_table stdout ----
thread 'source::postgres::tests::skip_table' panicked at 'byte index 821 is not a char boundary; it is inside 'í' (bytes 820..822) of `INSERT INTO public.customers (customer_id, company_name, contact_name, contact_title, address, city, region, postal_code, country, phone, fax) VALUES ('ALFKI', 'Alfreds Futterkiste', 'Maria Anders', 'Sales Representative', 'Obere Str. 57', 'Berlin', NULL, `[...]', dump-parser/src/utils.rs:207:53

---- source::postgres::tests::subset_options stdout ----
thread 'source::postgres::tests::subset_options' panicked at 'byte index 821 is not a char boundary; it is inside 'í' (bytes 820..822) of `INSERT INTO public.customers (customer_id, company_name, contact_name, contact_title, address, city, region, postal_code, country, phone, fax) VALUES ('ALFKI', 'Alfreds Futterkiste', 'Maria Anders', 'Sales Representative', 'Obere Str. 57', 'Berlin', NULL, `[...]', dump-parser/src/utils.rs:207:53
evoxmusic commented 2 years ago

It seems that all tests are passing now :)

danstewart commented 2 years ago

Yeah, this is fine to merge now, I'll pick up the double single-quote issue under another PR :slightly_smiling_face:

It's not a major issue, I think the only issue with it is that invalid SQL could be parsed as valid but I'm not 100% sure, I need to debug a bit more. Either way it's currently in main so no need to hold up this PR for it.