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

Bugs while deleting database connection #3361

Closed
Anish9901 opened this issue Dec 19, 2023 · 5 comments · Fixed by #3388
Closed

Bugs while deleting database connection #3361

Anish9901 opened this issue Dec 19, 2023 · 5 comments · Fixed by #3388
Assignees
Labels
help wanted Community contributors can implement this ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@Anish9901
Copy link
Member

Anish9901 commented Dec 19, 2023

Description

Part 1: (Fixed in #3377)

  • Deleting a database connection always drops associated mathesar schemas, regardless of the query_param del_msar_schemas being set to true or false.

Part 2:

  • Deleting a database connection with del_msar_schemas=true whose database is deleted outside of mathesar results in an Internal server error.

Expected behavior

Part 1:

  • Backend should respect the query_param del_msar_schemas.

Part 2:

  • Raise Internal server error with details about the non-reachability of the db for which the schemas are to be deleted.

To Reproduce

Part 1:

  • Visit http://localhost:8000/connections/
  • Notice a connection with the name mathesar_tables
  • In your terminal execute docker exec -it mathesar_dev_db psql -U mathesar
  • Execute \dn in the psql shell. Notice the different schemas in the database.
  • Try deleting a connection without checking the Delete associated Mathesar schemas box through the UI.
  • Execute \dn in the psql shell and notice that the schemas msar, __msar, and mathesar_types get dropped regardless.

Part 2:

  • Visit http://localhost:8000/connections/
  • Notice a connection with the name mathesar_tables, follow the steps in the note section if you don't see the connection.
  • Delete the database for which a connection exists using psql drop database mathesar with (force);
  • Try deleting the connection from the UI with Delete associated Mathesar schemas checked.
  • Notice the internal server error.

Note:
To recreate the deleted mathesar_tables connection you can send a post request to the following with the provided json until #3319 isn't merged:

http://localhost:8000/api/db/v0/connections/
{
    "nickname": "mathesar_tables",
    "database": "mathesar",
    "username": "mathesar",
    "password": "mathesar",
    "host": "mathesar_dev_db",
    "port": 5432
}
@Anish9901 Anish9901 added type: bug Something isn't working help wanted Community contributors can implement this work: backend Related to Python, Django, and simple SQL labels Dec 19, 2023
@Anish9901 Anish9901 added this to the v0.1.4 milestone Dec 19, 2023
@Anish9901 Anish9901 added the ready Ready for implementation label Dec 19, 2023
@seancolsen
Copy link
Contributor

seancolsen commented Dec 20, 2023

@Anish9901 thanks for opening this and for correctly applying the new labels! 🙂

Parts 1 and 2 seem to be independent. If they are related, can you clarify how? If they're independent, then can you please close this issue and open two separate ones to track these bugs independently?

Also as a side note, for bugs I think it helps to have some clearer steps to reproduce. With sufficiently clear repro steps, I don't think we even need the "Expected behavior" section. See #1563 and #2724 as examples. Whenever I write repro steps I try to think of them like pseudocode for a unit test. The repro steps should have some setup, some action, an expectation, and an observation. If you can write these steps out in that manner, I think it will help a community contributor pick these up more easily.

Alternatively, I think it's also okay to keep tickets a bit more vague and messy if they are labeled "restricted: maintainers".

@Anish9901
Copy link
Member Author

@seancolsen I've updated the repro steps.

Parts 1 and 2 seem to be independent. If they are related, can you clarify how? If they're independent, then can you please close this issue and open two separate ones to track these bugs independently?

Since the flag del_msar_schemas isn't working as expected as described in Part 1, Part 2 always ends up trying to remove the mathesar schemas making it impossible to delete a connection in absence of a db. So, Part 1 should get fixed first before moving to Part 2. Also, Part 1 is a one line fix.

@Taherpatrawala
Copy link
Contributor

Hello there @Anish9901 , can I work on this issue? I know I can't be assigned yet hence I already worked on the part 1, just wanted your permission to continue further..!

@Anish9901
Copy link
Member Author

Go ahead and submit a PR @Taherpatrawala, FYI the team will be on winter break from 23rd till 1st of next month so your PR will be reviewed once the break is over.

@mathemancer
Copy link
Contributor

@Anish9901 @Taherpatrawala I think that "Internal Server Error" is the correct error type (though we can add a more granular detail) if you try to delete a connection and the underlying schemas, but find that you can't reach the underlying DB in some (maybe most) cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Community contributors can implement this ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants