[Bug]: Required params not marked in GraphQL

[JerryNixon]

Expected

Required params typed as String! (NON_NULL)

Actual

All params always nullable String

[anushakolan]

Issue #3501 — [Bug]: Required params not marked in GraphQL

Summary

Stored-procedure input arguments in the generated GraphQL schema are always emitted as nullable (String) even when the runtime config marks them required: true. Clients have no way to know which arguments are mandatory from introspection alone, and the GraphQL contract is inconsistent with what the MCP describe_entities tool reports for the same entity.

Why this is needed

This is the GraphQL counterpart to the work done for MCP in issue #3400 / PR #3425. It applies the same locked SP-parameter rules so that every DAB surface (MCP, GraphQL, OpenAPI, Custom tool) reports a consistent contract for the same stored-procedure entity.

Locked rules (from the team discussion that closed #3400)

These rules were locked across MCP, Custom tool, OpenAPI, and — by extension — GraphQL:

1. Names & types — SQL metadata, no override.
2. Required — defaults to true; config overrides per param. (Jerry's rule.)
3. Defaults — config only (T-SQL doesn't expose SP parameter defaults; has_default_value is always 0 for T-SQL procs).
4. Descriptions — config only (no SQL column carries per-parameter descriptions).
5. Params not in config are still discovered from metadata (name + type), default to required, no default value, no description.

The "default required = true" rule was chosen specifically because is_nullable in sys.parameters describes whether the parameter's type accepts NULL, not whether the caller must supply it. Inferring required-ness from is_nullable would produce schemas that lie to the agent for procs like CREATE PROC GetBook(@id INT) — is_nullable = 1 but EXEC GetBook still fails with "expects parameter '@id'". Defaulting to required is never wrong at execute time; if a proc truly has a default, that's a one-line config fix.

Jerry called out this exact GraphQL bug (#3501) in the same thread that locked the rules, alongside the OpenAPI and Custom-tool variants. The fix here is to apply rules 1, 2, and 5 at the GraphQL schema-generation layer.

Surface status

Surface	Issue / PR	Status
MCP describe_entities	#3400 → PR #3425	✅ Rules locked, code shipped
GraphQL schema (String! vs String)	#3501	⏳ Open — this issue
GraphQL param descriptions	(separate bug)	⏳ Open
GraphQL entity description	(separate bug)	⏳ Open
OpenAPI required / descriptions / entity description	(Souvik)	⏳ Open
Custom tool required array	(Souvik)	⏳ Open

Behavior change to call out

GraphQL today ignores config entirely for SP parameter metadata. After this fix, GraphQL will start honoring config (required flag and default value). This is intentional and matches the locked rules — it's the same behavior change MCP just adopted in PR #3425.

Expected

Required params typed as String! (NON_NULL).

Actual

All params always nullable String.

Root cause

In src/Service.GraphQLBuilder/GraphQLStoredProcedureBuilder.cs (GenerateStoredProcedureSchema), the input value's type: is always built as a bare NamedTypeNode, never wrapped in NonNullTypeNode. The required flag from entity.Source.Parameters[i].Required is never consulted:

inputValues.Add(
    new(
        location: null,
        name: new(param),
        description: definition.Description != null
                    ? new StringValueNode(definition.Description)
                    : new StringValueNode($"parameters for {name.Value} stored-procedure"),
        type: new NamedTypeNode(SchemaConverter.GetGraphQLTypeFromSystemType(type: definition.SystemType)),
        defaultValue: defaultValueNode,
        directives: new List<DirectiveNode>())
    );

The builder iterates spdef.Parameters (DB-discovered names) but doesn't overlay the config-declared required flag.

Proposed fix

Apply the same merge rules locked in #3400 / PR #3425:

1. Names come from DB metadata (spdef.Parameters keys) — already correct.
2. required comes from config when the parameter is listed; defaults to true when config is silent.
3. When effective required == true, wrap the type in NonNullTypeNode.
4. When effective required == false, leave the type nullable.

Pseudocode for the change inside GenerateStoredProcedureSchema:

foreach ((string param, ParameterDefinition definition) in spdef.Parameters)
{
    IValueNode? defaultValueNode = null;
    bool required = true; // Default per locked rules from #3400.

    if (entity.Source.Parameters is not null)
    {
        ParameterMetadata? paramMetadata = entity.Source.Parameters
            .FirstOrDefault(p => p.Name == param);

        if (paramMetadata is not null)
        {
            if (paramMetadata.Default is not null)
            {
                Tuple<string, IValueNode> defaultGraphQLValue = ConvertValueToGraphQLType(
                    paramMetadata.Default.ToString()!, parameterDefinition: spdef.Parameters[param]);
                defaultValueNode = defaultGraphQLValue.Item2;
            }

            // Honor explicit required flag from config (true or false).
            required = paramMetadata.Required ?? true;
        }
    }

    ITypeNode typeNode = new NamedTypeNode(
        SchemaConverter.GetGraphQLTypeFromSystemType(type: definition.SystemType));

    if (required)
    {
        typeNode = new NonNullTypeNode((INullableTypeNode)typeNode);
    }

    inputValues.Add(
        new(
            location: null,
            name: new(param),
            description: definition.Description != null
                        ? new StringValueNode(definition.Description)
                        : new StringValueNode($"parameters for {name.Value} stored-procedure"),
            type: typeNode,
            defaultValue: defaultValueNode,
            directives: new List<DirectiveNode>())
        );
}

Important subtleties

1. NON_NULL + default is valid GraphQL. A String! argument with a defaultValue is still callable without the client supplying a value. So required: true + default: "..." should still emit String! and keep the existing defaultValueNode logic.

2. MCP / GraphQL parity is the goal. Whatever describe_entities reports as required: true must show as NON_NULL in GraphQL, and vice versa. Without this, an agent that introspects via MCP and calls via GraphQL gets inconsistent contracts.

3. Defaults are config-only. Same as MCP — T-SQL doesn't expose SP parameter defaults reliably, so don't try to read defaults from DB metadata.

4. DB-only parameters (not in config) default to required. Consistent with MCP behavior — if a DB param isn't listed in config, default to required: true so GraphQL callers see they must supply it.

5. No change to result-set column nullability. This issue is strictly about input arguments on the stored-procedure mutation/query field, not the returned object type.

Test coverage to add

Add tests under src/Service.GraphQLBuilder.Tests/ (or the appropriate existing SP-schema test file):

• required: true in config → input value type is NonNullTypeNode.
• required: false in config → input value type is nullable NamedTypeNode.
• Parameter omitted from config but discovered in DB → defaults to NON_NULL.
• required: true + default in config → NON_NULL with defaultValue attached.
• Multiple entities sharing the same SP source → each entity's GraphQL field reflects its own per-entity merge result.
• End-to-end introspection: __schema query confirms String! vs String matches expectations.
• Parity test: same SP entity yields consistent required from describe_entities (MCP) and __schema (GraphQL).

Before / after example

Given config:

"InsertBook": {
  "source": {
    "object": "insert_book",
    "type": "stored-procedure",
    "parameters": [
      { "name": "title", "required": true },
      { "name": "publisher_id", "required": false }
    ]
  }
}

Today (#3501 bug):

executeInsertBook(
  title: String
  publisher_id: String
): [InsertBook!]!

Expected (after fix):

executeInsertBook(
  title: String!
  publisher_id: String
): [InsertBook!]!

If parameters is omitted entirely from config, both title and publisher_id should appear as String! (DB-discovered, defaulting to required).

Cross-links

• Companion: [Bug]: describe_entities missing stored procedure parameters #3400 (MCP rules) and PR Added missing stored procedure parameters in describe_entities response. #3425 (MCP implementation).
• File to modify: src/Service.GraphQLBuilder/GraphQLStoredProcedureBuilder.cs — GenerateStoredProcedureSchema.
• Mirror logic from MCP: src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs — BuildParameterMetadataInfo. Consider extracting a shared helper if the rules continue to evolve, so MCP and GraphQL never drift.

Acceptance criteria

• Stored-procedure input arguments are emitted as NonNullTypeNode when effective required == true.
• Stored-procedure input arguments are emitted as nullable NamedTypeNode when effective required == false.
• Default rule when config is silent: required == true (NON_NULL).
• required: true + default still emits NON_NULL with default value attached.
• Unit tests cover all four scenarios above.
• MCP describe_entities and GraphQL __schema agree on required for the same entity (parity test).