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

API for database config #3170

Merged
merged 43 commits into from Oct 3, 2023
Merged

API for database config #3170

merged 43 commits into from Oct 3, 2023

Conversation

Anish9901
Copy link
Member

@Anish9901 Anish9901 commented Aug 24, 2023

Fixes #3157
Fixes #3158

Technical details

API Signature:

  • Create DB credentials:
POST /api/db/v0/databases/
{
    "name": "mathesar", (unique)
    "db_username": "mathesar",
    "db_password": "mathesar",
    "db_host": "mathesar_dev_db",
    "db_port": 5432
}
  • Alter DB credentials:
PATCH /api/db/v0/databases/<id>
{
    "name": "mathesar", (unique)
    "db_username": "mathesar",
    "db_password": "mathesar",
    "db_host": "mathesar_dev_db",
    "db_port": 5432
}
  • Delete DB credentials:
DELETE /api/db/v0/databases/<id>
  • Delete DB credentials & msar SQL & types schemas:
DELETE /api/db/v0/databases/<id>/?del_msar_schemas=True

Changed common_data dict:

  • If someone edits their database credentials outside of mathesar the django model for that connection becomes stale.
  • To address this the databases key in the common_dict will now provide a dict something like this for the corresponding stale database object:
{
    'name': 'mt_tables',
    'editable': True,
    'error': 'Error connecting to the database'
}

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@Anish9901
Copy link
Member Author

Anish9901 commented Aug 29, 2023

There is an issue with the install script that I'd like to point out, whenever there is a change in the function signature of our SQL functions the install script executes without termination even though psycopg exceptions can be noticed in the logs.

I don't think it's a good idea to reinstall those schema without the consent of the users as there might be db objects dependent on those schemas.

I'd like to here your thoughts on it @silentninja

@Anish9901 Anish9901 added the pr-status: review A PR awaiting review label Aug 29, 2023
@Anish9901 Anish9901 added this to the v0.1.4 milestone Aug 29, 2023
@Anish9901 Anish9901 marked this pull request as ready for review August 29, 2023 12:43
Copy link
Contributor

@silentninja silentninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start! However, there are a few issues

  • The Database model does not contain the unique name field. We cannot use the database name as a reference as it can be changed too even though the data inside the database is the same. Please add it to the Database model.

  • I have made specific comments to highlight other issues

db/types/install.py Show resolved Hide resolved
db/install.py Outdated Show resolved Hide resolved
db/types/install.py Show resolved Hide resolved
mathesar/models/base.py Outdated Show resolved Hide resolved
mathesar/exception_handlers.py Outdated Show resolved Hide resolved
mathesar/api/db/viewsets/databases.py Outdated Show resolved Hide resolved
mathesar/api/serializers/databases.py Outdated Show resolved Hide resolved
@silentninja
Copy link
Contributor

whenever there is a change in the function signature of our SQL functions the install script executes without termination even though psycopg exceptions can be noticed in the logs.

I thought we decided not to have a breaking function signature between release. If you are noticing this in the develop branch I don't think it is a problem. If not please raise an issue for it

I don't think it's a good idea to reinstall those schema without the consent of the users as there might be db objects dependent on those schemas.

For the sake of easier upgrades, we decided to not have any breaking changes. So I don't think it will affect any dependent objects

@silentninja silentninja assigned Anish9901 and unassigned silentninja Sep 11, 2023
@silentninja silentninja removed the pr-status: review A PR awaiting review label Sep 11, 2023
@Anish9901 Anish9901 removed their assignment Sep 11, 2023
@Anish9901 Anish9901 added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Sep 11, 2023
@Anish9901 Anish9901 mentioned this pull request Sep 12, 2023
7 tasks
@silentninja
Copy link
Contributor

unique is set to True in the Database model for the name field, am I missing something?

If the database object is added using a .env file, the only way to track the database object across database name change is by using a unique database key which is not tied to the property of the database. So we need a unique database key on top of database name

Copy link
Contributor

@silentninja silentninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Anish9901, it looks good to me! I would like to wait until the frontend work is done to merge this PR as it will be easier to confirm if something is broken.

@seancolsen
Copy link
Contributor

@silentninja said:

I would like to wait until the frontend work is done to merge this PR as it will be easier to confirm if something is broken.

Given that @rajatvijay has made significant progress on the front end PR now, do you think we could merge this backend PR? If not, then stwWhat's the reason to keep this PR open? We can handle any subsequent backend changes in other PRs, right?

@silentninja silentninja added this pull request to the merge queue Oct 3, 2023
Merged via the queue into develop with commit 19c774f Oct 3, 2023
18 checks passed
@silentninja silentninja deleted the db_config_api branch October 3, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
3 participants