-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Conversation
@seancolsen @pavish can either of you take a look and see if this suits frontend needs? I considered creating a separate endpoint, something like |
@dmos62 This looks good and makes it a lot easier for the frontend than having a separate endpoint. |
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. |
There was a problem hiding this 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?
@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 @pavish if getting rid of database aliases would solve your use case, feel free to close this PR. |
@dmos62 It's not only on the db types endpoint but also the
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 @kgodey I'd like your thoughts on this. |
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 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 |
@dmos62 Since we are all in agreement, I'm going to close this PR. |
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) andalias_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
isnull
, according to the information in PG's documentation, it's a canonical.Mentioned endpoint's output after changes:
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin