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

Add db type alias information to db types endpoint #1190

Closed
wants to merge 4 commits into from

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Mar 17, 2022

Fixes #1037

This adds database type alias information to the /api/db/v0/databases/1/types/ endpoint.

Technical details

Alias information copied from table in https://www.postgresql.org/docs/11/datatype.html.

Adds two new fields to objects in the types/ endpoint output: aliases (contains aliases for this type, if this type is canonical and has known aliases) and alias_of (contains the canonical alias for this type, if this type is known to be a non-canonical alias).

So, for example, if alias_for is null, according to the information in PG's documentation, it's a canonical.

Mentioned endpoint's output after changes:

[
  {
    "id": "_ARRAY",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "BIGINT",
    "aliases": [
      "INT8"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "BIT VARYING",
    "aliases": [
      "VARBIT"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "BIT",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "BOOLEAN",
    "aliases": [
      "BOOL"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "boolean"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "BYTEA",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "\"CHAR\"",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "string_like"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "CHARACTER VARYING",
    "aliases": [
      "VARCHAR"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "string_like"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "CHARACTER",
    "aliases": [
      "CHAR"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "string_like"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "CIDR",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "DATE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "point_in_time"
      },
      {
        "id": "date"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "DATERANGE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "DECIMAL",
    "aliases": [],
    "alias_of": "NUMERIC",
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "DOUBLE PRECISION",
    "aliases": [
      "FLOAT8"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "FLOAT",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "HSTORE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "INET",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "INT4RANGE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "INT8RANGE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "INTEGER",
    "aliases": [
      "INT",
      "INT4"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "INTERVAL",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "duration"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "JSON",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "JSONB",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "MACADDR",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "MONEY",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "NAME",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "string_like"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "NUMERIC",
    "aliases": [
      "DECIMAL"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "NUMRANGE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "OID",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "REAL",
    "aliases": [
      "FLOAT4"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "REGCLASS",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "SMALLINT",
    "aliases": [
      "INT2"
    ],
    "alias_of": null,
    "hints": [
      {
        "id": "comparable"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TEXT",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "string_like"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TIME",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      },
      {
        "id": "comparable"
      },
      {
        "id": "time"
      },
      {
        "id": "point_in_time"
      }
    ]
  },
  {
    "id": "TIME WITH TIME ZONE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      },
      {
        "id": "comparable"
      },
      {
        "id": "time"
      },
      {
        "id": "point_in_time"
      }
    ]
  },
  {
    "id": "TIME WITHOUT TIME ZONE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      },
      {
        "id": "comparable"
      },
      {
        "id": "time"
      },
      {
        "id": "point_in_time"
      }
    ]
  },
  {
    "id": "TIMESTAMP",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "time"
      },
      {
        "id": "date"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TIMESTAMP WITH TIME ZONE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "time"
      },
      {
        "id": "date"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TIMESTAMP WITHOUT TIME ZONE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "time"
      },
      {
        "id": "date"
      },
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TSRANGE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TSTZRANGE",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "TSVECTOR",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  },
  {
    "id": "UUID",
    "aliases": [],
    "alias_of": null,
    "hints": [
      {
        "id": "any"
      }
    ]
  }
]

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

@dmos62 dmos62 added the affects: architecture Improvements or additions to architecture label Mar 17, 2022
@dmos62 dmos62 added this to the [07] Initial Data Types milestone Mar 17, 2022
@dmos62 dmos62 requested a review from a team March 17, 2022 12:55
@dmos62 dmos62 self-assigned this Mar 17, 2022
@dmos62 dmos62 requested a review from mathemancer March 17, 2022 12:55
@dmos62 dmos62 added the work: backend Related to Python, Django, and simple SQL label Mar 17, 2022
@dmos62
Copy link
Contributor Author

dmos62 commented Mar 17, 2022

@seancolsen @pavish can either of you take a look and see if this suits frontend needs? I considered creating a separate endpoint, something like aliases/, with a mapping from aliases to canonicals, but instead added the aliases and alias_of fields to the types/ endpoint. If a separate endpoint makes more sense to you, it's not a lot of work on my end.

@pavish
Copy link
Member

pavish commented Mar 17, 2022

@dmos62 This looks good and makes it a lot easier for the frontend than having a separate endpoint.

@dmos62 dmos62 added the pr-status: review A PR awaiting review label Mar 17, 2022
@kgodey kgodey assigned dmos62 and mathemancer and unassigned dmos62 Mar 17, 2022
@kgodey
Copy link
Contributor

kgodey commented Mar 17, 2022

The structure looks good to me.

@dmos62 FYI, I edited your issue description to mark the snippet as JSON, the syntax highlighting makes it easier to read.

Copy link
Contributor

@mathemancer mathemancer left a comment

Choose a reason for hiding this comment

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

If we take as a given that this feature is desirable, then the canonical type name shouldn't be related to the DB docs, but rather to the string names for types in the engine.dialect.ischema_names dictionary. The reasons are:

  • This is the dictionary that defines the type for SQLAlchemy, and
  • these strings are guaranteed to be the name of the type upon reflection (otherwise reflection wouldn't work).

However, as I noted in my comment on the linked issue, I don't think we should add this feature.

  • If the front end is being handed two options as names for some type, that's a bug.
  • If the type name is different at different endpoints for some type, that's a bug.

Taking the example of both DECIMAL and NUMERIC being given as options, we should only give NUMERIC. Even if you create a column of type DECIMAL, it will be reflected as NUMERIC. This is in stark contrast to the situation with VARCHAR and TEXT. These are distinct types, since VARCHAR takes a length type option, whereas TEXT does not.

@pavish I'd be interested in your thoughts here. Do you actually see a need for alias information in the front end, or would it be better to simply avoid multiple names for a given type?

@dmos62
Copy link
Contributor Author

dmos62 commented Mar 21, 2022

@mathemancer good catch. I agree.

We had a call about this with Brent after he made this comment.

I summarized our thoughts in this discussion: #1184 (comment)

The short of it is that, if we're right that the only reason to expose type aliases to frontend was because the frontend is exposed to database aliases by the db types endpoint, then a better solution is to remove aliases from the db types endpoint. Or, from the backend's perspective, to remove aliases from ischema_names or wrap it in an abstraction.

@pavish if getting rid of database aliases would solve your use case, feel free to close this PR.

@pavish
Copy link
Member

pavish commented Mar 21, 2022

@dmos62 It's not only on the db types endpoint but also the column.type property on the columns endpoint. From the previous comments and the linked thread, I assume that is covered as well.

@mathemancer

Do you actually see a need for alias information in the front end, or would it be better to simply avoid multiple names for a given type?

I'm on board with not having any alias information and just have a single name for a single type.

However, we do show the DB type to the user on the frontend, so I'm not sure if it will be an issue for the user if we do not show the exact name of the type that the db shows. Eg., The DB cli tools show it as DECIMAL and we show it as NUMERIC.

@kgodey I'd like your thoughts on this.

@kgodey
Copy link
Contributor

kgodey commented Mar 22, 2022

For types with aliases, I think it's preferable to have a canonical type that the API always shows rather than have the frontend deal with aliases. I don't think it matters, for example, if we show DECIMAL or NUMERIC for the DB type since they're the same thing.

I'm in support of closing this PR and repurposing #1037 to do the work of ensuring that the API has a single name for a single type. I do think the API should be able to accept aliases (e.g. you could create a type as either DECIMAL or NUMERIC, and it would always reflect as NUMERIC), but we can handle that post-alpha if needed.

@pavish
Copy link
Member

pavish commented Mar 22, 2022

@dmos62 Since we are all in agreement, I'm going to close this PR.

@pavish pavish closed this Mar 22, 2022
@kgodey kgodey deleted the add_alias_info_to_db_types_endpoint branch March 22, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: architecture Improvements or additions to architecture pr-status: review A PR awaiting review work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add database type alias information to database type endpoint
4 participants