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 support for enum values to @pgQuery #786

Closed
wants to merge 1 commit into from
Closed

Add support for enum values to @pgQuery #786

wants to merge 1 commit into from

Conversation

angelyan
Copy link

Description

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

Fixes graphile/crystal#1601.

Performance impact

unknown

Security impact

unknown

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • 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).
  • If this is a breaking change I've explained why.

@benjie
Copy link
Member

benjie commented Apr 15, 2022

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

@shellscape
Copy link

@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)

@benjie
Copy link
Member

benjie commented Sep 27, 2023

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 benjie closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pgQuery for field returning enum fails
3 participants