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

Inconsistent database type name casing in backend code #1036

Closed
dmos62 opened this issue Feb 2, 2022 · 6 comments
Closed

Inconsistent database type name casing in backend code #1036

dmos62 opened this issue Feb 2, 2022 · 6 comments
Labels
affects: technical debt Improves the state of the codebase work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@dmos62
Copy link
Contributor

dmos62 commented Feb 2, 2022

Description

At the moment database type names have inconsistent casing applied in different parts of our code. Namely:

  • our enums indb.types have lowercase values,
  • while db.columns.base.MathesarColumn.plain_type returns uppercase values,
    • which effectively means that our Django models use uppercase database type names.

Also, interestingly db.types.operations.cast::get_robust_supported_alter_column_type_map uses both lower and upper cases. That's for robustness it seems, but I'm not sure what is causing the variation in inputs' case.

To be clear, the problem is that this breaks type identifier equality checks, among other things.

Expected behavior

Ideally, we would have a single source of truth for type IDs and use those everywhere. At a minimum, database type name (which is effectively the type identifier) casing should be consistent, both in terms of what is returned and what is expected.

Additional context

@mathemancer you seem to have authored some of the mentioned code. Could you comment on relevant decisions or provide general insight? I noticed that SA usually uses lowercase type names, but in the case of the methods called in MathesarColumn.plain_type it returns uppercase. Is that the sole source of this conflict?

@dmos62 dmos62 added type: bug Something isn't working affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL status: draft labels Feb 2, 2022
@dmos62 dmos62 added this to the [07.2] 2022-02 improvements milestone Feb 2, 2022
@dmos62 dmos62 mentioned this issue Feb 2, 2022
7 tasks
@mathemancer
Copy link
Contributor

The goals of the current setup are as follows:

  • input should be case-insensitive (this is because non-quoted names in PostgreSQL are case-insensitive).
  • Display (output) should be upper-case.

The first behavior listed is part of the SQL spec, and I strongly recommend we avoid changing it. This is the reason for the "robust" type name mapping. It generates a type map that's case-insensitive. The second behavior is more of a convention.
Many regular SQL users (including myself) are used to seeing certain names displayed using upper-case in SQL, type names being an example. For example, if you inspect the SQL generated by SQLAlchemy that includes a type name, it is always upper-case. I'm amenable to changing the second behavior, but it would be a major endeavor, since we'd need to change it wherever SQLAlchemy deals with type names (and any other names that are ususally upper-case). My reason for that amenability is that psql shows types lower-case (this is a client-side choice, though; recall that type names and all other names are case-insensitive by default in SQL).

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 4, 2022

I guess a way to satisfy both those goals is to always uppercase type names. That would include:

  • when an API ingests a type name, uppercase it
  • uppercase the db.types.base enum values

@kgodey kgodey changed the title Incosistent database type name casing in our codebase Incosistent database type name casing in our API Feb 10, 2022
@kgodey kgodey changed the title Incosistent database type name casing in our API Inconsistent database type name casing in our API Mar 1, 2022
@kgodey kgodey assigned dmos62 and unassigned mathemancer Mar 4, 2022
@dmos62 dmos62 changed the title Inconsistent database type name casing in our API Inconsistent database type name casing in backend code Mar 15, 2022
@dmos62 dmos62 added affects: technical debt Improves the state of the codebase and removed type: bug Something isn't working affects: architecture Improvements or additions to architecture status: draft labels Mar 15, 2022
@dmos62
Copy link
Contributor Author

dmos62 commented Mar 15, 2022

Casing presumptions turned out more ingrained in our code than first thought. Retreated to just uppercasing the endpoint's output, so as to conform with current policy to avoid doing technical debt work where possible. See PR #1176.

@kgodey can you help pick an appropriate milestone to move this to?

@kgodey
Copy link
Contributor

kgodey commented Mar 15, 2022

@dmos62 I made a new milestone, [v1] Technical Debt. Let's use that one.

@mathemancer
Copy link
Contributor

I guess a way to satisfy both those goals is to always uppercase type names. That would include:

  • when an API ingests a type name, uppercase it
  • uppercase the db.types.base enum values

Sorry, I didn't see this earlier. I want to make sure the db module functions also take type names case-insensitively. Just upper-casing from the API layer doesn't accomplish this. Recall that the db module should be able to stand alone.

@dmos62 dmos62 modified the milestones: [07] Initial Data Types, [v1] Technical Debt Mar 17, 2022
@kgodey kgodey modified the milestones: [v1] Technical Debt, Unprioritized Jun 1, 2022
@kgodey kgodey removed this from the Unprioritized milestone Jul 19, 2022
@dmos62
Copy link
Contributor Author

dmos62 commented Jul 25, 2022

Seems to be resolved, though I'm not certain that all your concerns have been addressed. @mathemancer let's reopen this if we find cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: technical debt Improves the state of the codebase work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

3 participants