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.

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 duplicate table create #284

Merged
merged 1 commit into from
Mar 21, 2022
Merged

Fix duplicate table create #284

merged 1 commit into from
Mar 21, 2022

Conversation

caspiano
Copy link
Contributor

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

An exception could be raised, preventing the duplicate table logic from
running.
@zedtux
Copy link
Collaborator

zedtux commented Mar 21, 2022

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
Copy link
Contributor Author

@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
Copy link
Collaborator

zedtux commented Mar 21, 2022

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 zedtux merged commit 1153e04 into NoBrainerORM:master Mar 21, 2022
@zedtux
Copy link
Collaborator

zedtux commented Mar 21, 2022

The version 0.41.1 has been published with this fix.

@caspiano caspiano deleted the fix/ensure-fix-table-duplicate branch March 21, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants