-
Couldn't load subscription status.
- Fork 283
[MCP] Added fields section for entities #2916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new fields section for entities as part of the Model Context Protocol (MCP) implementation. The change consolidates the legacy mappings and key-fields properties into a unified fields array that provides richer metadata (aliases, descriptions, primary key designation) for entity columns.
Key Changes
- Added
FieldMetadataclass to represent field-level configuration including name, alias, description, and primary key status - Updated
Entityconstructor and configuration to include optionalFieldsproperty - Modified CLI commands to support field configuration through new command-line options
- Converted legacy
mappingsandkey-fieldsusage to the newfieldsstructure in update operations
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Config/ObjectModel/FieldMetadata.cs | New class defining field metadata structure |
| src/Config/ObjectModel/Entity.cs | Added Fields property to Entity record |
| src/Cli/ConfigGenerator.cs | Implemented field validation, conversion from legacy properties, and MCP-related warnings |
| src/Cli/Commands/EntityOptions.cs | Added CLI options for field configuration |
| src/Cli/Commands/AddOptions.cs | Updated constructor to accept field-related parameters |
| src/Cli/Commands/UpdateOptions.cs | Updated constructor to accept field-related parameters |
| src/Service.GraphQLBuilder/Sql/SchemaConverter.cs | Updated to use Fields instead of Mappings for aliases and descriptions |
| src/Core/Services/OpenAPI/OpenApiDocumentor.cs | Enhanced to include field descriptions from Fields metadata |
| src/Core/Services/MetadataProviders/SqlMetadataProvider.cs | Updated GraphQL reserved name checking to use Fields; removed unused helper method |
| src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs | Added Fields: null to linking entity creation |
| schemas/dab.draft.schema.json | Added schema definition for fields array with mutual exclusivity constraints |
| Multiple test files | Updated test entity instantiations to include Fields: null parameter |
| Multiple snapshot files | Updated snapshots showing conversion of mappings/key-fields to Fields structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "not": { | ||
| "anyOf": [ | ||
| { "required": ["mappings"] }, | ||
| { "properties": { "source": { "properties": { "key-fields": { } }, "required": ["key-fields"] } } } |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSON schema constraint to prevent fields and key-fields coexistence is incorrect. The nested structure doesn't properly check for the presence of key-fields in the source object. It should be: { \"properties\": { \"source\": { \"required\": [\"key-fields\"] } } }.
| { "properties": { "source": { "properties": { "key-fields": { } }, "required": ["key-fields"] } } } | |
| { "properties": { "source": { "required": ["key-fields"] } } } |
|
One pending follow up task, is to check if the given field is a valid column in the entity. Will add this check as part of a follow up PR. |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
| ] | ||
| Type: View | ||
| }, | ||
| Fields: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ensure the new fields section is well-documented. Please create a doc task for @JerryNixon
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
| "name": { "type": "string", "description": "Database column name." }, | ||
| "alias": { "type": "string", "description": "Exposed name for the field." }, | ||
| "description": { "type": "string", "description": "Field description." }, | ||
| "primary-key": { "type": "boolean", "description": "Is this field a primary key?" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a question for someone? I think we should change this description
| Source: { | ||
| Object: s001.book, | ||
| Type: Table, | ||
| KeyFields: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is KeyFields a different name for mappings just to specify that it is a primary key? Please also mention this parameter in the PR description
| fieldsNameCollection: [], | ||
| fieldsAliasCollection: [], | ||
| fieldsDescriptionCollection: [], | ||
| fieldsPrimaryKeyCollection: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to make these parameters empty and not null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same question applies to the other instances where this change was made.
| string? config | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| string? config | |
| ) | |
| string? config) |
src/Cli/ConfigGenerator.cs
Outdated
| else | ||
| { | ||
| fields = []; | ||
| _logger.LogWarning("Using legacy 'mappings' and 'key-fields' properties. Consider using 'fields' for new entities."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this warning should be in the else if (hasMappings || hasKeyFields) section
| // Resolve PKs from fields first | ||
| List<string>? pkFields = entity.Fields? | ||
| .Where(f => f.PrimaryKey) | ||
| .Select(f => f.Name) | ||
| .ToList(); | ||
|
|
||
| // Fallback to key-fields from config | ||
| if (pkFields == null || pkFields.Count == 0) | ||
| { | ||
| pkFields = entity.Source.KeyFields?.ToList(); | ||
| } | ||
|
|
||
| // If still empty, fallback to DB schema PKs | ||
| if (pkFields == null || pkFields.Count == 0) | ||
| { | ||
| DataTable dataTable = await GetTableWithSchemaFromDataSetAsync( | ||
| entityName, | ||
| GetSchemaName(entityName), | ||
| GetDatabaseObjectName(entityName)); | ||
|
|
||
| pkFields = dataTable.PrimaryKey.Select(pk => pk.ColumnName).ToList(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this code is the same as the one above. Couldn't we make a helper function out of it in order to not have repeated code?
0045c97 to
89dd7f3
Compare
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Why make this change?
This change introduces richer metadata for entity fields and keys in the Data API Builder configuration. The goal is to enable semantic metadata for documentation, client generation, and MCP tools, and to begin deprecating the legacy mappings and source.key-fields properties.
What is this change?
fieldsproperty for entities in the config schema.name: database column name (required)alias: exposed name for the field (optional)description: field description (optional)primary-key: whether the field is a primary key (optional)fieldscannot be used with legacymappingsorsource.key-fields.dab add,dab update) to support specifying field alias, description, and primary-key.dab validateto warn if fields are missing and MCP is enabled.describe_entitiesresponses to include field descriptions.mappingsandsource.key-fieldsto the newfieldsformat when relevant CLI flags are used.fieldsand legacy props are not mixed.How was this tested?
Manually tested with following queries:
dotnet C:\DAB\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.dll update BookAuthor --map "BookID:book_id,AuthorID:author_id" --source.key-fields "BookID,AuthorID" --config "C:\DAB\data-api-builder\src\Service\dab-config.json"Validate the Configuration
dotnet C:\DAB\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.dll validate -c "C:\DAB\data-api-builder\src\Service\dab-config.json"Update Entity Field Metadata (Primary Key)
dotnet C:\DAB\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.dll update Todo --fields.name owner_id --fields.primary-key True --config C:\DAB\data-api-builder\src\Service\dab-config.jsonUpdate Entity Key Fields Only
dotnet C:\DAB\data-api-builder\src\out\cli\net8.0\Microsoft.DataApiBuilder.dll update BookAuthor --source.key-fields "BookID"Sample new format
"fields": [ { "name": "id", "alias": "id", "description": "The unique identifier for a todo item", "primary-key": true }, { "name": "title", "alias": "title", "description": "The title of the todo item", "primary-key": false }, { "name": "completed", "alias": "completed", "description": "Indicates whether the todo item is completed", "primary-key": false }, { "name": "owner_id", "alias": "owner", "description": "Hello", "primary-key": true }, { "name": "position", "alias": "position", "description": "The position of the todo item in the list", "primary-key": false } ],GraphQL Introspection Example
Query
{ __type(name: "Todo") { name description fields { name description } } }Result
{ "data": { "__type": { "name": "Todo", "description": "Represents a todo item in the system", "fields": [ { "name": "id", "description": "The unique identifier for a todo item" }, { "name": "title", "description": "The title of the todo item" }, { "name": "completed", "description": "Indicates whether the todo item is completed" }, { "name": "owner", "description": "Hello" }, { "name": "position", "description": "The position of the todo item in the list" } ] } } }OpenAPI Schema Example
"Todo": { "type": "object", "properties": { "id": { "type": "string", "description": "The unique identifier for a todo item", "format": "" }, "title": { "type": "string", "description": "The title of the todo item", "format": "" }, "completed": { "type": "boolean", "description": "Indicates whether the todo item is completed", "format": "" }, "owner_id": { "type": "string", "description": "Hello", "format": "" }, "position": { "type": "number", "description": "The position of the todo item in the list", "format": "" } }, "description": "Represents a todo item in the system" }