Implement support for Postgres enums #59

silasdavis · 2020-05-06T14:39:12Z

This allows pgtyped to generate code from queries involving database
enum types. The postgres enums will be converted to a string-valued const enum
in the generated code.

The getTypes reflection query now joins on pg_enum and accumulates
enum information as it collects other type information. I think it may
be possible to accomodate composite types by aggregating over the object
graph given by a suitable join query in a similar way.

@pgtyped/query now returns MappableTypes which are either a string
referencing a database type name or a fully-described Type. Type now
includes EnumType. This allows type definitions to be realised at
different stages in the reflection/mapping process and should allow for
custom overrides in config.

fixes #54

silasdavis · 2020-05-06T14:41:07Z

packages/cli/src/config.ts

-    user: t.union([t.string, t.undefined]),
-    dbName: t.union([t.string, t.undefined]),
-  }),
+  db: t.union([


convenient not to have to include this in config

This allows pgtyped to generate code from queries involving database enum types. The postgres enums will be converted to a string-valued const enum in the generated code. The `getTypes` reflection query now joins on pg_enum and accumulates enum information as it collects other type information. I think it may be possible to accomodate composite types by aggregating over the object graph given by a suitable join query in a similar way. @pgtyped/query now returns `MappableTypes` which are either a string referencing a database type name or a fully-described `Type`. `Type` now includes `EnumType`. This allows type definitions to be realised at different stages in the reflection/mapping process and should allow for custom overrides in config. Signed-off-by: Silas Davis <silas@monax.io>

adelsz

This looks great, thanks for the PR @silasdavis!
Enums will be a nice addition to the list of supported type and a good step towards other composite types.

Added some minor naming/refactoring comments here and there.
I would also propose to make enum names and values PascalCase by default, that should integrate them better into TS land.

packages/cli/src/index.ts

packages/example/config.json

adelsz · 2020-05-07T20:43:18Z

packages/example/config.json

@@ -11,11 +11,5 @@
      "emitTemplate": "{{dir}}/{{name}}.types.ts"
    }
  ],
-  "srcDir": "./src/",
-  "db": {


I suggest to keep the db config in config.json, not everyone will want to use docker

Actually my removing it was inspired by me not using docker and wanting to not have to modify the config to do so, at least for running the generator. I didn't realise env would override at that point.

How about we optimise for the non-docker case by making the config be for a standard localhost connection, the kind you get with docker run -e POSTGRES_PASSWORD=password -p 5432:5432 postgres? I think that is probably the lowest friction way to test locally.

As a side-note I think it would make sense to formally double up package/example as integration tests. For this PR I started off by defining the end result I wanted in package/example and really the example matter was essential for testing. I think it would work as a minimal integration test wired into CI, plus you know your example content works. What do you think?

Wrapping package/example with integration testing is a good idea, we should do that.
Eventually, we will want to decouple the example from the integration tests, but for now it should work just fine.

packages/example/docker-compose.yml

packages/query/src/type.ts

adelsz · 2020-05-07T20:55:39Z

packages/query/src/actions.ts

+SELECT pt.oid, pt.typname, pt.typtype, pe.enumlabel
+FROM pg_type pt
+LEFT JOIN pg_enum pe ON pt.oid = pe.enumtypid
+WHERE pt.oid IN (${usedTypesOIDs.join(',')})`,


Soon PgTyped will be self-hosting and this query will be typed by PgTyped, hehe

Oh I was thinking about that...

Okay so I couldn't resist. I generated the necessary SQL but I the issue i came up against was that I cannot run the generated query against AsyncQueue. It looks like the current interface needs the 'extended query' message to be supported on your wire package to support bindings as required by IDatabaseConnection. Perhaps we can handle binding substitution on this side of the pipe. It would be nice if AsyncQueue supported IDatabaseConnection other than that I think I have this self-hosted.

Haha okay nope I think you do supported the prepared message flow... I see it breaks down into the various parse messages. I will have to have a go at this in a future PR :)

I won't bother to ask why you implemented the wire protocol... it does seem pretty neat. But why did you :) ? Was there a particular frustration with pg, or a hard requirement, or was it just because it was there (I would certainly consider using a small library like this written in typescript directly over pg if it did enough...)?

Yes, self-hosting isn't ready yet :)
Back when I implemented the wire protocol, I couldn't find a good TS native package that provided packet level primitives for interacting with PostgreSQL. These low level messages are needed to query the DB for type data. Another reason was to potentially evolve PgTyped into a self-contained Postgres client. I think there is a good possibility to improve the reliability and performance of existing client, but this is a side goal and not a priority at the moment.

packages/query/src/actions.ts

silasdavis · 2020-05-08T10:13:24Z

Made all of your changes with variation on config which is closer to running out of the box without docker.

Also refactored the type row reduction for clarity and better testing. Added snapshot tests based on that that are in with a chance of surviving us adding additional fields to the type rows query output.

silasdavis · 2020-05-08T10:39:57Z

I would also propose to make enum names and values PascalCase by default

I did consider this, but I leaned towards not being too clever and leaving them as-is. Reasons:

The enum labels in Postgres are defined tokens in their own right it feels like providing another casing for them by default could cause some confusion in a codebase
There is a potential negative effect on code that uses a reverse lookup, suppose we have:

const enum Foo {
  bar = 'bar',
  baz = 'baz',
}

Then the reverse lookup Foo['bar'] is truthy as you would expect if passing in a string provided externally to check membership.

However if instead we generate:

const enum Foo {
  Bar = 'bar',
  Baz = 'baz',
}

Then Foo['bar'] is undefined which could break code written against code passing enum values around.

If we implement Support type overrides in config #60 then the aesthetes can satisfy themselves by explicitly mapping to a enum they control (admittedly they would need to generate the values, though we could potentially figure out a way to special-case logic where if you only provide a name against database enum we fill in the values for you, though this is probably not worth it.)

adelsz · 2020-05-08T11:08:14Z

@silasdavis thanks for all the updates, I will be looking through them a bit later today.
Also, can I kindly ask you to put new code changes into new commits to make tracking change history and reviewing a bit easier for me? Github UI doesn't play nice with branch force-pushes, requiring me to do manual git diffs between force-pushed commit hashes.

silasdavis · 2020-05-08T12:07:50Z

Ugh yeah sorry force of habit... Let me see what I can do with the help of my reflog on that front...

Also respond to various issues from review: - Tweak names - Make db config work with localhost Signed-off-by: Silas Davis <silas@monax.io>

silasdavis · 2020-05-08T12:15:39Z

Second commit has changes post-review

adelsz · 2020-05-08T16:29:21Z

Re: Enums

I think it is useful to have PgTyped transform postgres types to match TS style guidelines by default, potentially even converting snake-case field names into camel-case. Of course it should also be possible to opt-out of such behavior via the config.
This largely grows out of my frustration with existing JS DB tools that require you to camelcase identifiers manually before and after each DB interaction.

The good thing about PgTyped is that this conversion can now be deterministic because the DB<->JS interface is typed.

For this PR, I think we can merge it as is, but I do want to enable code style transforms by default before next release (with an opt-out switch ofc). Stabilizing this part of the PgTyped API for the end users is important, given its potential to cause generated code breaking changes later on.

silasdavis · 2020-05-08T21:13:08Z

How about adding the verbatim case enum values as well as the converted pascal case ones (is they differ). Another degenerate case is where two different enum values become identical when pascalcased.

This largely grows out of my frustration with existing JS DB tools that require you to camelcase identifiers manually before and after each DB interaction

Can you give an example?

adelsz · 2020-05-09T09:01:02Z

How about adding the verbatim case enum values as well as the converted pascal case ones (is they differ)

How would that look?

Another degenerate case is where two different enum values become identical when pascalcased.

It is a very rare case imo, it has to be an enum that looks like ENUM ('sad', 'Sad', 'happy', 'Happy)`. The general goal is to provide great defaults for most users and have an escape hatch for those who have rare edge-cases.

Can you give an example?

Yes, node-postgres, knex and most other DB libraries send/return field names as is from the DB. On the other hand, most JS linter defaults enforce camelcase. This requires the user to write/use case adapters which are not typesafe.

silasdavis · 2020-05-09T15:25:18Z

most JS linter defaults enforce camelcase.

That's the bit I was missing, ESLint seems to let me get away with this with what I thought were pretty standard rules in any case.

How would that look?

enum {
  foo = 'foo'
  Foo = 'foo'
}

Appears to be allowed at least. It would make the membership test work. That's the thing I'd be most reluctant to break. However if you have a linter that is being strict about enum label then presumably that wouldn't work.

It is a very rare case imo,

We still would need to do something - either fail, emit code that doesn't compile, or just output the verbatim enum values. Presumably the latter is best.

Another potential issue, I'm not sure if enum labels limited to latin scripts or can they be unicode. If the latter then there may exist no case conversion.

Personally, I would make this kind of cleverness an opt-in rather than an opt-out. But I see your point and you have the benefit of being the benevolent dictator here :) so happy to implement. Shall we address in a subsequent PR?

adelsz · 2020-05-09T20:18:00Z

Actually, having thought about it a bit more, I agree it does make sense to keep the current behavior as default. Changing case by default can be surprising and given the possible edge-cases it is best to provide it behind a config setting. Thanks for the discussion there, that was helpful. I will open a separate issue for implementing case conversion and a flag for it.

adelsz

Thanks @silasdavis!

silasdavis · 2020-05-11T10:50:35Z

That's great, it speaks well of a maintainer who can change their mind on occasion :)

Any thoughts on when this will make it into a release?

adelsz · 2020-05-11T22:39:40Z

Released in v0.7.0 :)

silasdavis mentioned this pull request May 6, 2020

Fix unhandled promise rejection from generator #53

Closed

silasdavis commented May 6, 2020

View reviewed changes

silasdavis force-pushed the enums branch from e651302 to 3537908 Compare May 6, 2020 14:47

silasdavis force-pushed the enums branch 2 times, most recently from ae72ada to 035401b Compare May 6, 2020 21:11

adelsz requested changes May 7, 2020

View reviewed changes

silasdavis force-pushed the enums branch from 035401b to 8a078dc Compare May 8, 2020 10:09

silasdavis force-pushed the enums branch from 8a078dc to 9cd1644 Compare May 8, 2020 10:57

silasdavis force-pushed the enums branch 2 times, most recently from 8b280e6 to 93998b2 Compare May 8, 2020 11:26

Add database catalog types row reduction test

ef6016f

Also respond to various issues from review: - Tweak names - Make db config work with localhost Signed-off-by: Silas Davis <silas@monax.io>

silasdavis force-pushed the enums branch from 93998b2 to ef6016f Compare May 8, 2020 12:13

adelsz approved these changes May 9, 2020

View reviewed changes

adelsz merged commit 0d87ab0 into adelsz:master May 9, 2020

silasdavis deleted the enums branch May 11, 2020 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for Postgres enums #59

Implement support for Postgres enums #59

silasdavis commented May 6, 2020 •

edited

Loading

silasdavis May 6, 2020

adelsz left a comment •

edited

Loading

adelsz May 7, 2020

silasdavis May 8, 2020

adelsz May 8, 2020

adelsz May 7, 2020

silasdavis May 8, 2020

silasdavis May 8, 2020

silasdavis May 8, 2020 •

edited

Loading

adelsz May 8, 2020

silasdavis commented May 8, 2020

silasdavis commented May 8, 2020 •

edited

Loading

adelsz commented May 8, 2020

silasdavis commented May 8, 2020

silasdavis commented May 8, 2020

adelsz commented May 8, 2020

silasdavis commented May 8, 2020

adelsz commented May 9, 2020 •

edited

Loading

silasdavis commented May 9, 2020 •

edited

Loading

adelsz commented May 9, 2020

adelsz left a comment

silasdavis commented May 11, 2020

adelsz commented May 11, 2020

Implement support for Postgres enums #59

Implement support for Postgres enums #59

Conversation

silasdavis commented May 6, 2020 • edited Loading

Choose a reason for hiding this comment

adelsz 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

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

silasdavis May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silasdavis commented May 8, 2020

silasdavis commented May 8, 2020 • edited Loading

adelsz commented May 8, 2020

silasdavis commented May 8, 2020

silasdavis commented May 8, 2020

adelsz commented May 8, 2020

silasdavis commented May 8, 2020

adelsz commented May 9, 2020 • edited Loading

silasdavis commented May 9, 2020 • edited Loading

adelsz commented May 9, 2020

adelsz left a comment

Choose a reason for hiding this comment

silasdavis commented May 11, 2020

adelsz commented May 11, 2020

silasdavis commented May 6, 2020 •

edited

Loading

adelsz left a comment •

edited

Loading

silasdavis May 8, 2020 •

edited

Loading

silasdavis commented May 8, 2020 •

edited

Loading

adelsz commented May 9, 2020 •

edited

Loading

silasdavis commented May 9, 2020 •

edited

Loading