Migrate django_db to sqlite #3203

Anish9901 · 2023-09-11T15:54:03Z

Technical details

This PR removes the necessity to use postgres for internal django models db, whenever env DJANGO_DATABASE_URL is not set the app will now create a fresh sqlite db, however if DJANGO_DATABASE_URL is set the app will respect the value of the env variable and create a db for the given db connection.

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
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.

silentninja

The PR is missing migration for moving existing data from an ArrayField to JSONField. Have you tested with an existing database created using Mathesar 0.1.3?

I also have a few more concerns which I added as specific comments

silentninja · 2023-09-13T15:06:06Z

config/settings/common_settings.py

-DATABASES[decouple_config('DJANGO_DATABASE_KEY', default="default")] = decouple_config('DJANGO_DATABASE_URL', cast=db_url)
+
+try:
+    DATABASES[decouple_config('DJANGO_DATABASE_KEY', default="default")] = decouple_config('DJANGO_DATABASE_URL', cast=db_url)


Wouldn't it be better to set the default SQLite dictionary as a default value? The default dictionary can be stored in a variable and used as a default value

Suggested change

DATABASES[decouple_config('DJANGO_DATABASE_KEY', default="default")] = decouple_config('DJANGO_DATABASE_URL', cast=db_url)

DEFAULT_DATABASE = {

'ENGINE': 'django.db.backends.sqlite3',

'NAME': os.path.join(BASE_DIR, 'db.sqlite3'),

}

DATABASES[decouple_config('DJANGO_DATABASE_KEY', default="default")] = decouple_config('DJANGO_DATABASE_URL', cast=db_url, default=DEFAULT_DATABASE

Wow, really clean, but this doesn't work because of cast. I've added the connection string as the default now and it works flawlessly.

silentninja · 2023-09-13T15:08:44Z

mathesar/api/serializers/table_settings.py

+    def to_internal_value(self, data):
+        column_order = data.get('column_order', None)
+        if isinstance(column_order, list):
+            data['column_order'] = list(map(int, column_order))


Won't setting this validation as a ListField work?

😲 Didn't knew setting a child would also typecast, I thought it would just type check. Anyway, fixed now.

silentninja · 2023-09-13T15:10:57Z

mathesar/models/base.py

@@ -884,7 +883,7 @@ class PreviewColumnSettings(BaseModel):
 class TableSettings(ReflectionManagerMixin, BaseModel):
    preview_settings = models.OneToOneField(PreviewColumnSettings, on_delete=models.CASCADE)
    table = models.OneToOneField(Table, on_delete=models.CASCADE, related_name="settings")
-    column_order = ArrayField(models.IntegerField(), null=True, default=None)
+    column_order = JSONField(null=True, default=list)


If the user has not customized the column order, then colum_order will be null.

I made a mistake when we had a sync call I think this should be None. Look at the issue description

Anish9901 · 2023-09-13T18:38:59Z

The PR is missing migration for moving existing data from an ArrayField to JSONField.

Postgres disallows casting a col from integer[] to jsonb and thus breaks our tests if a migration is added to alter it. I'll add the migration along with the already altered migration(0001_initial.py).

Anish9901 added 3 commits September 11, 2023 16:28

remove dependency on ArrayField

bfc433a

fix test config

23cccbb

rm start.sh

fae0af8

Anish9901 changed the title ~~Migrate to sqlite~~ Migrate django_db to sqlite Sep 11, 2023

Anish9901 added 7 commits September 12, 2023 18:33

add migrations and fix tests

8f2c82d

allow only integer array to be stored in column_order

297ea78

revert conftest

a5c153b

remove print statement

eacf56d

remove redundant env variables

e339fba

restore migrations

478f40b

restore conftest

ef95764

Anish9901 requested a review from silentninja September 12, 2023 14:34

Anish9901 assigned silentninja Sep 12, 2023

Anish9901 added the pr-status: review A PR awaiting review label Sep 12, 2023

Anish9901 marked this pull request as ready for review September 12, 2023 14:46

silentninja requested changes Sep 13, 2023

View reviewed changes

Merge branch 'develop' into migrate_to_sqlite

bcf929d

silentninja assigned Anish9901 and unassigned silentninja Sep 13, 2023

silentninja added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Sep 13, 2023

Anish9901 added 2 commits September 14, 2023 00:57

address review

1e81030

default None

8363afb

Anish9901 requested a review from silentninja September 13, 2023 19:47

Anish9901 assigned silentninja and unassigned Anish9901 Sep 13, 2023

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 13, 2023

handle array to jsonb casting during migration in postgres

6111af6

Merge branch 'develop' into migrate_to_sqlite

209a0c1

silentninja enabled auto-merge September 20, 2023 06:15

silentninja approved these changes Sep 20, 2023

View reviewed changes

silentninja added this pull request to the merge queue Sep 20, 2023

Merged via the queue into develop with commit 714e1f7 Sep 20, 2023
21 checks passed

silentninja deleted the migrate_to_sqlite branch September 20, 2023 06:16

silentninja mentioned this pull request Sep 26, 2023

Revert back DJANGO_DATABASE_URL env changes #3225

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate django_db to sqlite #3203

Migrate django_db to sqlite #3203

Anish9901 commented Sep 11, 2023 •

edited

silentninja left a comment •

edited

silentninja Sep 13, 2023

Anish9901 Sep 13, 2023

silentninja Sep 13, 2023

Anish9901 Sep 13, 2023 •

edited

silentninja Sep 13, 2023

Anish9901 commented Sep 13, 2023 •

edited

-    DATABASES[decouple_config('DJANGO_DATABASE_KEY', default="default")] = decouple_config('DJANGO_DATABASE_URL', cast=db_url)
+DEFAULT_DATABASE = {
+        'ENGINE': 'django.db.backends.sqlite3',
+        'NAME': os.path.join(BASE_DIR, 'db.sqlite3'),
+    }
+DATABASES[decouple_config('DJANGO_DATABASE_KEY', default="default")] = decouple_config('DJANGO_DATABASE_URL', cast=db_url, default=DEFAULT_DATABASE

Migrate django_db to sqlite #3203

Migrate django_db to sqlite #3203

Conversation

Anish9901 commented Sep 11, 2023 • edited

Checklist

Developer Certificate of Origin

silentninja left a comment • edited

Choose a reason for hiding this comment

silentninja Sep 13, 2023

Choose a reason for hiding this comment

Anish9901 Sep 13, 2023

Choose a reason for hiding this comment

silentninja Sep 13, 2023

Choose a reason for hiding this comment

Anish9901 Sep 13, 2023 • edited

Choose a reason for hiding this comment

silentninja Sep 13, 2023

Choose a reason for hiding this comment

Anish9901 commented Sep 13, 2023 • edited

Anish9901 commented Sep 11, 2023 •

edited

silentninja left a comment •

edited

Anish9901 Sep 13, 2023 •

edited

Anish9901 commented Sep 13, 2023 •

edited