Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(data-reader): add support for converting DateOnly and TimeOnly types from string #249

Merged

Conversation

dmitrii-kiselev
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows Conventional Commits
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

If the value in the database is stored as a string and the target type is DateOnly, this results in an exception:

System.InvalidCastException: Unable to cast object of type 'System.String' to type 'System.DateOnly'.
   at EFCoreSecondLevelCacheInterceptor.EFTableRowsDataReader.GetFieldValue[T](Int32 ordinal) in EFTableRowsDataReader.cs:line 593

If the value in the database is stored as a string and the target type is TimeOnly, this results in an exception:

System.InvalidCastException: Unable to cast object of type 'System.String' to type 'System.TimeOnly'.
   at EFCoreSecondLevelCacheInterceptor.EFTableRowsDataReader.GetFieldValue[T](Int32 ordinal) in EFTableRowsDataReader.cs:line 593

This is especially relevant for SQLite.

What is the new behavior?

Conversion without exceptions.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

In the GetFieldValue method of EFTableRowsDataReader class, there is code to convert a string to DateTimeOffsetType:

return (T)(object)DateTimeOffset.Parse((string)value, CultureInfo.InvariantCulture);

However, there is no corresponding code for the DateOnly and TimeOnly types.
This issue occurs when integrating with SQLite.

In the original SqliteValueReader, there is the following code:
https://github.com/dotnet/efcore/blob/bb583a91d7269510ecb09a4466fee61bc62280d1/src/Microsoft.Data.Sqlite.Core/SqliteValueReader.cs#L71

#if NET6_0_OR_GREATER
        public virtual DateOnly GetDateOnly(int ordinal)
        {
            var sqliteType = GetSqliteType(ordinal);
            switch (sqliteType)
            {
                case SQLITE_FLOAT:
                case SQLITE_INTEGER:
                    return DateOnly.FromDateTime(FromJulianDate(GetDouble(ordinal)));

                default:
                    return DateOnly.Parse(GetString(ordinal), CultureInfo.InvariantCulture);
            }
        }

        public virtual TimeOnly GetTimeOnly(int ordinal)
            => TimeOnly.Parse(GetString(ordinal), CultureInfo.InvariantCulture);
#endif

These changes expand DateOnly and TimeOnly support.

Best Regards,
Dmitrii Kiselev

…y` types from string

This commit adds missing support for converting `DateOnly` and `TimeOnly` types from string.
Copy link

what-the-diff bot commented Sep 27, 2024

PR Summary

  • Improved Data Conversion Capabilities:

    • Added features that enable conversion from text format into Date format, and similarly from text into Time format in certain methods used in database operations. This allows us to work with dates and times more easily in the system.
  • Amended and Enhanced Testing for Date Feature:

    • Improved the naming of certain test operations for better clarity of their function related to the Date conversion feature.
    • Included additional tests to ensure that no unexpected issues occur when making conversions from text to Date format.
  • Extended Testing for Time Feature:

    • Introduced tests to validate the functionality of converting text into Time format correctly.
    • Incorporated additional tests to prevent unexpected problems that can occur during the conversion from text to Time format.
  • Refined Test Assertions:

    • Updated the check mechanisms in test operations to better accommodate the newly introduced conversion features. Rather than just checking for errors, these mechanisms now also confirm that the conversions happened as intended.

@@ -579,11 +579,21 @@
return (T)(object)DateOnly.FromDateTime((DateTime)value);
}

if (expectedValueType == TypeExtensions.DateOnlyType && actualValueType == TypeExtensions.StringType)
{
return (T)(object)DateOnly.Parse((string)value, CultureInfo.InvariantCulture);

Check warning

Code scanning / CodeQL

Useless upcast Warning

There is no need to upcast from
DateOnly
to
Object
- the conversion can be done implicitly.
if (expectedValueType == TypeExtensions.TimeOnlyType && actualValueType == TypeExtensions.TimeSpanType)
{
return (T)(object)TimeOnly.FromTimeSpan((TimeSpan)value);
}

if (expectedValueType == TypeExtensions.TimeOnlyType && actualValueType == TypeExtensions.StringType)
{
return (T)(object)TimeOnly.Parse((string)value, CultureInfo.InvariantCulture);

Check warning

Code scanning / CodeQL

Useless upcast Warning

There is no need to upcast from
TimeOnly
to
Object
- the conversion can be done implicitly.
@VahidN VahidN merged commit a0698ea into VahidN:master Sep 27, 2024
3 checks passed
@dmitrii-kiselev dmitrii-kiselev deleted the feature/date-time-only-from-string branch September 27, 2024 16:10
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.

2 participants