Skip to content

Mark stored procedure parameters without defaults as required in OpenAPI - #3687

Open
souvikghosh04 with Copilot wants to merge 16 commits into
mainfrom
copilot/fix-openapi-required-params
Open

Mark stored procedure parameters without defaults as required in OpenAPI#3687
souvikghosh04 with Copilot wants to merge 16 commits into
mainfrom
copilot/fix-openapi-required-params

Conversation

Copilot AI commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Why make this change?

  • Closes [Bug]: Required params not marked in OpenAPI #3498. The generated OpenAPI document for stored procedure entities omitted the required array on the {entity}_sp_request schema, so parameters that must be supplied (e.g. firstName, lastName) were not flagged as required.

What is this change?

  • CreateSpRequestComponentSchema (OpenApiDocumentor.cs): a parameter is now marked required when it is explicitly flagged in config or has no default value. Previously only an explicit config required: true populated the array, which is almost never set, so the array was always empty.
    • Mirrors the existing IsRequestBodyRequired logic (!HasConfigDefault), keeping body-level and property-level required semantics consistent. An explicit config required: false is still honored.
if (def.Required ?? !def.HasConfigDefault)
{
    required.Add(parameter);
}

How was this tested?

  • Integration Tests
  • Unit Tests

Extended StoredProcedureGeneration.ValidateRequestBodyContents to assert the request body schema's required set contains the expected parameters. Verified the assertion fails when the fix is reverted.

Sample Request(s)

GET /api/openapi — request body schema component for a stored procedure with parameters title, publisher_name (neither having a default):

"sp1_sp_request": {
  "type": "object",
  "properties": {
    "title": { "type": "string" },
    "publisher_name": { "type": "string" }
  },
  "required": [ "title", "publisher_name" ]
}

Copilot AI changed the title [WIP] Fix required params not marked in OpenAPI schema Mark stored procedure parameters without defaults as required in OpenAPI Jun 26, 2026
Copilot AI requested a review from souvikghosh04 June 26, 2026 08:24
@souvikghosh04

Copy link
Copy Markdown
Contributor

@copilot please add tests from to support the changes

Copilot AI commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please add tests from to support the changes

Added tests in StoredProcedureGeneration.ValidateRequestBodyContents covering all branches of the required-parameter logic:

  • sp1: parameters without defaults are all required.
  • sp2 (insert_book): a parameter with a config-provided default is not required.
  • sp3 (update_book_title): a parameter explicitly marked required: false is not required.

Each entity uses a distinct stored procedure so per-entity config metadata doesn't cross-contaminate (entities sharing a source share the same parameter definition). Verified all rows pass with the fix and fail when it's reverted, against a local SQL Server. (79f... latest commit)

@souvikghosh04

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@souvikghosh04

Copy link
Copy Markdown
Contributor

@copilot fix the formatting issue in StoredProcedureGeneration.cs as it fails in build

/home/vsts/work/1/s/src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs(151,152): error WHITESPACE: Fix whitespace formatting. Replace 18 characters with '\n\n\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s'. [/home/vsts/work/1/s/src/Service.Tests/Azure.DataApiBuilder.Service.Tests.csproj]
/home/vsts/work/1/s/src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs(154,1): error WHITESPACE: Fix whitespace formatting. Insert '\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s\s'. [/home/vsts/work/1/s/src/Service.Tests/Azure.DataApiBuilder.Service.Tests.csproj]

Copilot AI commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix the formatting issue in StoredProcedureGeneration.cs as it fails in build

/home/vsts/work/1/s/src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs(151,152): error WHITESPAC...

Fixed in the latest commit — corrected the indentation on the ISet<string> requiredParameters line (was 15 spaces, now 16). dotnet format --verify-no-changes passes cleanly.

@souvikghosh04

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@aaronburtle

Copy link
Copy Markdown
Contributor

This introduces a third required/default precedence rule across DAB. OpenAPI now uses Required ?? !HasConfigDefault, GraphQL and describe_entities use Required ?? true, and MCP custom tools make any parameter with a default optional, even when Required == true. This produces different contracts for Required == null/true with a config default. Please confirm the canonical policy from #3400 and centralize it in a shared or clearly named helper, with truth-table coverage for all Required × HasConfigDefault combinations.

Comment thread src/Core/Services/OpenAPI/OpenApiDocumentor.cs
Comment thread src/Service.Tests/OpenApiDocumentor/StoredProcedureGeneration.cs

@aaronburtle aaronburtle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good to go once the last comments are addressed!

@souvikghosh04

Copy link
Copy Markdown
Contributor

@copilot review the open/unresolved comments as fix what necessary and respond to each unresolved comments.

…rtions and GET query param tests

Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com>

Copilot AI commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

This introduces a third required/default precedence rule across DAB. OpenAPI now uses Required ?? !HasConfigDefault, GraphQL and describe_entities use Required ?? true, and MCP custom tools make a...

The inconsistency across GraphQL, MCP, and OpenAPI is a valid concern. Centralizing the predicate into a shared helper would require touching the GraphQL schema builder and MCP tool generation, which is beyond the scope of this PR. I'd recommend tracking that refactor as a follow-up item (e.g., referencing #3400), so it gets proper truth-table coverage for all three surfaces in one dedicated PR.

@copilot review the open/unresolved comments as fix what necessary and respond to each unresolved comments.

All three open comments have been addressed in the latest commit:

  • #3672485963: AddStoredProcedureInputParameters now uses Required ?? !HasConfigDefault for GET query params + ValidateGetQueryParameters test added.
  • #3672518926: ValidateRequestBodyContents now asserts requestBody.Required; sp4 entity added to cover the all-optional case (requestBody.Required == false, empty schema required set).
  • #5115225081: Replied that centralizing the predicate across GraphQL/MCP is out of scope and should be a follow-up PR.

@souvikghosh04

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).

@souvikghosh04

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Review In Progress

Development

Successfully merging this pull request may close these issues.

[Bug]: Required params not marked in OpenAPI

6 participants