Skip to content

OpenAPI - Distinguish between stored procedure parameters and result set columns#1551

Merged
seantleonard merged 18 commits intomainfrom
dev/seantleonard/openapi_sp_params
Jul 12, 2023
Merged

OpenAPI - Distinguish between stored procedure parameters and result set columns#1551
seantleonard merged 18 commits intomainfrom
dev/seantleonard/openapi_sp_params

Conversation

@seantleonard
Copy link
Copy Markdown
Contributor

@seantleonard seantleonard commented Jun 27, 2023 •
edited
Loading

Why make this change?

What is this change?

  • The OpenApiDocumentor service now correctly distinguishes between input parameters and result set columns when documenting a stored procedures REST endpoint. Previously, only the result set columns were resolved. As a result, the OpenApi document that is generated now shows input parameters as fields to be used in the request bodies of POST, PUT, and PATCH. (GET and DELETE have no request bodies).

How was this tested?

  • Integration Tests - Added tests with additional stored procedures in our sql deployment script which demonstrates how parameters and output result set columns (and their JSON data types) are resolved and included in the open api doc.

OpenAPI document example

An example OpenAPI document which helps visualize how the parsing works. Irrelevant fields were removed.

{
    "openapi": "3.0.1",
    "paths": {
        "/sp1": {
            "get": {"..."},
            "post": {
                "tags": [
                    "sp1"
                ],
                "description": "Executes a stored procedure.",
                "requestBody": {
                    "content": {
                        "application/json": {
                            "schema": {
                                "$ref": "#/components/schemas/sp1_sp_request"
                            }
                        }
                    },
                    "required": true
                },
                "responses": {
                    "201": {
                        "description": "Created",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "properties": {
                                        "value": {
                                            "type": "array",
                                            "items": {
                                                "$ref": "#/components/schemas/sp1_sp_response"
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    },
                    "400": {
                        "description": "BadRequest"
                    }
                }
            }
		}
    },
    "components": {
        "schemas": {
            "sp1_sp_request": {
                "type": "object",
                "properties": {
                    "inputName": {
                        "type": "string",
                        "format": ""
                    }
                }
            },
            "sp1_sp_response": {
                "type": "object",
                "properties": {
                    "outputName": {
                        "type": "string",
                        "format": ""
                    }
                }
            }
        }
    }
}

@seantleonard
Improve documentation of StoredProcedures by distinguishing between p…
e71c421 
…arameters and result set columns for the request and response payloads.
@seantleonard seantleonard linked an issue Jun 27, 2023 that may be closed by this pull request
[Bug]: Inconsistent Example Schema for Request Body of Stored-Procedure in Swagger UI #1509
Closed
1 task
@seantleonard seantleonard marked this pull request as ready for review July 6, 2023 00:59
Aniruddh25
Aniruddh25 approved these changes Jul 8, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Looks good! Left few minor suggestions.

Comment thread src/Service.Tests/DatabaseSchema-MsSql.sql Outdated
Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs Outdated
Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs Outdated
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs Outdated
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs Outdated
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs Outdated
@abhishekkumams
Copy link
Copy Markdown
Contributor

abhishekkumams commented Jul 10, 2023

you mentioned in PR description that GET and DELETE won't have request bodies, but they can have input params, right? So, why not request bodies?

@seantleonard
Consollidate openapireference object validation into a helper which a…
60df2ec 
…sserts the necessary characteristics of a request and response body .

updates comments.
@seantleonard
Copy link
Copy Markdown
Contributor Author

seantleonard commented Jul 10, 2023

you mentioned in PR description that GET and DELETE won't have request bodies, but they can have input params, right? So, why not request bodies?

Request bodies in those two request types are not convention per HTTP spec:

  1. GET https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.1-6
  2. DELETE https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.5-6

Relevant text with my annotation in square brackets:

A client SHOULD NOT generate content in a [GET/DELETE] request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported.

@seantleonard
Utilizes existing stored procedure for tests which demonstrates the s…
45b28ef 
…cenarios under test: validating input parameters and json data types and output result set columns and their json data types. Removes new SP's added from the .sql deploy file.
@seantleonard
Merge branch 'main' into dev/seantleonard/openapi_sp_params
7275207
Aniruddh25
Aniruddh25 reviewed Jul 10, 2023
View reviewed changes
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs Outdated
severussundar
severussundar reviewed Jul 12, 2023
View reviewed changes
Comment thread src/Directory.Packages.props Outdated
severussundar
severussundar approved these changes Jul 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@severussundar severussundar left a comment

Choose a reason for hiding this comment

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

LGTM

@ayush3797
Copy link
Copy Markdown
Contributor

ayush3797 commented Jul 12, 2023

Nit: The comment here:

data-api-builder/src/Core/Services/OpenAPI/OpenApiDocumentor.cs

Line 169 in 73716e7

// Entities which disable their REST endpoint must not be included in
is repeated at line 178.

@ayush3797
Copy link
Copy Markdown
Contributor

ayush3797 commented Jul 12, 2023

It might not be relevant for this PR but I think we default stored procedures to atleast support POST operation if no method is defined. This doesn't seem to be the case here:

data-api-builder/src/Core/Services/OpenAPI/OpenApiDocumentor.cs

Line 456 in 73716e7

if (spRestMethods is null)

@seantleonard
Copy link
Copy Markdown
Contributor Author

seantleonard commented Jul 12, 2023

It might not be relevant for this PR but I think we default stored procedures to atleast support POST operation if no method is defined. This doesn't seem to be the case here:

data-api-builder/src/Core/Services/OpenAPI/OpenApiDocumentor.cs

Line 456 in 73716e7

if (spRestMethods is null)

Good catch. The sp default methods are added (POST) when RuntimeEntities is created and ProcessRestDefaults() is executed. This also looks to be artifacts of old config system where the collection may be null. The nullable annotation and null check are not necessary.

@seantleonard seantleonard enabled auto-merge (squash) July 12, 2023 19:08
@seantleonard seantleonard merged commit bda0d65 into main Jul 12, 2023
@seantleonard seantleonard deleted the dev/seantleonard/openapi_sp_params branch July 12, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@severussundar severussundar severussundar approved these changes

@Aniruddh25 Aniruddh25 Aniruddh25 approved these changes

@mbhaskar mbhaskar Awaiting requested review from mbhaskar

@JelteF JelteF Awaiting requested review from JelteF

@Mathos1432 Mathos1432 Awaiting requested review from Mathos1432

@gledis69 gledis69 Awaiting requested review from gledis69

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@milismsft milismsft Awaiting requested review from milismsft

@tarazou9 tarazou9 Awaiting requested review from tarazou9

@ayush3797 ayush3797 Awaiting requested review from ayush3797

@abhishekkumams abhishekkumams Awaiting requested review from abhishekkumams

@aaronpowell aaronpowell Awaiting requested review from aaronpowell

@ravishetye ravishetye Awaiting requested review from ravishetye

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Inconsistent Example Schema for Request Body of Stored-Procedure in Swagger UI

5 participants

@seantleonard @abhishekkumams @ayush3797 @Aniruddh25 @severussundar