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

Solve some of the array of custom type issues #92

Merged
merged 10 commits into from
Oct 28, 2017
Merged

Solve some of the array of custom type issues #92

merged 10 commits into from
Oct 28, 2017

Conversation

benjie
Copy link
Member

@benjie benjie commented Oct 13, 2017

Fixes #83

@benjie
Copy link
Member Author

benjie commented Oct 13, 2017

(The tests are failing because some of the schema tests only expose schema c whereas the new types are defined in schema a.)

@benjie
Copy link
Member Author

benjie commented Oct 28, 2017

Oh. My. Gosh.

So this took a bit to solve. Issue relates to getting types/classes from private schemas when they're required indirectly by items in the public schema (e.g. where a column is an array of a private type).

At first I tried rewriting our introspection query, but doing so turned out to be much more complex than I first anticipated (would have required a recursive CTE to pull in extra types at each level if they were required by the previous one).

Then I tried just expanding what the introspection query gives us, but that of course caused conflicts due to a poor architecture decision I made early on.

So I thought, why not filter it to just the required parts in the introspection query; but of course this would only work with default plugins and would make it harder for others to extend the system.

In the end I decided to correct my architectural decision - types are no longer defined eagerly ahead of time, instead you register a generator and then you call the generator when you need the type. (Even this turned out to be more complex than expected!)

This could all do with a serious refactor, but being short on time as I am we'll have to make do for now and refactor for v5.

On the plus side, this should avoid a number of the issues we were getting where types in private schemas that aren't ultimately exported were clashing with types in the public schema.

rel.relnamespace in (select "id" from namespace) or
rel.reltype in (select "returnTypeId" from procedure) or
rel.reltype in (select unnest("argTypeIds") from procedure)
) and
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the crux of the issue, making this filter broad enough was a real challenge. Instead I removed it and perform on-demand type definition rather than eagerly creating all the classes.

Copy link
Member Author

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

Approved 👍

@benjie benjie merged commit 2a7ee5e into master Oct 28, 2017
@benjie benjie deleted the fix-83 branch October 28, 2017 10:47
@chadfurman
Copy link

wow @benjie so this is what you've been up to! Great work!

@pyramation
Copy link

thanks @benjie looking forward to using this ASAP!

@benjie
Copy link
Member Author

benjie commented Oct 29, 2017

It's in the latest release of postgraphql@next/postgraphile - let me know if it works 👍

@pyramation
Copy link

looks like a lot great work! From what I reviewed, looks good :)

If I'm correct, graphile-build-pg is still querying all the types on server via psql, however, types don't leave the server to the client unless they are used in the schema that is being explicitly requested.

Solid work!!! Thanks again @benjie

@benjie
Copy link
Member Author

benjie commented Oct 29, 2017

Yeah, they effectively spider out from the tables/functions/views in the exposed schemas. By the way more integration tests would be welcome if there’s certain ways you’re using the system and you want to ensure they aren’t subtly changed without good cause.

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.

3 participants