Skip to content

Skip unsupported column data types during schema discovery - #3802

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

Joymax (joymaxnascimento) wants to merge 24 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?

  • Closes [Bug]: Table with a geometry column fails schema discovery ("GetFieldType returned null") even when fields.include excludes it #3801
    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.

Keys of a view

A view has no index of its own, so sys.indexes holds nothing for it and the narrowed path has
no catalog key to fall back to. An earlier revision took the key from is_part_of_unique_key in
sys.dm_exec_describe_first_result_set. That flag is wrong for this purpose: it is a per-column
union across every unique key of the underlying object and cannot be split back into one key. A
view exposing one column of a two-column key would have that single column inferred as the key,
and a partial key silently matches more than one row on an update or a delete.

Browse mode on the same procedure also reports source_schema, source_table and
source_column for each column. The narrowed path resolves the projection's source object from
those, requires a single source object — a projection over a join yields nothing and the missing
primary key is reported exactly as before — reads that object's primary key and non-nullable
unique candidates from the catalog it already caches, and accepts the first key the projection
exposes in full.

Every row browse mode returns counts toward the single-source check, the hidden ones included.
Browse mode reports the key columns of each participating object, so an object whose every
selected column was dropped from the projection survives in hidden rows alone: a view
select a.id, a.name, b.geom from A a join B b on ... narrows to [id], [name], and without
counting the hidden row for B's key the result would read as a single object and be keyed by
A's primary key — which does not identify a row, because the join can duplicate A's rows.
Only a column the projection selects can carry a key, so the hidden rows are excluded from the
key mapping itself.

A self-join gets past that check, because browse mode reports the physical table and not the
alias, so both instances look like one object. What gives it away is a source column reported
twice, since each instance contributes the key. Inference is declined when one repeats. The rule
is conservative: a projection that selects a single column twice under different names is
declined too, and simply needs source.key-fields, as it did before this change.

When no key qualifies, the reason is returned rather than thrown, and reported only if the object
ends up with no key at all. The schema read runs for every object while the answer matters only
for one that is keyless, so an object whose source.key-fields is configured must not be
rejected over a key it was told not to use — failing there would recommend precisely what was
configured. When the object is keyless, the error names the key column the projection leaves out,
which is more useful than the generic missing-primary-key one.

Identity, unreadable keys, stale references

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.

Policy field references are found by scanning the expression and skipping single-quoted literals,
with '' treated as an escaped quote, so a literal that happens to read like a column name is
not mistaken for one. Aliases resolve through fields before mappings, matching the precedence
GenerateExposedToBackingColumnMapUtil already applies.

A configured relationship's source.fields and target.fields are rejected on the same grounds,
and MultipleCreateOrderHelper is the path that would otherwise fail per request, since it
indexes SourceDefinition.Columns by each relationship field. That check runs after every object
definition is populated rather than alongside the others, because a relationship's
target.fields belong to the other entity's object, which may not have been read yet. Foreign
keys inferred from the catalog and linking.* fields are deliberately left alone: there is no
configuration to correct for an inferred key, so refusing to start would punish the database's
shape rather than the configuration, and the relationship simply not forming is the proportionate
outcome.

Catalog access

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. Both the
catalog read and the browse-mode describe are resolved before the object's connection is opened,
so nothing nests a second connection inside an open scope.

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
    Twenty-three tests in src/Service.Tests/UnitTests/SqlMetadataProviderUnitTests.cs infer metadata
    against a live MSSQL instance, and one REST test in
    src/Service.Tests/SqlTests/RestApiTests/Put/MsSqlPutApiTests.cs covers the PUT path:
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
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
ValidateIdentityTypeIsPreservedOnTheNarrowedPath a decimal identity column keeps its reported type and is still marked auto-generated
ValidateHiddenPeriodColumnsAreNotInferred the HIDDEN period columns of a temporal table stay out of the exposed contract, as does the spatial column
PutOneWithHiddenPeriodColumnsInTemporalTable an overwriting PUT against that table succeeds: the period columns are not in SourceDefinition.Columns, so AddNullifiedUnspecifiedFields never targets them
ValidateViewKeyIsInferredWithoutConfiguredKeyFields a view over a table with a primary key has that key inferred, with no source.key-fields configured
ValidateViewWithoutPrimaryKeyFallsBackToUniqueKey a view over a table whose only key is a unique index has that key inferred
ValidateViewOverPartiallyUnsupportedKeyDoesNotInferPartialKey a view exposing only part of the source object's composite key fails instead of inferring the exposed part as the key
ValidateViewOverJoinDoesNotInferKeyFromOneSide a view whose join's second object contributes only the unsupported column is not keyed by the first object's primary key, and still requires source.key-fields
ValidateViewOverSelfJoinDoesNotInferKey a view built on a self-join, which browse mode cannot distinguish from a single instance by source lineage, is not keyed by the object's own key
ValidateConfiguredKeyIsHonoredWhenViewOmitsUnderlyingKey a configured source.key-fields loads a view whose underlying key the projection cannot express, instead of being rejected over that key
ValidateRelationshipFieldOverUnsupportedColumnFailsInitialization a relationship's source.fields naming an unsupported column fails initialization instead of failing per request
ValidateConfiguredReferenceToUnsupportedColumnFailsInitialization a mappings entry over an unsupported column fails initialization
ValidateFieldsAliasOverUnsupportedColumnFailsInitialization a fields alias over an unsupported column fails initialization
ValidateIncludedFieldOverUnsupportedColumnFailsInitialization a permission's fields.include naming an unsupported column fails initialization
ValidateDatabasePolicyOverUnsupportedColumnFailsInitialization a database policy referencing an unsupported column fails initialization
ValidateQuotedLiteralInDatabasePolicyIsNotAFieldReference a quoted literal that reads like an unsupported column name is not treated as a field reference
ValidateEscapedQuoteInDatabasePolicyLiteralIsNotAFieldReference the same when the literal contains an escaped quote ('')
ValidateDatabasePolicyOverAliasOfSupportedColumnIsHonored a policy over an alias of a supported column still resolves, so the rejection does not overreach

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, using the two MSSQL filters from
.pipelines/mssql-pipelines.yml split into smaller blocks:

Filter Passed Failed
TestCategory=MsSql&FullyQualifiedName~SqlTests.Rest 349 0
TestCategory=MsSql&FullyQualifiedName~SqlTests.GraphQL 616 0
FullyQualifiedName~SqlMetadataProviderUnitTests&TestCategory=MSSQL 51 1
TestCategory=MsSql&(UnitTests|OpenApi|Telemetry|Authorization|Caching) 270 1
TestCategory=MsSql minus the above and minus hot reload 272 29
TestCategory=MsSql&FullyQualifiedName~ConfigurationHotReloadTests 9 1

All twenty-four tests above pass. The failures are environmental and reproduce on a clean checkout
of the merge base; none is in a code path this PR touches:

  • 29 of them, plus SqlQueryExecutorUnitTests.TestRetryPolicySuccessfullyExecutingQueryAfterNAttempts,
    fail on the container's self-signed certificate. Those tests run late-configured, and
    MsSqlQueryExecutor.ConfigureMsSqlQueryExecutor deliberately sets Encrypt = Mandatory with
    TrustServerCertificate = false for that mode, so no connection string can make them pass
    against a certificate the host does not trust. CI uses LocalDB and does not hit this.
  • SqlMetadataProviderUnitTests.CheckExceptionForBadConnectionStringForMsSql with an empty
    connection string fails when the class runs on its own and passes alongside the other MSSQL
    suites. SqlTestBase._queryBuilder is static and the helper only assigns it for MySQL and
    PostgreSQL, so for MSSQL it relies on a value another test left behind; with none, the key
    fallback added in c3515c8 reaches GetTableNameWithSchemaPrefix before any connection is
    opened and dereferences a null query builder. Reproduced on dfd3be4c, the merge base.
  • ConfigurationHotReloadTests.HotReloadConfigConnectionString times out waiting on the file
    watcher.
    The first three rows were re-run on the final commit. The last three are from the commit before
    it, which the following one changed only in the view key inference that the first three cover.

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.

Fixtures

In src/Service.Tests/DatabaseSchema-MsSql.sql, following the pattern already used for vector
columns:

  • geometry_type_table and geometry_only_view — a spatial column, and an object whose every
    column is spatial.
  • geometry_all_view — a view exposing the full key of a table with a primary key.
  • 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_composite_view — a view over that composite key exposing only its supported
    member, which is the partial-key case.
  • 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.
  • pk_and_unique_geometry_table and unique_without_pk_view — a table with both a primary key
    and a unique index, and a view exposing only the unique one.
  • join_geometry_view — a join whose second object contributes only the unsupported column, so
    after narrowing every selected column resolves to the first object and only browse mode's
    hidden row reveals the join.
  • self_join_geometry_view — a self-join, where browse mode reports the same physical table for
    both instances and only the key column appearing twice reveals it.
  • 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 and TemporalGeometryType entities, and
    src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt
    has the matching blocks.

I added only those blocks rather than accepting the full received file, and it is worth saying
why. The committed dab-config.MsSql.json has drifted from what the generator produces: the
committed file orders test_role_with_excluded_fields_on_mutation as read, create, update,
delete on Book and WebsiteUser, while the generator emits update, create, read, delete — which
is what the committed snapshot expects. It also differs in Connection Timeout and in a few
permission blocks that were collapsed onto one line by hand. Since CI regenerates the config
before testing, the snapshot matches there; against the committed file it does not. I confirmed
this by regenerating the config locally and running TestReadingRuntimeConfigForMsSql against
the generated file, where it passes with the blocks I added. Bringing the committed file back in
line with the generator would be a welcome cleanup but is not this PR's business, so I left those
28 lines alone. Happy to include them if you prefer.

The entities for the objects that fail by design are declared in memory inside the tests rather
than in dab-config.MsSql.json, since every MSSQL fixture initializes every configured entity.

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.

@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

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

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.

Copilot review overview

🟡 Changes recommended

Self-join key inference and relationships using skipped columns can produce unsafe keys or request-time failures.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Validate skipped relationship columns during initialization

src/​Core/​Services/​MetadataProviders/​SqlMetadataProvider.cs:2505

Skipped columns are checked in mappings, fields, policies, and permission includes, but relationship fields are omitted. A configured or catalog-inferred relationship can therefore retain an unsupported hierarchyid column that is absent from SourceDefinition.Columns; with multiple-create enabled, MultipleCreateOrderHelper indexes that dictionary by each relationship field (MultipleCreateOrderHelper.cs:240-241) and fails per request. Validate source.fields/target.fields (and inferred foreign-key columns) against skipped columns during initialization so these configurations fail clearly at startup.

Two more findings from review.

Browse mode reports the physical table rather than the alias, so the
single-source check cannot see a self-join: both instances of the object look
like one, and the object's key was inferred for a result whose rows the join can
duplicate. A source column reported twice is what gives it away, since each
instance contributes the key, so inference is declined when one repeats. The
rule is conservative: a projection selecting one column twice under different
names is declined too, and needs source.key-fields as it did before.

Configured relationships were the one place a reference to a skipped column was
still accepted. The exposed and backing column maps are built from the
configuration rather than from the source definition, so the reference kept
resolving and failed per request instead, in MultipleCreateOrderHelper, which
indexes SourceDefinition.Columns by each relationship field. The check runs
after every object definition is populated, because a relationship's
target.fields belong to the other entity's object. Foreign keys inferred from
the catalog are left alone: there is no configuration to correct there, so
refusing to start would punish the database's shape rather than the
configuration.

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.

Copilot review overview

🟡 Changes recommended

Unique-key discovery must exclude hypothetical indexes to avoid inferring unenforced keys.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

A hypothetical index exists only to hold column-level statistics and cannot be
used as a data access path, so it enforces no uniqueness. The catalog query
accepted one as a unique key candidate, which on a table with no primary key
would key the object on columns nothing keeps unique, letting an update or a
delete by key match more than one row. Excluded alongside disabled and filtered
indexes, for the same reason.

No fixture: the only way to create a hypothetical index is WITH STATISTICS_ONLY,
undocumented syntax the Database Engine Tuning Advisor uses, and tying the test
schema to it would break CI if it ever changes. Verified against SQL Server 2025
that such an index is reported with is_unique = 1 and is_hypothetical = 1, which
is what made it eligible.
Keep FillSchema for every object it can describe. When it rejects an
object because the provider reports no CLR type for a column (geometry,
geography, hierarchyid and other CLR types), re-read the schema with
SchemaOnly | KeyInfo and build the DataTable with the data adapter's
column and key rules, without those columns.

- Detect unreadable columns by their missing DataType rather than by
  type name, so Type System Version=SQL Server 2005 still exposes them
  as byte[] and custom CLR types are covered.
- Fail with a specific error when the inferred key, or every column,
  is unreadable.
- Validate configuration references to skipped columns once the
  exposed-to-backing maps exist: key-fields, mappings, fields,
  fields.include, database policies and relationship fields.
- Track skipped columns per schema DataTable, which entities over the
  same object share whatever the casing of its name.
- Remove the catalog-based projection and its queries.
Move the schema discovery tests into
SqlMetadataProviderUnsupportedColumnTypeTests, assert the exact errors,
expect the same keys as the data adapter infers, and cover relationship
fields named like an alias, entities that name an object in different
casing, and Type System Version=SQL Server 2005.

Add REST and GraphQL tests for the GeometryType entity and unit tests
for the policy field parser, and update the fixture comments.
@aaronburtle

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

@aaronburtle

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

… coverage

Autoentities skipped every table with a geometry, geography or hierarchyid
column. Schema discovery now leaves such columns out, so a table is only
skipped when one of them is in its primary key. The check covers any CLR type
and is defined once for both the filter and contains_invalid_types.

New tests cover an autoentity, and entities whose fields.include or
fields.exclude leave the column out, over REST and GraphQL; a linking object
with a skipped column; and linking fields that reference one.
@aaronburtle

Copy link
Copy Markdown
Contributor

Joymax (@joymaxnascimento), thank you for all the work on this. To help get #3801 over the line, I pushed a few commits on top of your branch. Your commits are unchanged, and the PR is still yours.

  • Merged main. This resolves the conflict with the cancellation-aware metadata initialization from Hot-Reload MCP tools #3747.

  • A simpler discovery path. FillSchema still describes every object it can. Only when it rejects an object because a column has no CLR type does discovery re-read the schema with CommandBehavior.SchemaOnly | KeyInfo and build the DataTable with the adapter's own rules, minus those columns. As a result:

    • keys are inferred exactly as today;
    • user-defined CLR types are covered;
    • Type System Version=SQL Server 2005 still exposes geometry/geography as byte[];
    • there are no extra catalog queries.

    This replaces the catalog-based projection and the join/self-join heuristics.

  • One validation pass rejects configuration that names a skipped column (keys, mappings, fields, permissions, policies, relationship and linking fields), with errors that name the entity.

  • Autoentities now generate tables with such columns, unless one is in the primary key.

  • Tests:

    • your metadata tests moved to SqlMetadataProviderUnsupportedColumnTypeTests, with more cases;
    • REST/GraphQL tests, including an autoentity and fields.include/fields.exclude around the column;
    • your temporal PUT test;
    • unit tests for the policy parser.

@aaronburtle

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

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

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

4 participants