Skip to content

Implement and align DbType.Time support across database dialects#163

Open
Copilot wants to merge 6 commits into
masterfrom
copilot/implement-dbtype-time
Open

Implement and align DbType.Time support across database dialects#163
Copilot wants to merge 6 commits into
masterfrom
copilot/implement-dbtype-time

Conversation

Copy link
Copy Markdown

Copilot AI commented May 20, 2026

  • Inspect the new PR comment and investigate the failing CI/build using GitHub Actions logs
  • Reproduce the reported build failure locally and identify the minimal fix
  • Apply a surgical change and run targeted validation for the affected build/tests
  • Run final automated review/security validation and reply to the new comment

Copilot AI requested review from Copilot and removed request for Copilot May 20, 2026 13:13
Copilot AI linked an issue May 20, 2026 that may be closed by this pull request
Agent-Logs-Url: https://github.com/dotnetprojects/Migrator.NET/sessions/439a3240-c184-402f-8fb3-17c4aab1fbeb

Co-authored-by: jogibear9988 <364896+jogibear9988@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 20, 2026 13:19
Copilot AI changed the title [WIP] Implement DbType.Time for job time column Implement SQL Server support for DbType.Time May 20, 2026
Copilot AI requested a review from jogibear9988 May 20, 2026 13:24
@jogibear9988
Copy link
Copy Markdown
Member

@copilot also create tests

Copy link
Copy Markdown
Author

Copilot AI commented May 20, 2026

@copilot also create tests

Added in dab1b02: ColumnPropertyMapperTest.SqlServerMapsTimeType for the dialect mapping and SQLServerTransformationProvider_AddTableTests.AddTableTime for the SQL Server provider path.

@jogibear9988 jogibear9988 marked this pull request as ready for review May 20, 2026 14:20
Copilot AI review requested due to automatic review settings May 20, 2026 14:20
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes SQL Server handling of DbType.Time so that migrations can create real TIME columns and schema introspection can resolve SQL Server time metadata back to DbType.Time, preventing type round-trip mismatches.

Changes:

  • Map DbType.Time to SQL Server TIME in SqlServerDialect (instead of DATETIME).
  • Extend SQL Server schema introspection (GetColumns) to recognize time and map it to MigratorDbType.Time.
  • Add regression tests covering dialect-level mapping and provider-level add/readback for DbType.Time.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/Migrator/Providers/Impl/SqlServer/SqlServerTransformationProvider.cs Adds schema metadata parsing for SQL Server timeMigratorDbType.Time.
src/Migrator/Providers/Impl/SqlServer/SqlServerDialect.cs Changes dialect type mapping for DbType.Time to emit TIME.
src/Migrator.Tests/Providers/SQLServer/SQLServerTransformationProvider_AddTableTests.cs Adds integration coverage to ensure DbType.Time round-trips via AddTable / GetColumnByName.
src/Migrator.Tests/ColumnPropertyMapperTest.cs Adds unit coverage ensuring dialect maps DbType.Time ↔ database type name time.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
else if (dataTypeString == "time")
{
column.MigratorDbType = MigratorDbType.Time;
Comment on lines 48 to 52
RegisterColumnType(DbType.String, 4000, "NVARCHAR($l)");
RegisterColumnType(DbType.String, int.MaxValue, "NVARCHAR(max)");
//RegisterColumnType(DbType.String, 1073741823, "NTEXT");
RegisterColumnType(DbType.Time, "DATETIME");
RegisterColumnType(DbType.Time, "TIME");
RegisterColumnType(DbType.VarNumeric, "NUMERIC(18,0)");
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.

Addressed in 4596e13. SqlServerDialect.Default now emits quoted TimeSpan defaults for SQL Server TIME columns, and I added a focused test covering the generated SQL.

Agent-Logs-Url: https://github.com/dotnetprojects/Migrator.NET/sessions/0ab9f682-2192-4257-b5e8-a8f5da25d152

Co-authored-by: jogibear9988 <364896+jogibear9988@users.noreply.github.com>
@jogibear9988
Copy link
Copy Markdown
Member

@copilot what about the other databases? We always should implement features for all

Agent-Logs-Url: https://github.com/dotnetprojects/Migrator.NET/sessions/b2c708ee-90fa-4e09-a63d-f7ca8baf53cb

Co-authored-by: jogibear9988 <364896+jogibear9988@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 20, 2026 16:13
Agent-Logs-Url: https://github.com/dotnetprojects/Migrator.NET/sessions/b2c708ee-90fa-4e09-a63d-f7ca8baf53cb

Co-authored-by: jogibear9988 <364896+jogibear9988@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 20, 2026 16:15
Copy link
Copy Markdown
Author

Copilot AI commented May 20, 2026

@copilot also create tests

Added focused tests in f7aec93/980835d: cross-database dialect tests for DbType.Time, plus provider add-table coverage for SQLite and PostgreSQL alongside the existing SQL Server coverage.

@copilot what about the other databases? We always should implement features for all

Handled in f7aec93. I extended the follow-up beyond SQL Server by adding DbType.Time coverage for PostgreSQL, SQLite, and MySQL where the repo already has native TIME mappings, including quoted TimeSpan defaults for SQLite/MySQL and a shared exact-match fix so PostgreSQL time resolves correctly. Oracle/Firebird still need a separate design because they do not currently round-trip a native time-only type the same way.

Copilot AI changed the title Implement SQL Server support for DbType.Time Implement and align DbType.Time support across database dialects May 20, 2026
@jogibear9988
Copy link
Copy Markdown
Member

@copilot the build fails, look after it

Agent-Logs-Url: https://github.com/dotnetprojects/Migrator.NET/sessions/47922b64-b978-4b97-96e7-50d505b94852

Co-authored-by: jogibear9988 <364896+jogibear9988@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 20, 2026 20:42
Copy link
Copy Markdown
Author

Copilot AI commented May 20, 2026

@copilot the build fails, look after it

Fixed in 1027e03. The failing CI test was PostgreSQL AddTableTime: PostgreSQL reports native TIME columns as time without time zone, but PostgreSQLTransformationProvider.GetColumns only handled time. I updated the introspection mapping and re-ran dotnet build Migrator.slnx plus dotnet test Migrator.slnx successfully.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 1.0.0.127.in-addr.arpa
    • Triggering command: /opt/mssql/bin/sqlservr (dns block)
  • metadata.google.internal
    • Triggering command: /opt/mssql/bin/sqlservr (dns block)
  • settings-win.data.microsoft.com
    • Triggering command: /opt/mssql/bin/sqlservr (dns block)
  • vortex.data.microsoft.com
    • Triggering command: /opt/mssql/bin/sqlservr (dns block)
  • www.example.com
    • Triggering command: /opt/oracle/product/26ai/dbhomeFree/bin/oracle oracleFREE (DESCRIPTION=(LOCAL=YES)(ADDRESS=(PROTOCOL=beq))) (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DbType.Time is not implemented

3 participants