Type logic refactor #1222

dmos62 · 2022-03-23T20:52:35Z

The initial goal of refactor was to centralize and improve our db/ui type logic, of which there is a lot. That dominoed into a lot of secondary refactors, because I often couldn't make the changes I needed outright.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master 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.

…ic-refactor

mathemancer

Overall, I think this is going to be a massive improvement. We'll need to go through the code base and make sure no function deals with bare SA type classes, ever, except to convert them to the appropriate Enum member, or at the last point when creating a column. I'm not sure how to guarantee that long-term. Maybe a ban on imports of SA type classes outside the types.base module?

db/types/base.py

mathemancer · 2022-03-24T16:24:20Z

db/types/base.py

+
+    def get_sa_class(self, engine):
+        """
+        Returns the SA class corresponding to this type or None if this type is not supported by


This will more precisely return a SA class corresponding to this type. In particular, we will return the SQL Standard or Vendor-Specific SA class for the type, with the exceptions of name and "char", which will return (for now) the String generic SA class.

See https://docs.sqlalchemy.org/en/14/core/type_basics.html#sql-standard-and-multiple-vendor-types

dmos62 · 2022-05-23T15:53:41Z

Latest revision adds pytest-xdist support. To use it:

install pytest-xdist on your development docker instance, or rebuild it;
add -n [number of CPUs (or cores?) you have] to your pytest call: something like pytest -vv --ignore=mathesar/tests/integration/ -n 4;
- -n auto should also work, but it's failed on me at times.

dmos62 · 2022-05-23T16:07:32Z

Note, I added -n auto to setup.cfg, so that tests run in parallel automatically. Fall back to using -n [specific number of parallel threads] if that doesn't work for some reason. -n 0 disables parallelization.

dmos62 · 2022-05-24T05:38:40Z

I see that some e2e tests have failed in the Github runner, while they pass fine on my machine. I'm not sure if this is a fluke or if there's an actual problem. Rerunning the test suite.

dmos62 · 2022-05-24T07:25:22Z

I see that some e2e tests have failed in the Github runner, while they pass fine on my machine. I'm not sure if this is a fluke or if there's an actual problem. Rerunning the test suite.

I'm guessing that the problem is that e2e test runner is choking, because I've enabled test parallelization (this runner is using 2 parallel threads). I'll disable automatic parallelization for e2e tests, when I get online.

dmos62 · 2022-05-24T10:42:37Z

I disabled test parallelization on GitHub's e2e test workflow and now it seems to be passing. I presume the problem was that parallelization slowed down the test runner too much. That's not a concern in a regular development environment, as far as I can tell.

I also removed TIME and TIMESTAMP types from the codebase, since as @mathemancer pointed out, we'll never reflect them (we'll reflect types like TIME_WITH_TIME_ZONE instead).

Many changes made, including the requested changes.

mathemancer

Overall, a massive improvement, good work.

I have some qualms with approving, since I admittedly wasn't able to completely verify everything, but:

I looked through tests to asses whether they're still valid; I think they are.
I looked through code to see if I noticed any obvious bugs or problems; none found.
I ran the service and fiddled around with the API to see if I noticed any obvious regressions; none found.

I think we should get this merged ASAP. @dmos62 is it ready to go?

db/types/categories.py

mathemancer · 2022-05-24T12:56:39Z

mathesar/database/types.py

+    id: str  # noqa: NT001
+    display_name: str  # noqa: NT001
+    db_types: Collection[DatabaseType]  # noqa: NT001


Why are these needed?

For clarity. These mixins only support objects that have these attributes. I didn't see another way to state that concisely and clearly. Also, this being actual type annotations, instead of a comment, helps with IDE complaining that referencing these attributes in this class is a mistake.

I'm open to getting rid of it, but I think this is a useful exception to the no-type-hints rule.

seancolsen

@dmos62 You've clearly been busy with this PR 😲

I'm not reviewing the back-end code, but I looked over the front end changes and E2E changes. I added a couple small commits. Looks good!

seancolsen · 2022-05-24T12:48:49Z

docker-compose.yml

@@ -15,8 +15,7 @@ services:
  service:
    build:
      context: .
-      # Change this to Dockerfile.integ-tests to be able to run e2e integ tests
-      dockerfile: Dockerfile
+      dockerfile: ${DOCKERFILE}


This is a great idea, moving this config to .env. I added 7cdfa7c to update the documentation to reflect this change. If I'm understanding this change correctly, then all existing devs will need to add DOCKERFILE=Dockerfile or DOCKERFILE=Dockerfile.integ-tests to their .env before they can re-build the container. Yes? If so, we should give everyone a heads up after merging this PR.

Right! I had forgotten about this! Thanks!

Specifically, my suggestion would have been to do cp .env.example .env.

seancolsen · 2022-05-24T13:39:01Z

mathesar/tests/integration/test_columns.py

@@ -22,6 +23,7 @@ def test_convert_text_column_to_number(page, go_to_patents_data_table):
    expect(page.locator(toast_box)).to_be_visible()


+@pytest.mark.skip(reason="unclear why test is failing: deferring for later")


@dmos62 was this failing for you locally? Or just in CI? I just ran it locally and it passed for me. Maybe we don't need to skip it?

It was failing for me locally. Could we try and resolve this outside this PR? I'd love to get this merged soon.

Yep. Sounds good.

…esar into some-type-logic-refactor

dmos62 · 2022-05-24T18:18:45Z

I've enabled auto-merge. Thanks for the reviews @mathemancer and @seancolsen!

I also disabled test parallelization by default, because @mathemancer discovered a bug, where we can't have more than 7 threads, due to the testing suite not cleaning up unused database connections (i think that's the reason).

dmos62 and others added 20 commits March 17, 2022 14:49

add db type alias information to db types endpoint

af4fa7c

fix set membership test

41c6623

Merge branch 'master' into add_alias_info_to_db_types_endpoint

9b294c8

Exploratory comments and minor refactor

10bf354

Some more slight refactors and comments

d644c37

+

b52b3ab

Fix None filtering

57cf327

Merge branch 'add_alias_info_to_db_types_endpoint' into some-type-log…

95595e3

…ic-refactor

Add utilities to DatabaseType for looking up its ischema key

d98271d

Linter fixes

9264c0d

Minor refactor

9576f27

Add get_sa_class method to DatabaseType

430c8af

Get rid of aliases information from DatabaseType

f0517e8

Access ischema_names dict directly

754e793

Add DatabaseType::is_available_on_engine

da659d3

Delete get_available_types, some constants

e5023cb

Various type-related refactors

69fc2b2

Remove leftover aliases namespace

c2357fe

Minor refactor

0fd467e

Linter fixes

3a73895

dmos62 added affects: technical debt Improves the state of the codebase work: backend Related to Python, Django, and simple SQL status: draft labels Mar 23, 2022

dmos62 added this to the [07] Initial Data Types milestone Mar 23, 2022

dmos62 self-assigned this Mar 23, 2022

dmos62 added 4 commits March 24, 2022 14:07

Merge branch 'master' into some-type-logic-refactor

4a9aa6d

Remove other aliases remnants

e614e2c

Minor changes

278d885

Various fixes

50a4425

mathemancer reviewed Mar 24, 2022

View reviewed changes

dmos62 requested a review from seancolsen May 23, 2022 15:47

dmos62 added 3 commits May 23, 2022 19:00

Vulture fixes

ec16cc3

Linter fixes

f8ff2cb

Add -n auto to setup.cfg so that tests run in parallel automatically

0708bc0

dmos62 requested a review from mathemancer May 24, 2022 05:39

dmos62 added 3 commits May 24, 2022 12:54

Disable xdist for e2e tests

dc03636

Get rid of non-canonical DatabaseTypes

b133c4f

Remove is_alias DatabaseType property

62fa079

Update E2E setup docs

7cdfa7c

mathemancer approved these changes May 24, 2022

View reviewed changes

mathemancer enabled auto-merge May 24, 2022 13:10

mathemancer disabled auto-merge May 24, 2022 13:17

Use correct case for URI type

91adbb7

seancolsen approved these changes May 24, 2022

View reviewed changes

dmos62 and others added 3 commits May 24, 2022 21:15

Disable automatic test parallelization: fails with >7 threads

bfcf549

Merge branch 'some-type-logic-refactor' of github.com:centerofci/math…

80b368e

…esar into some-type-logic-refactor

Merge branch 'master' into some-type-logic-refactor

1b53d4c

dmos62 enabled auto-merge May 24, 2022 18:18

dmos62 merged commit 66f34de into master May 24, 2022

dmos62 deleted the some-type-logic-refactor branch May 24, 2022 18:39

Cronus1007 mentioned this pull request May 24, 2022

Setup Python class to represent name db #1387

Closed

7 tasks

dmos62 mentioned this pull request May 27, 2022

Resolve logic duplication in the db.types namespace #1100

Closed

dmos62 mentioned this pull request Jun 7, 2022

Improve pytest developer experience #1368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type logic refactor #1222

Type logic refactor #1222

dmos62 commented Mar 23, 2022 •

edited

Loading

mathemancer left a comment

mathemancer Mar 24, 2022

dmos62 commented May 23, 2022

dmos62 commented May 23, 2022 •

edited

Loading

dmos62 commented May 24, 2022

dmos62 commented May 24, 2022

dmos62 commented May 24, 2022

mathemancer left a comment •

edited

Loading

mathemancer May 24, 2022

dmos62 May 24, 2022

seancolsen left a comment

seancolsen May 24, 2022

dmos62 May 24, 2022

seancolsen May 24, 2022

dmos62 May 24, 2022

seancolsen May 24, 2022

dmos62 commented May 24, 2022 •

edited

Loading

		@@ -22,6 +23,7 @@ def test_convert_text_column_to_number(page, go_to_patents_data_table):
		expect(page.locator(toast_box)).to_be_visible()


		@pytest.mark.skip(reason="unclear why test is failing: deferring for later")

Type logic refactor #1222

Type logic refactor #1222

Conversation

dmos62 commented Mar 23, 2022 • edited Loading

Checklist

Developer Certificate of Origin

mathemancer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented May 23, 2022

dmos62 commented May 23, 2022 • edited Loading

dmos62 commented May 24, 2022

dmos62 commented May 24, 2022

dmos62 commented May 24, 2022

mathemancer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seancolsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented May 24, 2022 • edited Loading

dmos62 commented Mar 23, 2022 •

edited

Loading

dmos62 commented May 23, 2022 •

edited

Loading

mathemancer left a comment •

edited

Loading

dmos62 commented May 24, 2022 •

edited

Loading