Skip unsupported column data types during schema discovery - #3802
Skip unsupported column data types during schema discovery#3802Joymax (joymaxnascimento) wants to merge 5 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
@microsoft-github-policy-service agree |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 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
FillSchemaForTableAsyncfromSELECT *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 schemaDataTablecache. - Add an MSSQL test fixture/table/config entity to validate that a
geometrycolumn omitted fromfields.includeis 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 |
There was a problem hiding this comment.
🟡 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
There was a problem hiding this comment.
🟡 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:
GeometryTypehas neitherfields[].primary-keynorsource.key-fields, soconfiguredPrimaryKeyis empty, this returns unrestricted, andBuildSchemaProjectionAsyncproducesSELECT *.FillSchemawill therefore still encountergeomand 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
…ring field permissions
Thanks — all three points landed, and taken together they convinced me the approach was wrong The PR now skips columns whose data type the provider cannot map to a CLR type, instead of The reasoning: read permission and physical existence are different things. Several paths need a Your summary line was also right that the primary-key guard left the reported scenario on One note on scope: I did not reuse the type list from MsSqlQueryBuilder's autoentity discovery. |
There was a problem hiding this comment.
🟡 Changes recommended
The implemented provider-wide type filtering materially differs from the documented permission-based behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
Why make this change?
geometrycolumn cannot be exposed at all.FillSchemaForTableAsyncreadsthe object shape with
SELECT *, andDbDataAdapter.FillSchemaneeds a CLR type for everycolumn in the projection.
Microsoft.Data.SqlClientresolves a CLR UDT's type through theMicrosoft.SqlServer.Typesassembly, which DAB does not reference, so the reader reports notype and the adapter fails with
DataReader.GetFieldType(N) returned null. The entity neverloads, 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.
SqlMetadataProvidergains avirtual UnsupportedColumnDataTypes, empty by default. When it isempty the projection stays
SELECT *, so PostgreSQL, MySQL and Cosmos are untouched.MsSqlMetadataProvideroverrides it with the three SQL Server CLR user-defined types:geometry,geography,hierarchyid.GetColumnsAsynccall — the
Columnsschema collection, catalog metadata only, so the offending type is nevermaterialized — and the projection names the remaining columns. Identifiers come from the
catalog's own
COLUMN_NAME, so casing and quoting match the database rather than the config.back to
SELECT *and behavior is exactly what it is today.so the omission is discoverable rather than silent.
Deliberately limited to three types
MsSqlQueryBuilder's autoentity discovery skips objects containinggeography,geometry,hierarchyid,sql_variant,xml,rowversionorvector. That list is intentionally notreused here:
SqlTypeConstants.SupportedSqlDbTypesmarkstimestampandvectoras supported,and there are dedicated
vectortests, so excluding them would regress shipped behavior. Only theCLR UDTs actually make
GetFieldTypereturn null, and only those are skipped.Why not honor
fields.includeinsteadAn 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.Columnswithout needing it to be readable.A database policy on an excluded column stops parsing against the EDM model
(
EdmModelBuilderiteratesColumns) and returns 400 on every request in that role; entitiessharing a
source.objectshare oneSourceDefinition, so a per-entity filter cannot isolateanything; multiple-create indexes
Columnsby relationship column name; RESTPUTstops nullingthe 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,geographyorhierarchyidcolumn is left out even for a role thatgrants unrestricted read, and
fields.include/fields.excludeno longer affect discovery inany way.
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.Columnsstops reaching it, so REST,GraphQL, OpenAPI, MCP and database policies see exactly what they see today for every object that
works today. Providers other than MSSQL declare no unsupported types and keep
SELECT *.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?
ValidateUnsupportedColumnTypeIsNotInferredinsrc/Service.Tests/UnitTests/SqlMetadataProviderUnitTests.csinfers metadata against a live MSSQLinstance and asserts that the
geometrycolumn is absent from the inferredSourceDefinitionwhile the other columns are present. The entity places no field restriction, so the test covers
the column being skipped on the strength of its type alone. Before this change the same fixture
fails during
InitializeAsync.To be transparent: I have not run this test locally — it needs a provisioned SQL Server and I do
not have a disposable instance available. It compiles and reaches the fixture setup, but its
result has not been observed. Please let CI run it, and tell me if it needs adjusting.
The fixture follows the pattern already used for
vectorcolumns:src/Service.Tests/DatabaseSchema-MsSql.sql— newgeometry_type_table(drop, create, seed).config-generators/mssql-commands.txt—add GeometryType.src/Service.Tests/dab-config.MsSql.json— the regenerated entity.src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt— refreshed, since the config gained an entity.
MSSQL only. No MySQL, PostgreSQL, DW SQL or Cosmos fixture changes, so those suites are untouched.
Sample Request(s)
Table:
Entity — no special configuration needed:
Before this change the engine fails while reading metadata for
Geom. After it, the entity loadsand responds:
{ "value": [ { "Id": 1, "Name": "Shaft collar" } ] }with this in the log at startup: