Skip to content

Skip unsupported column data types during schema discovery - #3802

Open
Joymax (joymaxnascimento) wants to merge 12 commits into
Azure:mainfrom
joymaxnascimento:fix/schema-discovery-honors-field-permissions
Open

Joymax (joymaxnascimento) wants to merge 12 commits into
Azure:mainfrom
joymaxnascimento:fix/schema-discovery-honors-field-permissions

Conversation

@joymaxnascimento

@joymaxnascimento Joymax (joymaxnascimento) commented Sep 4, 2026

Copy link
Copy Markdown

Why make this change?

A table holding a geometry column cannot be exposed at all. FillSchemaForTableAsync reads the object shape with SELECT *, and DbDataAdapter.FillSchema needs a CLR type for every column in the projection. For a SQL Server CLR user-defined type the reader reports no CLR type, so SchemaMapping fails with DataReader.GetFieldType(N) returned null and takes the whole object down. DAB does not reference Microsoft.SqlServer.Types. The entity never loads, and there is no configuration-side workaround — excluding the column through permissions does not help, because discovery runs before authorization is consulted.

What is this change?

Schema discovery leaves out columns whose data type the provider cannot map to a CLR type, so the rest of the object stays reachable.

SqlMetadataProvider gains a virtual UnsupportedColumnDataTypes, empty by default. When it is empty the projection stays SELECT *, so PostgreSQL, MySQL and Cosmos are untouched. MsSqlMetadataProvider overrides it with the three SQL Server CLR user-defined types: geometry, geography, hierarchyid.

When a provider declares such types, the projection is built from the catalog and names the remaining columns. Identifiers come from the catalog's own COLUMN_NAME, so casing and quoting match the database rather than the config. The catalog is read once per object and shared with column definition population, instead of being queried twice.

The projection reproduces the set of columns SELECT * exposes before subtracting the unsupported types. This matters because the catalog also reports columns declared GENERATED ALWAYS AS ROW START/END HIDDEN, which SELECT * does not return: naming them would widen the exposed contract rather than preserve it, and since the read-only classification does not recognize generated-always period columns, an overwriting PUT would target them. They are excluded through sys.columns.is_hidden, and they are not reported as skipped — they were never part of the object's shape as the engine sees it.

When — and only when — the projection is narrowed, the shape is read with CommandBehavior.SchemaOnly rather than through DbDataAdapter.FillSchema. FillSchema runs with KeyInfo, under which the provider performs its own key discovery and returns key columns absent from the SELECT list as hidden reader columns; an unsupported type among them reintroduces DataReader.GetFieldType(N) returned null even though the projection excluded it. That is reachable through the object's own primary key and through any unique index, including when a supported key is configured through source.key-fields. Without KeyInfo the projection is authoritative, and the primary key comes from the catalog instead. When nothing is skipped, FillSchema still runs with KeyInfo exactly as before, so ordinary primary-key inference is untouched for every object that works today.

The primary key comes from the object's own key when it has one. Absent a primary key, the first unique index whose every key column is non-nullable and readable is used, in index order — which is what the data adapter reports as DataTable.PrimaryKey on the unnarrowed path, so the two paths agree and the narrowed one does not start demanding source.key-fields for an object the other resolves on its own.

Identity columns are carried from sys.columns.is_identity rather than through DataColumn.AutoIncrement. That setter coerces a DataType it cannot increment to Int32, and SQL Server allows identity on tinyint, numeric and decimal, so using it as the transport would report the wrong SystemType and reach parameter typing and the generated API schemas.

A primary key that cannot be read is rejected at initialization, with the column and its data type named. This covers a key configured over such a column, and the object's own database key when no key is configured — single column or one member of a composite key. Omitting it would leave the key in SourceDefinition.PrimaryKey while absent from SourceDefinition.Columns, and that inconsistency surfaces much later as a lookup failure. The generic "primary key not configured" error is not used for these, because it reads as something the user forgot to configure even though no configuration can express that key.

Configuration naming a column left out of the projection also fails initialization. The exposed and backing column maps are built from entity fields and mappings without requiring the column to exist in SourceDefinition.Columns, and the authorization resolver accepts explicitly included field names, so such a reference keeps resolving after the column is gone and then fails per request rather than at startup. mappings, fields, a permission's fields.include and a database policy are all rejected; exclude is left alone, since it asks for what already happened. The policy case matters for the same reason the field-permission approach was withdrawn: a policy is parsed per request against the OData model, which EdmModelBuilder builds from SourceDefinition.Columns, so one naming an absent column returns 400 on every request for that role.

The catalog facts that the Columns schema collection does not carry — which columns the database hides from SELECT *, and which columns form the object's primary key — come from a new provider hook, GetObjectCatalogMetadataAsync, which returns null in the base class. Only MsSqlMetadataProvider implements it, reading sys.columns joined to sys.indexes / sys.index_columns, cached once per object for the duration of initialization. Providers that declare no unsupported types never call it, and neither does an object that holds none: the classification runs first over the Columns schema collection, which is read for every object anyway, so only an object whose projection must be narrowed pays the additional query.

If nothing unsupported is present, or the catalog cannot be read, or it does not report a usable type name, the projection falls back to SELECT * and behavior is exactly what it is today. That last case includes an engine without sys.columns.is_hidden, which exists from SQL Server 2016 on, and a login without VIEW DEFINITION. When every column of an object is unsupported it fails with a clear initialization error instead, since falling back there would re-issue the projection that cannot be read.

Skipped columns are logged once per object at Warning level, naming each column and its type, so the omission is discoverable rather than silent.

Deliberately limited to three types

MsSqlQueryBuilder's autoentity discovery skips objects containing geography, geometry, hierarchyid, sql_variant, xml, rowversion or vector. That list is intentionally not reused here: SqlTypeConstants.SupportedSqlDbTypes marks timestamp and vector as supported, and there are dedicated vector tests, so excluding them would regress shipped behavior. This change is scoped to the CLR user-defined types, which are the ones whose CLR type the provider does not report.

Why not honor fields.include instead

An earlier revision of this PR narrowed the projection to the columns the permissions allow to be read. I withdrew it: read permission and physical existence are different things, and several paths need a column to be in SourceDefinition.Columns without needing it to be readable. A database policy on an excluded column stops parsing against the EDM model (EdmModelBuilder iterates Columns) and returns 400 on every request in that role; entities sharing a source.object share one SourceDefinition, so a per-entity filter cannot isolate anything; multiple-create indexes Columns by relationship column name; REST PUT stops nulling the hidden columns. Skipping by type has none of that blast radius, because a column the provider cannot type was never usable in any of those paths.

Compatibility impact

Skipping is provider-wide and driven by the column's data type alone, so it does not consult permissions: a geometry, geography or hierarchyid column is left out even for a role that grants unrestricted read, and fields.include / fields.exclude no longer narrow discovery. They are not ignored either — naming one of these columns in fields, mappings or a permission's fields.include now fails initialization, because the reference cannot be honored once the column is not part of the exposed contract.

That is a deliberate widening, and it takes nothing away in practice. Those three types cannot be read today at all — the object fails discovery and the entity never loads, which is #3801. The observable change is that such an entity now loads with the column absent, instead of not loading. No column that previously reached SourceDefinition.Columns stops reaching it, so REST, GraphQL, OpenAPI, MCP and database policies see exactly what they see today for every object that works today.

Objects that fail today now fail with a specific reason instead of the provider's opaque error: one whose configured or database primary key is of an unsupported type, and one whose every column is. Providers other than MSSQL and DWSQL declare no unsupported types and keep SELECT *. DWSQL shares MsSqlMetadataProvider and therefore inherits the override, which is inert there since Synapse has no spatial types.

The one thing worth reviewing is discoverability: an omitted column is now a Warning in the log rather than a startup failure. I chose the Warning deliberately, but if you would rather the engine keep failing loudly and require an explicit opt-in per entity, that is a small change and I am happy to make it.

How was this tested?

  • Integration Tests
  • Unit Tests

Eleven tests in src/Service.Tests/UnitTests/SqlMetadataProviderUnitTests.cs infer metadata against a live MSSQL instance:

Test What it asserts
ValidateUnsupportedColumnTypeIsNotInferred a geometry column is absent from the inferred SourceDefinition while the supported columns are present, on the strength of its type alone
ValidateObjectWithOnlyUnsupportedColumnsFailsInitialization an object whose every column is unsupported fails with a specific error instead of the provider's opaque one
ValidatePrimaryKeyOnUnsupportedColumnFailsInitialization a configured key naming an unsupported column fails, naming the column and its type
ValidateHiddenPeriodColumnsAreNotInferred the HIDDEN period columns of a temporal table stay out of the exposed contract, as does the spatial column
ValidateUnsupportedDatabasePrimaryKeyFailsInitialization an object whose own database key is of an unsupported type is rejected before adapter execution, with the reason
ValidateUnsupportedColumnInCompositeDatabasePrimaryKeyFailsInitialization the same when the unsupported column is one member of a composite key
ValidateConfiguredKeyIsUsedWhenUniqueIndexColumnIsUnsupported an object whose unique index covers an unsupported column loads when a supported key is configured through source.key-fields
ValidateUniqueKeyIsInferredWhenNoDatabasePrimaryKeyExists an object with no database primary key has its non-nullable unique key inferred, with no source.key-fields configured
ValidateConfiguredReferenceToUnsupportedColumnFailsInitialization a mappings entry over an unsupported column fails initialization
ValidateDatabasePolicyOverUnsupportedColumnFailsInitialization a database policy referencing an unsupported column fails initialization
ValidateIdentityTypeIsPreservedOnTheNarrowedPath a decimal identity column keeps its reported type and is still marked auto-generated

Run against SQL Server 2025 (RTM-CU8-GDR) 17.0.4085.5, Developer Edition, in a local Linux container, with DatabaseSchema-MsSql.sql applied in full:

total: 11; failed: 0; passed: 11

MSSQL only. DWSQL, MySQL and PostgreSQL need engines I do not have locally, so CI remains the check for those suites — no fixture of theirs is touched by this change.

The fixtures follow the pattern already used for vector columns, in src/Service.Tests/DatabaseSchema-MsSql.sql:

  • geometry_type_table and geometry_only_view — a spatial column, and an object whose every column is spatial.
  • temporal_geometry_type_table — system-versioned, with HIDDEN period columns and a spatial column.
  • hierarchyid_pk_table and hierarchyid_composite_pk_table — an unsupported type as the database primary key, alone and as one member of a composite key.
  • hierarchyid_unique_table — no database primary key, an unsupported column under a unique index, and a supported column to configure as the key.
  • unique_key_geometry_table — no database primary key, a non-nullable unique index over a supported column, and a spatial column.
  • decimal_identity_geometry_table — a decimal(18, 0) identity column, whose CLR type DataColumn.AutoIncrement would coerce to Int32, and a spatial column.

config-generators/mssql-commands.txt and src/Service.Tests/dab-config.MsSql.json carry the GeometryType entity, and src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt has the matching block. I added only that block rather than accepting the full received file: on my machine the committed dab-config.MsSql.json and the committed snapshot already disagree on action ordering for Book and WebsiteUser, and I did not want to bake that local difference into the snapshot. Happy to regenerate both properly if you prefer.

The entities for the objects added in the latest round are declared in memory inside the tests rather than in dab-config.MsSql.json, since several of them fail by design and every MSSQL fixture initializes every configured entity. No further snapshot regeneration is involved.

Sample Request(s)

Table:

create table dbo.Geom
(
    Id       int identity(1,1) not null primary key,
    Name     nvarchar(100)     not null,
    Location geometry          null
);

Entity — no special configuration needed:

"Geom": {
  "source": { "object": "dbo.Geom", "type": "table" },
  "permissions": [
    { "role": "anonymous", "actions": [ { "action": "read" } ] }
  ]
}

Before this change the engine fails while reading metadata for Geom. After it, the entity loads and responds:

GET /api/Geom
{ "value": [ { "Id": 1, "Name": "Shaft collar" } ] }

with this in the log at startup:

warn: Skipping column(s) of dbo.Geom whose data type is not supported: Location (geometry).
      They are not exposed through REST, GraphQL or MCP.
## Why make this change?

A table holding a geometry column cannot be exposed at all. FillSchemaForTableAsync reads the
object shape with SELECT *, and DbDataAdapter.FillSchema needs a CLR type for every column in
the projection. For a SQL Server CLR user-defined type the reader reports no CLR type, so
SchemaMapping fails with DataReader.GetFieldType(N) returned null and takes the whole object
down. DAB does not reference Microsoft.SqlServer.Types. The entity never loads, and there is no
configuration-side workaround — excluding the column through permissions does not help, because
discovery runs before authorization is consulted.

What is this change?

Schema discovery leaves out columns whose data type the provider cannot map to a CLR type, so the
rest of the object stays reachable.

SqlMetadataProvider gains a virtual UnsupportedColumnDataTypes, empty by default. When it is
empty the projection stays SELECT *, so PostgreSQL, MySQL and Cosmos are untouched.
MsSqlMetadataProvider overrides it with the three SQL Server CLR user-defined types:
geometry, geography, hierarchyid.

When a provider declares such types, the projection is built from the catalog and names the
remaining columns. Identifiers come from the catalog's own COLUMN_NAME, so casing and quoting
match the database rather than the config. The catalog is read once per object and shared with
column definition population, instead of being queried twice.

The projection reproduces the set of columns SELECT * exposes before subtracting the
unsupported types. This matters because the catalog also reports columns declared
GENERATED ALWAYS AS ROW START/END HIDDEN, which SELECT * does not return: naming them would
widen the exposed contract rather than preserve it, and since the read-only classification does
not recognize generated-always period columns, an overwriting PUT would target them. They are
excluded through sys.columns.is_hidden, and they are not reported as skipped — they were never
part of the object's shape as the engine sees it.

When — and only when — the projection is narrowed, the shape is read with
CommandBehavior.SchemaOnly rather than through DbDataAdapter.FillSchema. FillSchema runs
with KeyInfo, under which the provider performs its own key discovery and returns key columns
absent from the SELECT list as hidden reader columns; an unsupported type among them
reintroduces DataReader.GetFieldType(N) returned null even though the projection excluded it.
That is reachable through the object's own primary key and through any unique index, including
when a supported key is configured through source.key-fields. Without KeyInfo the projection
is authoritative, and the primary key comes from the catalog instead. When nothing is skipped,
FillSchema still runs with KeyInfo exactly as before, so ordinary primary-key inference is
untouched for every object that works today.

The primary key comes from the object's own key when it has one. Absent a primary key, the first
unique index whose every key column is non-nullable and readable is used, in index order — which
is what the data adapter reports as DataTable.PrimaryKey on the unnarrowed path, so the two
paths agree and the narrowed one does not start demanding source.key-fields for an object the
other resolves on its own.

Identity columns are carried from sys.columns.is_identity rather than through
DataColumn.AutoIncrement. That setter coerces a DataType it cannot increment to Int32, and SQL
Server allows identity on tinyint, numeric and decimal, so using it as the transport would
report the wrong SystemType and reach parameter typing and the generated API schemas.

A primary key that cannot be read is rejected at initialization, with the column and its data
type named. This covers a key configured over such a column, and the object's own database key
when no key is configured — single column or one member of a composite key. Omitting it would
leave the key in SourceDefinition.PrimaryKey while absent from SourceDefinition.Columns, and
that inconsistency surfaces much later as a lookup failure. The generic "primary key not
configured" error is not used for these, because it reads as something the user forgot to
configure even though no configuration can express that key.

Configuration naming a column left out of the projection also fails initialization. The exposed
and backing column maps are built from entity fields and mappings without requiring the column to
exist in SourceDefinition.Columns, and the authorization resolver accepts explicitly included
field names, so such a reference keeps resolving after the column is gone and then fails per
request rather than at startup. mappings, fields, a permission's fields.include and a
database policy are all rejected; exclude is left alone, since it asks for what already
happened. The policy case matters for the same reason the field-permission approach was withdrawn:
a policy is parsed per request against the OData model, which EdmModelBuilder builds from
SourceDefinition.Columns, so one naming an absent column returns 400 on every request for that
role.

The catalog facts that the Columns schema collection does not carry — which columns the
database hides from SELECT *, and which columns form the object's primary key — come from a new
provider hook, GetObjectCatalogMetadataAsync, which returns null in the base class. Only
MsSqlMetadataProvider implements it, reading sys.columns joined to sys.indexes /
sys.index_columns, cached once per object for the duration of initialization. Providers that
declare no unsupported types never call it, and neither does an object that holds none: the
classification runs first over the Columns schema collection, which is read for every object
anyway, so only an object whose projection must be narrowed pays the additional query.

If nothing unsupported is present, or the catalog cannot be read, or it does not report a usable
type name, the projection falls back to SELECT * and behavior is exactly what it is today. That
last case includes an engine without sys.columns.is_hidden, which exists from SQL Server 2016
on, and a login without VIEW DEFINITION. When every column of an object is unsupported it fails
with a clear initialization error instead, since falling back there would re-issue the projection
that cannot be read.

Skipped columns are logged once per object at Warning level, naming each column and its type, so
the omission is discoverable rather than silent.

Deliberately limited to three types

MsSqlQueryBuilder's autoentity discovery skips objects containing geography, geometry,
hierarchyid, sql_variant, xml, rowversion or vector. That list is intentionally not
reused here: SqlTypeConstants.SupportedSqlDbTypes marks timestamp and vector as supported,
and there are dedicated vector tests, so excluding them would regress shipped behavior. This
change is scoped to the CLR user-defined types, which are the ones whose CLR type the provider
does not report.

Why not honor fields.include instead

An earlier revision of this PR narrowed the projection to the columns the permissions allow to be
read. I withdrew it: read permission and physical existence are different things, and several
paths need a column to be in SourceDefinition.Columns without needing it to be readable. A
database policy on an excluded column stops parsing against the EDM model (EdmModelBuilder
iterates Columns) and returns 400 on every request in that role; entities sharing a
source.object share one SourceDefinition, so a per-entity filter cannot isolate anything;
multiple-create indexes Columns by relationship column name; REST PUT stops nulling the hidden
columns. Skipping by type has none of that blast radius, because a column the provider cannot
type was never usable in any of those paths.

Compatibility impact

Skipping is provider-wide and driven by the column's data type alone, so it does not consult
permissions: a geometry, geography or hierarchyid column is left out even for a role that
grants unrestricted read, and fields.include / fields.exclude no longer narrow discovery.
They are not ignored either — naming one of these columns in fields, mappings or a
permission's fields.include now fails initialization, because the reference cannot be honored
once the column is not part of the exposed contract.

That is a deliberate widening, and it takes nothing away in practice. Those three types cannot be
read today at all — the object fails discovery and the entity never loads, which is #3801. The
observable change is that such an entity now loads with the column absent, instead of not
loading. No column that previously reached SourceDefinition.Columns stops reaching it, so REST,
GraphQL, OpenAPI, MCP and database policies see exactly what they see today for every object that
works today.

Objects that fail today now fail with a specific reason instead of the provider's opaque error:
one whose configured or database primary key is of an unsupported type, and one whose every
column is. Providers other than MSSQL and DWSQL declare no unsupported types and keep
SELECT *. DWSQL shares MsSqlMetadataProvider and therefore inherits the override, which is
inert there since Synapse has no spatial types.

The one thing worth reviewing is discoverability: an omitted column is now a Warning in the log
rather than a startup failure. I chose the Warning deliberately, but if you would rather the
engine keep failing loudly and require an explicit opt-in per entity, that is a small change and
I am happy to make it.

How was this tested?

  • Integration Tests
  • Unit Tests

Eleven tests in src/Service.Tests/UnitTests/SqlMetadataProviderUnitTests.cs infer metadata
against a live MSSQL instance:

Test What it asserts
ValidateUnsupportedColumnTypeIsNotInferred a geometry column is absent from the inferred SourceDefinition while the supported columns are present, on the strength of its type alone
ValidateObjectWithOnlyUnsupportedColumnsFailsInitialization an object whose every column is unsupported fails with a specific error instead of the provider's opaque one
ValidatePrimaryKeyOnUnsupportedColumnFailsInitialization a configured key naming an unsupported column fails, naming the column and its type
ValidateHiddenPeriodColumnsAreNotInferred the HIDDEN period columns of a temporal table stay out of the exposed contract, as does the spatial column
ValidateUnsupportedDatabasePrimaryKeyFailsInitialization an object whose own database key is of an unsupported type is rejected before adapter execution, with the reason
ValidateUnsupportedColumnInCompositeDatabasePrimaryKeyFailsInitialization the same when the unsupported column is one member of a composite key
ValidateConfiguredKeyIsUsedWhenUniqueIndexColumnIsUnsupported an object whose unique index covers an unsupported column loads when a supported key is configured through source.key-fields
ValidateUniqueKeyIsInferredWhenNoDatabasePrimaryKeyExists an object with no database primary key has its non-nullable unique key inferred, with no source.key-fields configured
ValidateConfiguredReferenceToUnsupportedColumnFailsInitialization a mappings entry over an unsupported column fails initialization
ValidateDatabasePolicyOverUnsupportedColumnFailsInitialization a database policy referencing an unsupported column fails initialization
ValidateIdentityTypeIsPreservedOnTheNarrowedPath a decimal identity column keeps its reported type and is still marked auto-generated

Run against SQL Server 2025 (RTM-CU8-GDR) 17.0.4085.5, Developer Edition, in a local Linux
container, with DatabaseSchema-MsSql.sql applied in full:

total: 11; failed: 0; passed: 11

MSSQL only. DWSQL, MySQL and PostgreSQL need engines I do not have locally, so CI remains the
check for those suites — no fixture of theirs is touched by this change.

The fixtures follow the pattern already used for vector columns, in
src/Service.Tests/DatabaseSchema-MsSql.sql:

  • geometry_type_table and geometry_only_view — a spatial column, and an object whose every
    column is spatial.
  • temporal_geometry_type_table — system-versioned, with HIDDEN period columns and a spatial
    column.
  • hierarchyid_pk_table and hierarchyid_composite_pk_table — an unsupported type as the
    database primary key, alone and as one member of a composite key.
  • hierarchyid_unique_table — no database primary key, an unsupported column under a unique
    index, and a supported column to configure as the key.
  • unique_key_geometry_table — no database primary key, a non-nullable unique index over a
    supported column, and a spatial column.
  • decimal_identity_geometry_table — a decimal(18, 0) identity column, whose CLR type
    DataColumn.AutoIncrement would coerce to Int32, and a spatial column.

config-generators/mssql-commands.txt and src/Service.Tests/dab-config.MsSql.json carry the
GeometryType entity, and
src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt
has the matching block. I added only that block rather than accepting the full received file: on
my machine the committed dab-config.MsSql.json and the committed snapshot already disagree on
action ordering for Book and WebsiteUser, and I did not want to bake that local difference
into the snapshot. Happy to regenerate both properly if you prefer.

The entities for the objects added in the latest round are declared in memory inside the tests
rather than in dab-config.MsSql.json, since several of them fail by design and every MSSQL
fixture initializes every configured entity. No further snapshot regeneration is involved.

Sample Request(s)

Table:

create table dbo.Geom
(
    Id       int identity(1,1) not null primary key,
    Name     nvarchar(100)     not null,
    Location geometry          null
);

Entity — no special configuration needed:

"Geom": {
  "source": { "object": "dbo.Geom", "type": "table" },
  "permissions": [
    { "role": "anonymous", "actions": [ { "action": "read" } ] }
  ]
}

Before this change the engine fails while reading metadata for Geom. After it, the entity loads
and responds:

GET /api/Geom
{ "value": [ { "Id": 1, "Name": "Shaft collar" } ] }

with this in the log at startup:

warn: Skipping column(s) of dbo.Geom whose data type is not supported: Location (geometry).
      They are not exposed through REST, GraphQL or MCP.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@joymaxnascimento

Copy link
Copy Markdown
Author

Joymax (Joymax (@joymaxnascimento)) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

Contribution License Agreement

This Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”), and conveys certain license rights to Microsoft Corporation and its affiliates (“Microsoft”) for Your contributions to Microsoft open source projects. This Agreement is effective as of the latest signature date below.

  1. Definitions.
    “Code” means the computer software code, whether in human-readable or machine-executable form,
    that is delivered by You to Microsoft under this Agreement.
    “Project” means any of the projects owned or managed by Microsoft and offered under a license
    approved by the Open Source Initiative (www.opensource.org).
    “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any
    Project, including but not limited to communication on electronic mailing lists, source code control
    systems, and issue tracking systems that are managed by, or on behalf of, the Project for the purpose of
    discussing and improving that Project, but excluding communication that is conspicuously marked or
    otherwise designated in writing by You as “Not a Submission.”
    “Submission” means the Code and any other copyrightable material Submitted by You, including any
    associated comments and documentation.
  2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any
    Project. This Agreement covers any and all Submissions that You, now or in the future (except as
    described in Section 4 below), Submit to any Project.
  3. Originality of Work. You represent that each of Your Submissions is entirely Your original work.
    Should You wish to Submit materials that are not Your original work, You may Submit them separately
    to the Project if You (a) retain all copyright and license information that was in the materials as You
    received them, (b) in the description accompanying Your Submission, include the phrase “Submission
    containing materials of a third party:” followed by the names of the third party and any licenses or other
    restrictions of which You are aware, and (c) follow any other instructions in the Project’s written
    guidelines concerning Submissions.
  4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else
    for whom You are acting in making Your Submission, e.g. as a contractor, vendor, or agent. If Your
    Submission is made in the course of Your work for an employer or Your employer has intellectual
    property rights in Your Submission by contract or applicable law, You must secure permission from Your
    employer to make the Submission before signing this Agreement. In that case, the term “You” in this
    Agreement will refer to You and the employer collectively. If You change employers in the future and
    desire to Submit additional Submissions for the new employer, then You agree to sign a new Agreement
    and secure permission from the new employer before Submitting those Submissions.
  5. Licenses.
  • Copyright License. You grant Microsoft, and those who receive the Submission directly or
    indirectly from Microsoft, a perpetual, worldwide, non-exclusive, royalty-free, irrevocable license in the
    Submission to reproduce, prepare derivative works of, publicly display, publicly perform, and distribute
    the Submission and such derivative works, and to sublicense any or all of the foregoing rights to third
    parties.
  • Patent License. You grant Microsoft, and those who receive the Submission directly or
    indirectly from Microsoft, a perpetual, worldwide, non-exclusive, royalty-free, irrevocable license under
    Your patent claims that are necessarily infringed by the Submission or the combination of the
    Submission with the Project to which it was Submitted to make, have made, use, offer to sell, sell and
    import or otherwise dispose of the Submission alone or with the Project.
  • Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement.
    No additional licenses or rights whatsoever (including, without limitation, any implied licenses) are
    granted by implication, exhaustion, estoppel or otherwise.
  1. Representations and Warranties. You represent that You are legally entitled to grant the above
    licenses. You represent that each of Your Submissions is entirely Your original work (except as You may
    have disclosed under Section 3). You represent that You have secured permission from Your employer to
    make the Submission in cases where Your Submission is made in the course of Your work for Your
    employer or Your employer has intellectual property rights in Your Submission by contract or applicable
    law. If You are signing this Agreement on behalf of Your employer, You represent and warrant that You
    have the necessary authority to bind the listed employer to the obligations contained in this Agreement.
    You are not expected to provide support for Your Submission, unless You choose to do so. UNLESS
    REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING, AND EXCEPT FOR THE WARRANTIES
    EXPRESSLY STATED IN SECTIONS 3, 4, AND 6, THE SUBMISSION PROVIDED UNDER THIS AGREEMENT IS
    PROVIDED WITHOUT WARRANTY OF ANY KIND, INCLUDING, BUT NOT LIMITED TO, ANY WARRANTY OF
    NONINFRINGEMENT, MERCHANTABILITY, OR FITNESS FOR A PARTICULAR PURPOSE.
  2. Notice to Microsoft. You agree to notify Microsoft in writing of any facts or circumstances of which
    You later become aware that would make Your representations in this Agreement inaccurate in any
    respect.
  3. Information about Submissions. You agree that contributions to Projects and information about
    contributions may be maintained indefinitely and disclosed publicly, including Your name and other
    information that You submit with Your Submission.
  4. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and
    the parties consent to exclusive jurisdiction and venue in the federal courts sitting in King County,
    Washington, unless no federal subject matter jurisdiction exists, in which case the parties consent to
    exclusive jurisdiction and venue in the Superior Court of King County, Washington. The parties waive all
    defenses of lack of personal jurisdiction and forum non-conveniens.
  5. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and
    supersedes any and all prior agreements, understandings or communications, written or oral, between
    the parties relating to the subject matter hereof. This Agreement may be assigned by Microsoft.

@microsoft-github-policy-service agree

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI 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.

🟡 Changes recommended

Case-sensitive permission/primary-key filtering can incorrectly drop permitted columns or primary keys when config/schema casing differs, breaking metadata inference.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates SQL metadata discovery to honor entity field-level permissions when building the schema-read projection, avoiding provider failures on unsupported CLR-mapped columns (e.g., SQL Server geometry) that are not readable per config.

Changes:

  • Narrow FillSchemaForTableAsync from SELECT * to a permission-derived projection where possible, with fallbacks to preserve current behavior.
  • Re-apply per-entity column restrictions when populating SourceDefinition.Columns, accounting for the shared schema DataTable cache.
  • Add an MSSQL test fixture/table/config entity to validate that a geometry column omitted from fields.include is not inferred.
File summaries
File Description
src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Builds a permission-aware column projection for schema discovery and filters inferred columns accordingly.
src/Service.Tests/UnitTests/SqlMetadataProviderUnitTests.cs Adds an MSSQL test asserting excluded geometry column is not inferred.
src/Service.Tests/DatabaseSchema-MsSql.sql Adds geometry_type_table and seed data for the new test case.
src/Service.Tests/dab-config.MsSql.json Adds GeometryType entity with fields.include restricting readable columns.
src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt Updates snapshot to reflect the added MSSQL entity.
config-generators/mssql-commands.txt Adds generator command to create the GeometryType entity with fields.include.
Review details

Suppressed comments (1)

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:1904

  • Permitted column resolution uses case-sensitive sets (StringComparer.Ordinal) for Included/Excluded, but SourceDefinition.Columns is case-insensitive (StringComparer.InvariantCultureIgnoreCase). If config uses different casing than the schema (common with SQL Server), IsColumnPermitted() can incorrectly treat a readable column as non-permitted and drop it from SourceDefinition.Columns, breaking runtime behavior.
            HashSet<string> included = new(StringComparer.Ordinal);
            HashSet<string>? excluded = null;
            bool allColumns = false;
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Outdated
@joymaxnascimento

Copy link
Copy Markdown
Author

🟡 Changes recommended

Case-sensitive permission/primary-key filtering can incorrectly drop permitted columns or primary keys when config/schema casing differs, breaking metadata inference.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview
This PR updates SQL metadata discovery to honor entity field-level permissions when building the schema-read projection, avoiding provider failures on unsupported CLR-mapped columns (e.g., SQL Server geometry) that are not readable per config.

Changes:

  • Narrow FillSchemaForTableAsync from SELECT * to a permission-derived projection where possible, with fallbacks to preserve current behavior.
  • Re-apply per-entity column restrictions when populating SourceDefinition.Columns, accounting for the shared schema DataTable cache.
  • Add an MSSQL test fixture/table/config entity to validate that a geometry column omitted from fields.include is not inferred.

File summaries
File Description
src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Builds a permission-aware column projection for schema discovery and filters inferred columns accordingly.
src/Service.Tests/UnitTests/SqlMetadataProviderUnitTests.cs Adds an MSSQL test asserting excluded geometry column is not inferred.
src/Service.Tests/DatabaseSchema-MsSql.sql Adds geometry_type_table and seed data for the new test case.
src/Service.Tests/dab-config.MsSql.json Adds GeometryType entity with fields.include restricting readable columns.
src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt Updates snapshot to reflect the added MSSQL entity.
config-generators/mssql-commands.txt Adds generator command to create the GeometryType entity with fields.include.
Review details

Suppressed comments (1)

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:1904

  • Permitted column resolution uses case-sensitive sets (StringComparer.Ordinal) for Included/Excluded, but SourceDefinition.Columns is case-insensitive (StringComparer.InvariantCultureIgnoreCase). If config uses different casing than the schema (common with SQL Server), IsColumnPermitted() can incorrectly treat a readable column as non-permitted and drop it from SourceDefinition.Columns, breaking runtime behavior.
            HashSet<string> included = new(StringComparer.Ordinal);
            HashSet<string>? excluded = null;
            bool allColumns = false;
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Good catch. Fixed: Included/Excluded and the exposed-to-backing map now use
StringComparer.OrdinalIgnoreCase, matching the comparer on SourceDefinition.Columns and the
field name lookups in TryGetExposedColumnName/TryGetBackingColumn. The primary key guard
compares case-insensitively too, since SourceDefinition.PrimaryKey is a plain List.

Copilot AI 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.

🟡 Changes recommended

The new permission-based schema projection can omit primary key columns when PKs are inferred (not configured), causing initialization failures for valid configs that restrict fields.include without including PKs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Outdated

Copilot AI 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.

🟡 Changes recommended

The unconfigured-primary-key guard leaves the reported geometry scenario on SELECT *, while shared metadata and case-sensitive identifiers introduce additional correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:2026

  • This guard disables the new behavior for the exact regression fixture: GeometryType has neither fields[].primary-key nor source.key-fields, so configuredPrimaryKey is empty, this returns unrestricted, and BuildSchemaProjectionAsync produces SELECT *. FillSchema will therefore still encounter geom and fail before the new test reaches its assertions. The primary key needs to be obtained without materializing every column (for example from catalog metadata) and unioned into the restricted projection; requiring it in configuration would not fix the reported configuration.
            if (entity.Permissions is null || entity.Permissions.Length == 0 || configuredPrimaryKey.Count == 0)
            {
                return new(AllColumns: true, included, new HashSet<string>(StringComparer.OrdinalIgnoreCase));
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Outdated
Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Outdated

Copilot AI 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.

🟡 Changes recommended

Nested database connections can deadlock under a one-connection pool, and cache keys collide for supported dotted identifiers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:1847

  • BuildSchemaProjectionAsync opens another connection through GetColumnsAsync, but the outer connection is already open here. With the valid SQL Server setting Max Pool Size=1, the inner OpenAsync waits for this connection until timeout, so schema discovery fails; larger pools also consume two simultaneous connections per object. Build the projection before opening the adapter connection, or reuse the same connection for the catalog query.
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Outdated
Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs
@joymaxnascimento

Copy link
Copy Markdown
Author

🟡 Changes recommended

Nested database connections can deadlock under a one-connection pool, and cache keys collide for supported dotted identifiers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:1847

  • BuildSchemaProjectionAsync opens another connection through GetColumnsAsync, but the outer connection is already open here. With the valid SQL Server setting Max Pool Size=1, the inner OpenAsync waits for this connection until timeout, so schema discovery fails; larger pools also consume two simultaneous connections per object. Build the projection before opening the adapter connection, or reuse the same connection for the catalog query.

  • Files reviewed: 7/7 changed files

  • Comments generated: 2

  • Review effort level: Balanced

Correct, and thank you for catching it — this one was mine to see. The projection is now resolved
before the adapter connection is opened, so the catalog query no longer runs inside an open
connection. I left a comment at the call site explaining the ordering, since the obvious
refactor is to move it back down.

Copilot AI 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.

🔵 Needs a closer look

Skipped configured fields remain resolvable, and case-sensitive object names can collide in the metadata cache.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:104

  • This case-insensitive cache key aliases distinct SQL Server objects in a case-sensitive database. For example, dbo.Foo and dbo.foo can both be exposed, but after metadata for the first is cached the second reuses those catalog rows, so its projection and column definitions can silently come from the wrong table. Use an ordinal case-sensitive comparer for this per-object catalog cache; case-insensitive databases remain unaffected.
    src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:1946
  • Configured fields and legacy mappings are still copied into the exposed/backing maps even when their backing column was skipped here, and both TryGetBackingColumn/TryGetExposedColumnName also fall back directly to Entity.Fields. As a result, an entity that explicitly lists geom still reports it as valid to callers such as MCP aggregate validation, which checks only TryGetBackingColumn, and can issue a runtime query against the supposedly omitted UDT. Build these maps only from columns present in SourceDefinition.Columns and prevent the fallback from resolving skipped configured fields; add a regression case with geom in fields or mappings.
    src/Core/Services/MetadataProviders/SqlMetadataProvider.cs:1972
  • The configured-primary-key rejection has no regression coverage: the new tests exercise a mixed table with an inferred supported key and an all-unsupported view, so neither reaches this branch. Add an in-memory MSSQL entity over geometry_type_table with key-fields: ["geom"] and assert the initialization status, substatus, column name, and type; otherwise this consistency guard can regress unnoticed.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@joymaxnascimento

Copy link
Copy Markdown
Author

🔵 Needs a closer look

Skipped configured fields remain resolvable, and case-sensitive object names can collide in the metadata cache.

Review details

Two of the three are fixed in the latest commit.

The cache comparer is now ordinal, with the reason documented on both fields — you are right that
object identity and field name are different concerns, and I had applied the field-name rule to
both. Inner column names stay case-insensitive, matching how this class resolves configured names
against the schema.

The primary-key guard now has coverage: ValidatePrimaryKeyOnUnsupportedColumnFailsInitialization
declares an in-memory entity over geometry_type_table with key-fields: ["geom"] and asserts the
status, sub-status, column name and data type.

On the third, I agree with the finding but would rather not fix it here. Building the
exposed/backing maps only from SourceDefinition.Columns, and stopping TryGetBackingColumn /
TryGetExposedColumnName from falling back to Entity.Fields, touches name resolution for every
entity kind — tables, views, stored procedures and linking entities — and needs its own tests.
This PR has already changed approach once and grown twice; I would rather not widen it again.

The exposure is bounded: it requires explicitly naming the unsupported column in fields or
mappings, and before this change that configuration did not work at all, because the entity never
loaded. Happy to do it here if you prefer, or to open a follow-up PR — your call.

@aaronburtle aaronburtle self-assigned this Sep 14, 2026
@aaronburtle aaronburtle moved this from Todo to Review In Progress in Data API builder Sep 14, 2026
@aaronburtle aaronburtle added this to the Backlog milestone Sep 14, 2026
}
else
{
readableColumns.Add(columnName);

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.

The catalog lookup happens indirectly through existing code:

BuildSchemaProjectionAsync()GetCachedColumnsAsync()GetColumnsAsync()conn.GetSchemaAsync("Columns", columnRestrictions).

GetColumnsAsync() already exists in the base SqlMetadataProvider class, so the GetSchemaAsync() call is not a newly added line in this diff. For MSSQL, conn is a SqlConnection.

Previously, this catalog supplemented metadata obtained through FillSchema; it did not determine the SELECT projection. This PR now uses the catalog's column names to construct that projection.

The distinction matters because the catalog includes SQL Server columns declared HIDDEN, whereas SELECT * omits them. Adding every non-spatial catalog column to readableColumns therefore exposes hidden temporal period columns.

Reproduced configuration:

  • A temporal table with Id, Name, nullable Location geometry, and ValidFrom/ValidTo declared GENERATED ALWAYS AS ROW START/END HIDDEN.
  • Permissions include * and exclude Location.
  • Both revisions use Type System Version=SQL Server 2005 in the connection string. This allows the merge-base to discover the spatial column through the driver's legacy type representation, establishing a previously working configuration. It is a test precondition, not a proposed general workaround.

Observed difference:

  • Merge-base: REST/GraphQL expose only Id and Name; PUT with only Name returns 200.
  • PR head: both period columns become exposed; the same PUT returns 500: Cannot update GENERATED ALWAYS columns.

DAB's PUT uses overwrite semantics. BaseSqlQueryStructure.AddNullifiedUnspecifiedFields() assigns null to writable columns absent from the request. The newly discovered period columns reach that path because the existing read-only classification does not recognize them as generated-always temporal columns.

Please preserve the columns that SELECT * would expose, then subtract the unsupported types. This requires accounting for database hidden-column metadata, not applying permission filters. Add regression tests for both the exposed schema and supported-field PUT.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Confirmed and fixed. The projection now reproduces what SELECT * exposes before subtracting the unsupported types — hidden columns are excluded through sys.columns.is_hidden, and they are not reported as skipped, since they were never part of the object's shape as the engine sees it. Covered by ValidateHiddenPeriodColumnsAreNotInferred, over a new temporal_geometry_type_table fixture with HIDDEN period columns and a spatial column. Measured run is in the summary comment.

subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
}

_skippedColumnsByObject[GetObjectCacheKey(schemaName, tableName)] = skippedColumns;

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.

There are several separate representations of an entity's fields in DAB:

  • SourceDefinition.Columns: discovered database-column metadata used by query generation and validation.
  • Exposed/backing maps: translations between database column names and API aliases.
  • Authorization metadata: fields permitted for each role and operation.

This PR removes unsupported columns from the first representation, but the other representations can retain them.

The existing GenerateExposedToBackingColumnMapUtil() adds entries from Entity.Fields and legacy Entity.Mappings without requiring those columns to exist in SourceDefinition.Columns. The existing AuthorizationResolver.SetEntityPermissionMap() also accepts explicitly included field names. PopulateAllowedExposedColumns() then resolves those names through the maps.

Consequently, an explicitly permitted alias can remain usable even though its column metadata was removed. Those helpers are not new in this diff, but allowing startup makes this inconsistent state reachable.

Reproduced configuration:

  • Database column Location geometry.
  • API alias Position, configured through either fields or mappings.
  • Permissions explicitly include the backing names Id, Name, and Location.
  • For the mutation reproduction, create/update permissions include those fields and request-body-strict is false.

Observed results:

  • Startup succeeds and logs that Location was skipped.
  • Default REST GET returns 500: FOR JSON cannot serialize CLR objects. The permission-derived default projection includes the stale alias and selects the spatial column.
  • POST/PATCH supplying Position return 500 with KeyNotFoundException.
  • MCP create/update encounter the same missing-column error.
  • With the fields alias format, MCP describe_entities advertises Position, although GraphQL omits it and explicit MCP reads reject it.

For the mutation failure, successful TryGetBackingColumn() resolution leads SqlMutationEngine.PopulateParamsFromRestRequest() to index SourceDefinition.Columns["Location"], which no longer exists. Non-strict mode allows the input past the earlier unexpected-field validation.

Aliases with wildcard permissions alone were rejected correctly; the confirmed failure requires the explicit permission reference above. This is not a demonstrated authorization bypass.

Please either reject incompatible configured references during initialization or consistently reconcile mappings, permission-derived projections, MCP metadata, and name-resolution fallbacks with the usable columns. Add tests for both alias formats, default reads, and non-strict mutations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, and I am taking the first of your two options: initialization now fails when mappings, fields or a permission's fields.include name a column left out of the projection. exclude is left alone, since it asks for what already happened. Covered by ValidateConfiguredReferenceToUnsupportedColumnFailsInitialization. I did not do the full reconciliation here — reasoning in the summary comment — and I am happy to take it as a follow-up, or in this PR if you prefer.

Connection = conn
Connection = conn,
CommandText
= $"SELECT {projection} FROM {tableNameWithSchemaPrefix}"

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.

The filtered SELECT list does not fully control the columns examined by FillSchema.

DbDataAdapter.FillSchema() internally requests schema and key information using CommandBehavior.KeyInfo. That behavior comes from the framework implementation; there does not need to be an explicit KeyInfo reference in this PR.

SQL Server/SqlClient can consequently append primary or unique-key columns that were omitted from the SELECT list as hidden reader columns. These provider-added hidden columns are distinct from SQL table columns declared HIDDEN.

Reproduced cases:

  1. A table whose database primary key is Location hierarchyid, selecting only Name.
  2. A composite database primary key of (TenantId, Location hierarchyid), selecting only TenantId and Name.
  3. A table with no database primary key, a non-null Location hierarchyid UNIQUE, and a supported Id configured as DAB's key through source.key-fields: ["Id"], selecting only Id and Name.

In all three cases, the reader still contains Location with IsHidden = true and an unresolved CLR type. FillSchema() therefore throws the original DataReader.GetFieldType(N) returned null error.

The new RejectPrimaryKeyOnUnsupportedColumn() guard cannot handle this because it runs in PopulateSourceDefinitionAsync() only after GetTableWithSchemaFromDataSetAsync() has completed. The adapter throws inside that earlier call.

The third case is important: configuring a supported DAB key does not change the adapter's independent database key discovery, so it still retrieves the unsupported unique-key column and prevents startup.

Please account for provider-added key metadata when collecting schema. Diagnose unsupported actual keys before adapter execution where rejection is necessary, and avoid reintroducing excluded unique-key columns when a supported configured key is available. Preserve ordinary primary-key inference when changing this path.

Add tests using real hierarchyid primary/composite keys and the unique-key scenario above. The existing test that configures geometry as a key on a table whose actual database primary key is int does not exercise this framework behavior.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You were right, and this was the one that decided the approach: the narrowed projection was not fixing #3801 wherever the unsupported type participates in a key. When the projection is narrowed, discovery now reads the shape with CommandBehavior.SchemaOnly instead of FillSchema and takes the primary key from the catalog. When nothing is skipped, FillSchema still runs with KeyInfo unchanged, so ordinary primary-key inference is untouched. Your three cases are covered by ValidateUnsupportedDatabasePrimaryKeyFailsInitialization, ValidateUnsupportedColumnInCompositeDatabasePrimaryKeyFailsInitialization and ValidateConfiguredKeyIsUsedWhenUniqueIndexColumnIsUnsupported.

…d references

Schema discovery reproduces the columns SELECT * exposes before subtracting unsupported data types, so HIDDEN period columns stay out of the exposed contract. When the projection is narrowed it reads the shape with CommandBehavior.SchemaOnly instead of DbDataAdapter.FillSchema, whose KeyInfo behaviour reintroduced excluded key columns as hidden reader columns, and takes the primary key from the catalog. Ordinary primary-key inference is untouched when nothing is skipped. Configuration naming a column left out of the projection now fails initialization. Addresses the three review findings on Azure#3802.
@joymaxnascimento

Joymax (joymaxnascimento) commented Sep 15, 2026

Copy link
Copy Markdown
Author

All three findings are addressed and pushed. I replied in each thread with what changed and which
test covers it.

I also rewrote the description. It described only the projection narrowing, and its compatibility
section still claimed that fields.include / fields.exclude no longer affect discovery in any
way — which stopped being true once configured references to a skipped column began failing
initialization.

One correction to the record: the description previously said the automated test had not been run,
because I had no disposable SQL Server instance. I set one up. The eight tests now listed there are
observed results, against SQL Server 2025 (RTM-CU8-GDR) 17.0.4085.5, Developer Edition, in a local
Linux container with DatabaseSchema-MsSql.sql applied in full:

total: 11; failed: 0; passed: 11

MSSQL only — DWSQL, MySQL and PostgreSQL need engines I do not have locally, so CI remains the check
for those suites. No fixture of theirs is touched by this change.

The one design question still open is on your second finding. I rejected incompatible configured
references at initialization instead of reconciling the exposed and backing maps, the
permission-derived projections, MCP metadata and the name-resolution fallbacks with the usable
columns. Reconciling touches name resolution for tables, views, stored procedures and linking
entities alike and needs its own tests, and this PR has already changed approach once and grown
twice. Rejecting at startup closes the inconsistent state you demonstrated without that blast
radius, but it is your call — I am happy to do the full reconciliation here, or as a follow-up
against a green baseline.

Copilot AI 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.

🟡 Changes recommended

Identifier handling and reconstructed key/type metadata can still reject valid entities or expose incorrect schemas.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs Outdated
Comment thread src/Core/Services/MetadataProviders/MsSqlMetadataProvider.cs Outdated
Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs
Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs Outdated
…he narrowed path

object_id() now delimits both name parts through QUOTENAME, so a schema or table needing quoting resolves instead of returning no rows and silently leaving the projection at SELECT *.

Identity no longer travels through DataColumn.AutoIncrement, whose setter coerces a DataType it cannot increment to Int32 — SQL Server allows identity on tinyint, numeric and decimal. It comes from sys.columns.is_identity and is applied after the columns are populated, so the provider-reported DataType is preserved.

Absent a database primary key, the first non-nullable unique index whose key columns are all readable is inferred, in index order. The data adapter does this on the unnarrowed path, and dropping it made the narrowed path demand source.key-fields for objects the other path resolves on its own.

The catalog read is deferred until an object is known to hold an unsupported type, so objects that need no narrowing no longer pay an extra query per startup.

Adds unique_key_geometry_table and ValidateUniqueKeyIsInferredWhenNoDatabasePrimaryKeyExists. Addresses the Copilot review on Azure#3802.
@joymaxnascimento

Copy link
Copy Markdown
Author

🟡 Changes recommended

Identifier handling and reconstructed key/type metadata can still reject valid entities or expose incorrect schemas.

Get a fresh assessment by requesting another Copilot review.

Review details

All four are addressed. Two of them changed the approach, so noting what moved rather than only
that it moved.

object_id identifier delimiting. Fixed as suggested — both components go through
QUOTENAME inside the object_id argument, and the names are passed raw so the server does the
quoting. QuoteTableNameAsDBConnectionParam is a no-op on MSSQL, so the previous form was passing
undelimited identifiers. Your [a.b].[c] case now resolves, and the failure mode you describe —
no rows, projection falls back to SELECT *, entity still taken down by the unsupported column —
was exactly right.

DataColumn.AutoIncrement coercing the CLR type. You are right, and I had this wrong in both
directions: an earlier revision guarded the assignment by allowing byte and decimal, which are
precisely the types the setter coerces. DataColumn.AutoIncrement is no longer used as the
transport at all. The provider-reported DataType is carried through untouched, and identity comes
from sys.columns.is_identity, applied to ColumnDefinition.IsAutoGenerated / IsReadOnly after
the columns are populated. No-op for any object whose projection was not narrowed, where the
adapter still reports the flag itself.

Unique key inference when there is no primary key. Restored. The catalog read now returns the
eligible unique indexes alongside the primary key — disabled and filtered indexes excluded, and
key_ordinal > 0 already excludes an index's included columns. A candidate qualifies only when
none of its key columns is nullable, which is the condition under which the adapter promotes a
unique key to DataTable.PrimaryKey. At the point of use the object's own primary key wins; absent
one, the first candidate whose key columns are all present in the projection is taken, in index
order, so the choice is deterministic. When none qualifies, the missing-primary-key error is
reported as before.

Covered by ValidateUniqueKeyIsInferredWhenNoDatabasePrimaryKeyExists over a new
unique_key_geometry_table fixture: no database primary key, code varchar(20) not null unique,
and a geometry column. It asserts that the inferred key is code, with no source.key-fields
configured. ValidateConfiguredKeyIsUsedWhenUniqueIndexColumnIsUnsupported still passes, which is
the other half — a candidate over a column the projection dropped is correctly not promoted, and a
configured key takes precedence.

Catalog query on every object. Reordered as suggested. Classification now runs first over the
Columns schema collection, which is read for every object regardless, and returns SELECT *
before touching the new query when nothing is unsupported. Only an object whose projection must be
narrowed pays the extra round trip. The "every column unsupported" failure also moved ahead of that
read, since it does not need it either. Worth doing on its own terms given #3430.

Nine tests pass against SQL Server 2025 (RTM-CU8-GDR) 17.0.4085.5 Developer Edition in a local
Linux container:

total: 9; failed: 0; passed: 9

Copilot AI 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.

🟡 Changes recommended

Database policies can still reference omitted columns and consequently fail every request at runtime.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs
Comment thread src/Core/Services/MetadataProviders/SqlMetadataProvider.cs
…data

A database policy is parsed per request against the OData model, which EdmModelBuilder builds from SourceDefinition.Columns, so one naming a column left out of the projection returns 400 on every request for that role. Configured references in mappings, fields, fields.include and policy.database are now all rejected at initialization. Policy field references are scanned rather than parsed, because the model the parser needs does not exist yet at that point.

Adds identity assertions to ValidateUnsupportedColumnTypeIsNotInferred, plus decimal_identity_geometry_table and ValidateIdentityTypeIsPreservedOnTheNarrowedPath, which is what verifies that carrying identity from the catalog preserves a DataType that DataColumn.AutoIncrement would coerce to Int32.

Addresses the second Copilot review on Azure#3802.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Review In Progress

Development

Successfully merging this pull request may close these issues.

[Bug]: Table with a geometry column fails schema discovery ("GetFieldType returned null") even when fields.include excludes it

3 participants