[GraphQL] Updates to Stored Procedure support for Engine

[yorek]

Summary

Add a new execute permission to make it easier to publish stored procedures

Motivation

Supporting Stored Procedure execution via methods other than POST for REST and Queries for GraphQL creates some confusion on how permissions are defined and also it leads to a potential break of best practices regarding the idempotency of non-POST methods.

Functional Specification

To best support Stored Procedure in Data API Builder, we may want to introduce a new action, named execute that

• can be used only if the published object is a stored procedure
• must be the only action defined in a role
• REST: will be mapped to the POST method by default
• GraphQL: will generate a Mutation by default

Users will be able to decide if they want to use another method other than POST and generating a Query instead of a Mutation by defining their preference using the rest or graphql objects that are optionally available withing the entity object. For example:

"GetCowrittenBooksByAuthor": {      
  "source": {
	"type": "stored-procedure",
	"object": "dbo.stp_get_all_cowritten_books_by_author",
	"parameters": {
	  "author": "?",
	  "searchType": "c"
	}
  }, 
  "graphql":{
	"operation": "mutation" 
  },
  "rest": {
	"method": ["GET"]
  },           
  "permissions": [
	{
	  "role": "anonymous",
	  "actions": [ "execute" ]
	}
  ]
}

With this approach we can provide developers the best behaviour by default, but let them decide what is best for them if they need to depart from the best practices for any reasons.

References

[GraphQL] Stored Procedure support for Engine
[REST] Update/Create is breaking with Single Permission
Support stored procedure
Behavior of Update Action for Stored-Procedures in CLI

[Aniruddh25]

I think you meant to set execute in the example above:
"actions": [ "execute" ]
instead of
"actions": [ "read" ]

Just clarifying, the developer can override the default method for REST (and operation for GraphQL) but they CANNOT override the action execute right?

If execute is absent, but read is present in the permissions, then we will NOT permit the stored procedure execution even if the overridden method in REST is GET. Is that right? This will be a breaking change as compared to 0.4.11 - is that fine?

[yorek]

That is right, I have corrected it now.
Also correct the fact that the execute action CANNOT be overridden. execute is the only action possible for stored procedures.
If anything else other than execute is specified for a stored procedure, an error must be generated.
Yes, it is a breaking change. We'll make sure to document it very well when we release the next version. I'm ok with this as for now the amount of people using DAB is very limited, so the impact should be minimal. Let's make sure to add this change before we go Public Preview, after that it will be more complex to add breaking changes.

[seantleonard]

Do we want to have the following restrictions for the operations defined for an Stored Procedures REST/GraphQL operations?

Key:

• Columns = GraphQL
• Rows = REST

	Query	Mutation
GET	X
POST		X
PUT		X
PATCH		X
DELETE		X

e.g. If GraphQL operation is mutation, REST operation cannot be GET.

[Aniruddh25]

REST: will be mapped to the POST method by default

By default, does it mean REST on stored procedure entities won't support any of the other mutation operations - PUT/PATCH/DELETE ? These methods will only be allowed if explicitly configured, otherwise they will result in a bad request.

[Aniruddh25]

Do we want to have the following restrictions for the operations defined for an Stored Procedures REST/GraphQL operations?

Key:

• Columns = GraphQL
• Rows = REST

Query Mutation
GET X
POST X
PUT X
PATCH X
DELETE X
e.g. If GraphQL operation is mutation, REST operation cannot be GET.

If the developer is explicitly configuring REST method to be GET, and specifies nothing for GraphQL, they will end up in a situation where default is picked for GraphQL which is mutation. If we restrict this, the developer has to modify the config file, to override the default operation for GraphQL to be query, which seems additional overhead if I am only using REST.
I dont think we should restrict if they configure incompatible REST/GraphQL operations - the fact that they override the defaults means they understand what they are doing so we rely on what they have specified.

[Aniruddh25]

"rest": {
	"method": "GET"
  }
"

Shouldn't method here be an array? To enable multiple methods for the same stored procedure if the developer so chooses?

[severussundar]

• CLI options

operation for graphQL and method for REST would be new elements in the json schema. So, new CLI options are required for the users to be able to configure the values.

Suggestion: --rest.method and --graphql.operation. Please let me know if this is fine for the option names.

These would be optional fields (just like rest and graphQL are, at the moment). Not specifying anything will result in REST: POST and GraphQL: Mutation being created.

• For REST, are PUT, PATCH and DELETE supported?

[seantleonard]

Also want to identify how to name the GraphQL mutation with the new operation "execute".
Currently graphQL mutations are named according to the operation permitted: createEntity updateEntity deleteEntity. Would we want to now define executeEntity?

[yorek]

"rest": {
	"method": "GET"
  }
"

Shouldn't method here be an array? To enable multiple methods for the same stored procedure if the developer so chooses?

That make sense.

[yorek]

Also want to identify how to name the GraphQL mutation with the new operation "execute". Currently graphQL mutations are named according to the operation permitted: createEntity updateEntity deleteEntity. Would we want to now define executeEntity?

Yes, that makes sense and is coherent with what we do already, so let's go with this. A developer can always specified the name the want to use via the graphql section if they don't like to have stored procedure prefixed with "execute"

[yorek]

• CLI options

operation for graphQL and method for REST would be new elements in the json schema. So, new CLI options are required for the users to be able to configure the values.

Suggestion: --rest.method and --graphql.operation. Please let me know if this is fine for the option names.

These would be optional fields (just like rest and graphQL are, at the moment). Not specifying anything will result in REST: POST and GraphQL: Mutation being created.

• For REST, are PUT, PATCH and DELETE supported?

Following what @Aniruddh25 suggested I call the option --rest. methods and one will be able to provide comma-separated list of the methods they want ot be supported

[seantleonard]

Also want to identify how to name the GraphQL mutation with the new operation "execute". Currently graphQL mutations are named according to the operation permitted: createEntity updateEntity deleteEntity. Would we want to now define executeEntity?

Yes, that makes sense and is coherent with what we do already, so let's go with this. A developer can always specified the name the want to use via the graphql section if they don't like to have stored procedure prefixed with "execute"

@yorek, we currently do not allow developers to fully rename the query/mutation. Only the GraphQL singular/plural value of the entity. so in this case, we would always prefix with "execute" but the will be sourced from what the developer defines in entity.graphql.singular or entity.graphql.plural.

Query has an edge case too, where the queries entity_by_pk and entities are generated for getOne and getMany when the "Read" permission is defined. So, when a dev defines graphql: "Query" for a stored procedure, what should we generate?

[yorek]

@seantleonard ah, right. Let's go with the "execute" prefix then with no ability, at the moment, to remove it.

For GraphQL, if a developer wants to have the Stored Procedure exposed as a Query instead of a Mutation, only one query object will be generated (so no _by_pk object):

execute<EntityName>

Btw, just to make sure we're on the same page, even if "Mutation" will be chosed as the object to be generated, only one mutation will be produced named exactly as the query:

execute<EntityName>

What do you think?

[seantleonard]

@seantleonard ah, right. Let's go with the "execute" prefix then with no ability, at the moment, to remove it.

For GraphQL, if a developer wants to have the Stored Procedure exposed as a Query instead of a Mutation, only one query object will be generated (so no _by_pk object):

execute<EntityName>

Btw, just to make sure we're on the same page, even if "Mutation" will be chosed as the object to be generated, only one mutation will be produced named exactly as the query:

execute<EntityName>

What do you think?

@yorek, makes sense to me.

[ayush3797]

There are already rest and graphql properties present in the json schema for each entity. Inside these rest/graphql properties, I see that the path/type properties are required. Does it mean that when we want to specify the method/operation property inside the rest/graphql section, we need to specify the path/type properties as well?

[seantleonard]

see that the path/type properties are required.

Currently that is how i have it implemented but haven't added additional restrictions to make method/operation optional.

[yorek]

Using method or operation should be independent of having path or type specified. They are all optional.

[ayush3797]

If the rest/graphql are boolean properties, we are good. But when we want to specify the operation/method properties, we need to have rest/graphql properties as object rather than boolean. But current implementation mandates that we have to specify path/type properties (required properties) when rest/graphql properties are object. However, we only wanted to specify the method/operation properties and had nothing to do with the path/type properties. So maybe, we would have to make all the properties as "not required".

[Aniruddh25]

Closing the rfc since all implementing issues have been closed. The doc is in this PR: #1163