search

NoBrainerORM / nobrainer

Ruby ORM for RethinkDB
http://nobrainer.io/
Other
387 stars 49 forks source link

Fix duplicate table create #284

Closed caspiano closed 2 years ago

caspiano commented 2 years ago

An exception could be raised, preventing the duplicate table logic from running.

zedtux commented 2 years ago

Thank you @caspiano for this PR.

Can you please elaborate a little bit more about the issue you're solving ? I don't really see yet how that change helps. Also, moving the method's rescue doesn't look good to me since then exceptions raised from other calls (i.e NoBrainer.run) will not be caught.

caspiano commented 2 years ago

@zedtux Sorry for the quick PR and thanks for taking a look, I was in a bit of a rush. The /Table `#{db_name}\.#{table_name}` already exists/ will only match an exception caused by thetable_create operation, the rescue looks like it is re-raising anything else. The following NoBrainer.run calls on lines 43 and 52 will not trigger the exception as they are not creating tables.

The problem that this fixes is if a runtime exception is triggered by the table_create operation, the exception would be caught and dropped on line 52, by-passing the logic that removes the duplicate table entry, borking the DB.

For context, I'm the author and maintainer of Crystal RethinkORM. My coworker submitted the original patch that inserted the duplicate table error fix.

zedtux commented 2 years ago

Thank you @caspiano for the details 👍 .

It make a lot of sense now. Let me run the test suite from your PR (I have to fix the pipeline execution in order to get it automatic again) and will come back to you.

zedtux commented 2 years ago

The version 0.41.1 has been published with this fix.