Skip to content

feat(pg): support domain constrained composite types#615

Merged
benjie merged 5 commits intographile:v4from
jcgsville:v4
Nov 13, 2020
Merged

feat(pg): support domain constrained composite types#615
benjie merged 5 commits intographile:v4from
jcgsville:v4

Conversation

@jcgsville
Copy link
Copy Markdown
Contributor

Addresses #600

Input types now support domains that constrain composite types instead of failing to generate the schema on startup.

I didn't add any testing as I was a bit unclear what the testing story is for a change like this. Happy to make any additions in this area with a quick point in the right direction.

@benjie
Copy link
Copy Markdown
Member

benjie commented Apr 15, 2020

Thanks @jcgsville; this is effectively a new type so we should add it to b.types:

Also add a mutation test for it in here:

https://github.com/graphile/graphile-engine/blob/v4/packages/postgraphile-core/__tests__/fixtures/mutations/types.graphql

And a query test here:

https://github.com/graphile/graphile-engine/blob/v4/packages/postgraphile-core/__tests__/fixtures/queries/types.graphql

These are all integration tests, effectively.

Changes to the above will require a lot of snapshot updates, you can run yarn --cwd packages/postgraphile-core jest -u to update them. You may need to fix a few other tests too - let the test results guide you.

Thanks!

Chandler Gonzales added 2 commits April 26, 2020 23:12
@jcgsville
Copy link
Copy Markdown
Contributor Author

Thanks for the guidance @benjie! I added some tests, which pass with Postgres 11, but it looks like Postgres 10 and earlier did not let you create a domain over a composite type, so the tests fail there. Is there a mechanism testing things only in certain versions?

@benjie
Copy link
Copy Markdown
Member

benjie commented Apr 29, 2020

@jcgsville Not a good one, but we did have to avoid PG10 tests running in PG9.6:

# Now run the tests
if [ $SERVER_VERSION_NUM -ge 100000 ]; then
jest --forceExit -i $@
elif [ $SERVER_VERSION_NUM -ge 90500 ]; then
jest --forceExit -i -t "^((?!pg10).)*$" $@
else
jest --forceExit -i -t "^((?!pg10|json-overflow).)*$" $@
fi;

I'm not happy with our testing system in general (it's very legacy, clunky and slow), and am planning to overhaul it as part of the work I'm doing on V5.

@benjie
Copy link
Copy Markdown
Member

benjie commented Jul 29, 2020

@jcgsville Are you able to add a relevant workaround for PG11 as indicated above?

Copy link
Copy Markdown
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.

Need the tests to be skipped on unsupported PostgreSQL versions. (Added this status to help me track the issue.)

@benjie
Copy link
Copy Markdown
Member

benjie commented Nov 13, 2020

Merged v4 branch and then moved the test of this feature to the PG11 test suite.

Copy link
Copy Markdown
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 for the fix 🙌

@benjie benjie changed the title feat(postgraphile): add support for domain constrained composite type… feat(pg): support domain constrained composite types Nov 13, 2020
@benjie benjie merged commit 215f5cf into graphile:v4 Nov 13, 2020
@jcgsville
Copy link
Copy Markdown
Contributor Author

Sorry to have not gotten to the testing tweaks sooner. Thanks for ferrying it the rest of the way through 🙂

@benjie
Copy link
Copy Markdown
Member

benjie commented Nov 15, 2020

No worries 🙌

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.

2 participants