Support tracking VOLATILE SQL functions as mutations. Closes #1514 #5858

jberryman · 2020-09-29T00:22:02Z

Description

Support tracking VOLATILE functions as mutations (top-level fields under mutation root field). Support is added to the v2 API while remaining backwards compatible. See comments for API design decisions.

Changelog

CHANGELOG.md is updated with user-facing content relevant to this PR. If no changelog is required, then add the no-changelog-required label.

Affected components

Related Issues

#1514

Solution and Design

I re-used the machinery from support for tracking functions as queries and tried to do as little as possible.

Steps to test and verify

The tests written here represent the extent that I've tested/tried this out, so you can run those.

Limitations, known bugs & workarounds

The tests aren't particularly thorough; since so much of the queries code is re-used this seems all right to me.

Aggregations on the result set isn't implemented; it might be as simple as uncommenting the relevant line (I just noticed this as an unresolved question).

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

No
Yes
- Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

No
Yes
- Does run_sql auto manages the new metadata through schema diffing?
  - Yes
  - Not required
- Does run_sql auto manages the definitions of metadata on renaming?
  - Yes
  - Not required
- Does export_metadata/replace_metadata supports the new metadata added?
  - Yes
  - Not required

GraphQL

No new GraphQL schema is generated
New GraphQL schema is being generated:
- New types and typenames are correlated

Breaking changes

netlify · 2020-09-29T00:24:09Z

Deploy preview for hasura-docs ready!

Built with commit 4945ace

https://deploy-preview-5858--hasura-docs.netlify.app

srghma · 2020-10-18T11:59:26Z

Heterogeneous execution was merged #5869

server/src-lib/Hasura/GraphQL/Schema.hs

server/src-lib/Hasura/RQL/DDL/Schema/Function.hs

server/tests-py/queries/graphql_mutation/functions/function_as_mutations.yaml

CHANGELOG.md

docs/graphql/core/api-reference/schema-metadata-api/custom-functions.rst

docs/graphql/core/schema/custom-functions.rst

abooij · 2020-10-29T09:16:02Z

server/src-lib/Hasura/GraphQL/Execute/Query.hs

+-- | A pared-down version of 'convertQuerySelSet', for use in execution of
+-- special case of SQL function mutations (see 'MDBFunction').


I feel a bit iffy about adding a function, convertFunction, which is only used in Hasura.GraphQL.Execute.Mutation, but defined in Hasura.GraphQL.Execute.Query. It suggests there should be a module called Hasura.GraphQL.Execute.Common. Or is there a different way to resolve this?

Speaking of convertFunction, is it possible to have remote joins as part of a mutation custom function?

I feel a bit iffy ...

Yeah, I think it would be better to leave that for a separate PR, post- https://github.com/hasura/graphql-engine/pull/5797/files#diff-563b07c689cd5e65fabcd9d3915cb42bb1cc065d507af4ded924dba3fca9408c

Speaking of convertFunction, is it possible to have remote joins as part of a mutation custom function?

Do you mean "does that work in the PR?" or "is it possible to make it work?" ? I don't know the answer to either, but can look into it. Maybe a future enhancement?

kodiakhq · 2020-10-29T15:21:05Z

This PR currently has a merge conflict. Please resolve this and then re-add the auto-update-auto-merge label.

jberryman · 2020-10-29T22:58:13Z

@abooij Thanks for the review. Hopefully not coming across as stubborn in how I came down on some of the suggestions here, though I did carefully (re)consider and appreciate them all. Let me know if you feel strongly about what remains open!

...after we needed to re-use some Query.hs code in Mutate.hs No implementation changes. With the state of documentation of this part of the code it's hard to say whether this is moving towards a better module structure or is just an arbitrary change.

jberryman · 2020-11-02T21:21:15Z

@abooij please give a look over the last two commits. I'll rebase before merging. Thanks!

jberryman · 2020-11-02T21:29:46Z

@tirumaraiselvan can I get a changelog check please

tirumaraiselvan · 2020-11-03T03:16:58Z

CHANGELOG.md

+### Support tracking VOLATILE SQL functions as mutations. (closing #1514)
+
+Previously we could only track `STABLE` or `IMMUTABLE` functions, and only as
+queries. Now the version 2 of `track_table` also supports tracking functions as


typo track_table?

tirumaraiselvan

Changelog LGTM barring small typo.

abooij

Thanks, @jberryman!

docs/graphql/core/api-reference/schema-metadata-api/custom-functions.rst

docs/graphql/core/schema/custom-functions.rst

tirumaraiselvan

Metadata spec review

jberryman · 2020-11-03T16:25:22Z

After discussion with @tirumaraiselvan we're going to rework this with a more permissive API wrt volatility:

new field (replacing as_mutation): expose_as: query or expose_as: mutation
- if included we'll place the function where requested, regardless of its volatility
- ...with the exception that non-volatile functions requested exposed as mutations will raise an error, since this probably indicates a mistake either in choice of function, or when defining the function
- we'll raise a warning in some way whenever the volatility and expose_as are "mismatched"
if that field is omitted, we'll infer expose_as from the volatility (VOLATILE implies mutation, otherwise query)

jberryman · 2020-11-03T17:57:03Z

Mmm, seems I lost the race with fd8d51a ...

Since Schema.hs will now need a full rewrite and we're changing the API significantly i'm going to close this and open a new PR that can be reviewed from scratch, which I think will be more clear.

jberryman · 2020-11-03T21:27:19Z

Progress here is blocked on some deeper review of Schema.hs changes in metadata separation PRs, see discussion in slack

jberryman · 2020-11-05T20:50:06Z

@marionschleifer thanks! I tried to incorporate those changes into v2 of this work, though there are a lot of changes

This also supports tracking VOLATILE functions as queries. Review and discussion of the initial version of this work can be found in this closed PR: hasura#5858

jberryman force-pushed the 1514-sql-function-mutations branch from 164b021 to f5a57e9 Compare October 3, 2020 22:41

jberryman force-pushed the 1514-sql-function-mutations branch 3 times, most recently from 0137bc9 to 0ba38d0 Compare October 28, 2020 15:28

jberryman marked this pull request as ready for review October 28, 2020 16:10

jberryman requested review from a team as code owners October 28, 2020 16:10

jberryman force-pushed the 1514-sql-function-mutations branch from 0ba38d0 to 731bead Compare October 28, 2020 16:37

jberryman added auto-update-auto-merge labels Oct 28, 2020

jberryman requested review from abooij and rakeshkky October 28, 2020 16:50

jberryman changed the title ~~WIP~~ Support tracking VOLATILE SQL functions as mutations. Closes #1514 Oct 28, 2020

jberryman linked an issue Oct 28, 2020 that may be closed by this pull request

extend support for custom SQL functions (mutations, other functions types) #1514

Closed

abooij reviewed Oct 29, 2020

View reviewed changes

kodiakhq bot removed the auto-update-auto-merge label Oct 29, 2020

jberryman added kodiak:merge.method='squash' and removed kodiak:merge.method='merge' labels Oct 29, 2020

jberryman removed the request for review from rakeshkky October 29, 2020 15:34

Support tracking VOLATILE SQL functions as mutations. Closes hasura#1514

9a29d9e

jberryman mentioned this pull request Oct 29, 2020

ParseError doesn't seem to result in 400 status returned #6106

Open

SQUASHME review fixups

cf5a236

jberryman force-pushed the 1514-sql-function-mutations branch from 731bead to cf5a236 Compare October 29, 2020 22:53

jberryman added the auto-update-auto-merge label Oct 29, 2020

jberryman requested a review from abooij November 2, 2020 14:53

SQUASHME review fixups 2

62b8b87

jberryman added kodiak:merge.method='merge' and removed auto-update-auto-merge labels Nov 2, 2020

jberryman force-pushed the 1514-sql-function-mutations branch from 074e4f4 to 4072bb2 Compare November 2, 2020 21:19

Merge branch 'master' into 1514-sql-function-mutations

4945ace

tirumaraiselvan reviewed Nov 3, 2020

View reviewed changes

tirumaraiselvan approved these changes Nov 3, 2020

View reviewed changes

abooij approved these changes Nov 3, 2020

View reviewed changes

marionschleifer requested changes Nov 3, 2020

View reviewed changes

jberryman added this to the v1.3.3 milestone Nov 3, 2020

tirumaraiselvan requested changes Nov 3, 2020

View reviewed changes

jberryman modified the milestones: v1.3.3, v1.4 Nov 3, 2020

jberryman closed this Nov 3, 2020

jberryman mentioned this pull request Nov 10, 2020

Support tracking SQL functions as mutations. Closes #1514 #6160

Closed

34 tasks

tirumaraiselvan removed this from the v1.4 milestone Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support tracking VOLATILE SQL functions as mutations. Closes #1514 #5858

Support tracking VOLATILE SQL functions as mutations. Closes #1514 #5858

jberryman commented Sep 29, 2020 •

edited

Loading

netlify bot commented Sep 29, 2020 •

edited

Loading

srghma commented Oct 18, 2020

abooij Oct 29, 2020

jberryman Oct 29, 2020

kodiakhq bot commented Oct 29, 2020

jberryman commented Oct 29, 2020

jberryman commented Nov 2, 2020

jberryman commented Nov 2, 2020

tirumaraiselvan Nov 3, 2020

tirumaraiselvan left a comment

abooij left a comment

tirumaraiselvan left a comment

jberryman commented Nov 3, 2020

jberryman commented Nov 3, 2020 •

edited

Loading

jberryman commented Nov 3, 2020

jberryman commented Nov 5, 2020

		-- \| A pared-down version of 'convertQuerySelSet', for use in execution of
		-- special case of SQL function mutations (see 'MDBFunction').

Support tracking VOLATILE SQL functions as mutations. Closes #1514 #5858

Support tracking VOLATILE SQL functions as mutations. Closes #1514 #5858

Conversation

jberryman commented Sep 29, 2020 • edited Loading

Description

Changelog

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Metadata

GraphQL

Breaking changes

netlify bot commented Sep 29, 2020 • edited Loading

srghma commented Oct 18, 2020

abooij Oct 29, 2020

Choose a reason for hiding this comment

jberryman Oct 29, 2020

Choose a reason for hiding this comment

kodiakhq bot commented Oct 29, 2020

jberryman commented Oct 29, 2020

jberryman commented Nov 2, 2020

jberryman commented Nov 2, 2020

tirumaraiselvan Nov 3, 2020

Choose a reason for hiding this comment

tirumaraiselvan left a comment

Choose a reason for hiding this comment

abooij left a comment

Choose a reason for hiding this comment

tirumaraiselvan left a comment

Choose a reason for hiding this comment

jberryman commented Nov 3, 2020

jberryman commented Nov 3, 2020 • edited Loading

jberryman commented Nov 3, 2020

jberryman commented Nov 5, 2020

jberryman commented Sep 29, 2020 •

edited

Loading

netlify bot commented Sep 29, 2020 •

edited

Loading

jberryman commented Nov 3, 2020 •

edited

Loading