graphile-engine icon indicating copy to clipboard operation
graphile-engine copied to clipboard

Add support for enum values to @pgQuery

Open angelyan opened this issue 3 years ago • 2 comments

Description

Changes the isScalarType check in makeExtendSchemaPlugin to isLeafType to support enum values with @pgQuery.

Fixes https://github.com/graphile/postgraphile/issues/1601.

Performance impact

unknown

Security impact

unknown

Checklist

  • [x] My code matches the project's code style and yarn lint:fix passes.
  • [x] I've added tests for the new feature, and yarn test passes.
  • [ ] I have detailed the new feature in the relevant documentation.
  • [ ] I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • [x] If this is a breaking change I've explained why.

angelyan avatar Apr 14 '22 15:04 angelyan

Thank you for your work on this! Before this can be fully reviewed, it will need tests for enum edge cases. In GraphQL the internal values for enums can be basically anything, and we have various different types of enum in PostGraphile, so please add tests for:

  • a PostgreSQL enum value that has to be heavily transformed by the inflection system (e.g. lowercase, contains special symbols, etc)
  • an "enum table" enum value that has to be heavily transformed by the inflection system (https://www.graphile.org/postgraphile/enums/#with-enum-tables)
  • an enum that uses exotic internal values, for example an OrderBy enum value. (This almost certainly won't make sense, but it should fail in a graceful way rather than trying to send a tuple containing SQL fragments/etc into the DB.)

benjie avatar Apr 15 '22 09:04 benjie

@benjie its pretty cool that @angelyan did the work to get this started, but I'd wager based on the lack of response that they don't have the expertise to satisfy your test requirements. given that you do, please do consider adding the tests you're seeking. (it would be great if this wasn't punted to v5)

shellscape avatar Aug 23 '22 18:08 shellscape

Reviewing this again, I don't think that this will work for the edge cases that I laid out above. Since V5 is now available to use (and won't suffer from the underlying issue due to a different architecture) I'm going to close it.

benjie avatar Sep 27 '23 15:09 benjie