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

Schemas are not returned in the APIs right after editing the database connection details #3230

Closed
rajatvijay opened this issue Oct 3, 2023 · 11 comments
Assignees
Labels
affects: ux Related to user experience priority: urgent Blocks other ongoing work restricted: maintainers Only maintainers can resolve this issue type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@rajatvijay
Copy link
Contributor

rajatvijay commented Oct 3, 2023

To Reproduce

  1. Add a new database connection from the mathesar UI.
  2. Rename the database from the postgres server.
  3. Reload the database page of this particular database on mathesar.
  4. Expect connection error on the page.
  5. Click on the "Edit Connection" button.
  6. Change the name & click on the "Update the connection" button.
  7. Get redirected to the database page of this particular database.
  8. Expect to see the schemas from this database. Instead see no schemas being listed.

Additional information

  1. Frontend calls the POST: /api/ui/v0/reflect/ API right the editing of the database connection succeeds.
  2. To reproduce this please use the branch: db-connection-pages
@rajatvijay rajatvijay added type: bug Something isn't working status: triage labels Oct 3, 2023
@rajatvijay rajatvijay added affects: ux Related to user experience work: backend Related to Python, Django, and simple SQL status: draft restricted: maintainers Only maintainers can resolve this issue priority: urgent Blocks other ongoing work and removed status: triage labels Oct 3, 2023
@rajatvijay rajatvijay added this to the v0.1.4 milestone Oct 3, 2023
@rajatvijay rajatvijay mentioned this issue Oct 3, 2023
7 tasks
@dmos62
Copy link
Contributor

dmos62 commented Oct 3, 2023

Still can't reproduce. Could be some kind of race condition, but the /api/ui/v0/reflect/ seems to be blocking.

I'll start out by having backend automatically reflect when you edit a connection string. And, to only reflect the affected db (the /reflect endpoint reflects everything).

@Anish9901
Copy link
Member

I'll start out by having backend automatically reflect when you edit a connection string. And, to only reflect the affected db (the /reflect endpoint reflects everything).

@dmos62 I already tried to do that, but it results in a bunch of failed test cases.

@dmos62
Copy link
Contributor

dmos62 commented Oct 5, 2023

@Anish9901 yeah, I'm running into some tech debt. You'd want to reflect the database whenever you make changes to the connection string (because it might now be a totally different database), but during reflection we actually make changes to the database models too (to mark them as deleted or not deleted), which causes infinite recursion, because it triggers reflection. I think it's fairly important to get reflection right; I'm seeing what I can do.

@Anish9901
Copy link
Member

One possible solution I see for this problem is to detect which fields are altered in the model and triggering reflection only when db_name, name, host or port are changed, but that might not be a quick fix given the urgent priority of this issue.

@dmos62
Copy link
Contributor

dmos62 commented Oct 23, 2023

One possible solution I see for this problem is to detect which fields are altered in the model and triggering reflection only when db_name, name, host or port are changed, but that might not be a quick fix given the urgent priority of this issue.

I had similar ideas. Though I opted to just get rid of the deleted field altogether. Seemed like a confused concept, and it wasn't used by frontend. And, there was a whole bunch of other buggy weirdness associated with it. Now, we just do a connectability check whenever listing databases in the API, and we don't persist the result of that. If it turns out that we want to persist it for some reason (right now we don't need to), we can come up with something better than deleted.

@dmos62
Copy link
Contributor

dmos62 commented Oct 24, 2023

When rebasing #3223 on top of #3245, I can't reproduce this anymore. Though, before I wasn't able to reproduce too, sort of: then I succeeded to reproduce, and couldn't tell why I couldn't reproduce from the beginning. My point is that I'm not confident that the bug is fixed.

@rajatvijay would you find the time to checkout #3223, merge #3245 on top (note that it removes the deleted field from AppTypes.ts), and see if you can reproduce? Even if #3223 is dead, we need to fix this bug. If you succeed in reproducing, could you check if the steps you follow are exactly as described in the top post?

@rajatvijay
Copy link
Contributor Author

Sure @dmos62 I will get to it later today or first thing in morning tomorrow.

@dmos62
Copy link
Contributor

dmos62 commented Oct 24, 2023

@rajatvijay thanks! By the way, with changes in that pr, frontend shouldn't have to trigger reflection manually when making changes via api.

@seancolsen seancolsen assigned mathemancer and unassigned dmos62 Nov 1, 2023
@seancolsen
Copy link
Contributor

@mathemancer since you're already assigned to the corresponding PR, I'm assigning you to figure out the next steps with this issue no that Dom will non longer be working on it.

@mathemancer
Copy link
Contributor

mathemancer commented Dec 22, 2023

@seancolsen I closed the referenced PR, since huge chunks of it are no longer relevant with the new way of handling connections. As for this issue, it's always been difficult to reproduce, the reason behind it wasn't clear to me, and it may have disappeared on its own. I'm going to try to reproduce it myself, and close this issue if I can't. We can reopen or create a fresh issue.

@mathemancer
Copy link
Contributor

Okay, I tried a few times and can't reproduce. I did crash into the bug reported in #3329 , and we should certainly resolve that one.

@kgodey kgodey closed this as not planned Won't fix, can't repro, duplicate, stale Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: ux Related to user experience priority: urgent Blocks other ongoing work restricted: maintainers Only maintainers can resolve this issue type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

6 participants