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

fix(postgres): cast list of enums to their correct type (#417) #463

Closed
wants to merge 16 commits into from

Conversation

danielbuechele
Copy link
Contributor

@danielbuechele danielbuechele commented May 11, 2017

This adds an explicit type cast to enum arrays

Fixes #417

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Thanks @danielbuechele, this is a much requested feature!

I'm not confident in the sql.raw with the notNullType.name; can we use sql.identifier here instead? Something like

const listTypeCast = sql.query(`::${sql.identifier(nonNullType instanceof PgEnumType ? nonNullType.name : 'text')}[]`)

I'm not sure if that will work?

@danielbuechele
Copy link
Contributor Author

Now using sql.identifier. Also changed it to work not only with enums but custom classes, too.

@benjie
Copy link
Member

benjie commented May 17, 2017

This looks great @danielbuechele! Are you able to add any tests for it?

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

I'm not comfortable merging this until there are some tests in place (because I don't understand enough of PostGraphQL to be certain this won't cause any issues).

@danielbuechele
Copy link
Contributor Author

Added test for the function I modified

@danscan
Copy link

danscan commented Jul 9, 2017

@benjie @danielbuechele What is the status of this? Looks like it's just blocked by some tests. I can add these if that'd be helpful.

@benjie
Copy link
Member

benjie commented Jul 9, 2017

@danscan I think we just need CI to pass before we can merge.

@benjie
Copy link
Member

benjie commented Jul 9, 2017

It seems to just be lint issues that need resolving.

@danielbuechele
Copy link
Contributor Author

I'll fix them tomorrow! (and get my linter working)

@danielbuechele
Copy link
Contributor Author

Huh, linter is now passing, but a lot of other test failing. I don't really know where this is coming from.

@benjie
Copy link
Member

benjie commented Jul 10, 2017

The errors centre around:

GraphQLError: function c.types_mutation(unknown, unknown, unknown, text[], unknown, numrange) does not exist
GraphQLError: function c.types_query(unknown, unknown, unknown, text[], unknown, numrange) does not exist

Here's the relevant definitions:

https://github.com/postgraphql/postgraphql/blob/886f8752f03d3fa05bdbdd97eeabb153a4d0343e/examples/kitchen-sink/schema.sql#L155-L156

Seems the integer array is being interpreted as a text array instead.

@benjie
Copy link
Member

benjie commented Oct 29, 2017

[semi-automated message] Hi, there has been no activity in this issue for over 8 weeks so I'm closing it to keep the issues/pull request manageable. If this is still an issue, please re-open with a comment explaining why.

@benjie benjie closed this Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants