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

document enum support #261

Closed
kbilsted opened this issue Jun 3, 2019 · 9 comments
Closed

document enum support #261

kbilsted opened this issue Jun 3, 2019 · 9 comments
Assignees
Labels
deployed Feature or bug is deployed at the current release enhancement New feature or request question Further information is requested

Comments

@kbilsted
Copy link

kbilsted commented Jun 3, 2019

Your project looks nice. Currently I'm using Dapper which Im sure you know.

One of the pain points of dapper is the lack of support for enums as documented here DapperLib/Dapper#813 DapperLib/Dapper#259
DapperLib/Dapper#458
DapperLib/Dapper#433

How is the support for enums in repodb? I've searched the documentation with little luck

@mikependon mikependon self-assigned this Jun 3, 2019
@mikependon mikependon added the question Further information is requested label Jun 3, 2019
@mikependon
Copy link
Owner

Hi @kbilsted , seems interesting. I can't even say the behavior right now, TBH, this is not tested by me. I will find look how I can support this soon. This should be not be a big change on the compilers, but I need to write more test on this one before publishing.

Will update this ticket about the progress moving forward.

@mikependon
Copy link
Owner

mikependon commented Jun 4, 2019

Hi @kbilsted , I am about to work on this feature. RepoDb supports type mapping like below.

Existing features:

Type level:

  TypeMapper.Map(typeof(String), DbType.String);
  TypeMapper.Map(typeof(DateTime), DbType.DateTime2);

Property Level:

 public class Person
 {
      public int Id { get; set; }
      public string Name { get; set; }
      [TypeMap(DbType.Int)]
      public int Gender { get; set; }
 }

So on this enum support, I can simply do it like this.

Type Level:

 TypeMapper.Map(typeof(TypeOfEnum), DbType.String);

 TypeMapper.Map(typeof(TypeOfEnum), DbType.Int);

Property Level:

 public enum Gender
 {
       Male,
       Female
 }

 public class Person
 {
      public int Id { get; set; }
      public string Name { get; set; }
      [TypeMap(DbType.String)]
      public Gender Gender { get; set; }
 }

Question:

With you as a possible user, how would you behave this enum if we are missing a mapping? Should we save it as String, or should we save it as Enum Underlying Type?

I do not want to get influenced by outside discussions, so you might have already talked this one with somebody else. This question is targeted, and I would like to hear that. Thank you!

@kbilsted
Copy link
Author

kbilsted commented Jun 6, 2019

Hi, thanks for asking. There are a number of dimensions you have to consider

regular enums
"Flags" enums
storing enum as a number
storing enum as a string
invalid number (reading a number that does not fit an enum value (or combination in case of flags))
invalid string (reading a string from db that does not fit an enum value (or combination in case of flags))

so fill out all the cells below and you have the answer ;)

enums "Flags" enums enum as a number enum as a string invalid number invalid string
enums
"Flags" enums
enum as a number
enum as a string
invalid number
invalid string

@mikependon
Copy link
Owner

This make sense and thanks. Anyway, I had covered the following on first check-in:

  1. regular enums - covered
  2. "Flags" enums - covered
  3. storing enum as a number - covered
  4. storing enum as a string - covered
  5. invalid number (reading a number that does not fit an enum value (or combination in case of flags)) - exception might be thrown as the Enum.ToObject() might fail.
  6. invalid string (reading a string from db that does not fit an enum value (or combination in case of flags)) -- exception will be thrown as the Enum.Parse() will fail.

I also included the following.

  1. Save the Enum as INT even the Property is STRING if you force the [TypeMap(DbType.Int)], or TypeMapper.Map(typeof(Gender), DbType.Int);
  2. Consider the mapping in the "LinqExpression" like this (connection.Query(p => p.Gender == Gender.Female);) -- this is currently failing, so I cannot issue a "beta" build.

For now, I have an initial check-in and here is the Integration Tests.

Link: https://github.com/mikependon/RepoDb/blob/master/RepoDb/RepoDb.Tests/RepoDb.IntegrationTests/EnumPropertyTest.cs

The #8 corresponds to Ln 100 to Ln 117 -- that is currently commented.

FYI: I have to write a few more tests for this to covered some more scenarios. But I will definitely issue a beta for you (to be able to test it).

@mikependon
Copy link
Owner

mikependon commented Jun 10, 2019

@kbilsted - a beta version has been introduced that includes this change. May I request for you to check it out and let me know about your feedback?

Documentation: https://repodb.readthedocs.io/en/latest/pages/enumeration.html
Test for NetFramework: https://github.com/mikependon/RepoDb/blob/master/RepoDb/RepoDb.Tests/RepoDb.IntegrationTests/EnumPropertyTest.cs
Test for NetCore: https://github.com/mikependon/RepoDb/blob/master/RepoDb.Core/RepoDb.Tests/RepoDb.IntegrationTests/EnumPropertyTest.cs
Available At: >= v1.9.8-beta1
Package Manager Command: Install-Package RepoDb -Version 1.9.8-beta1

@mikependon mikependon added the deployed Feature or bug is deployed at the current release label Jun 10, 2019
@kbilsted
Copy link
Author

Great stuff!

I don't see, however, the test cases cover half of the combinations of the matrix, e.g. where the top horizontal values are writing and the vertical values are reading. I'd focus more on getting the matrix right before rushing to code.

@mikependon
Copy link
Owner

mikependon commented Jun 10, 2019

Scenarios mentioned by your vertical and horizontal table is covered by the definitions below. But I need it to be tested well before I issue a new release build. The Integration Tests and Unit Tests are intact as per my expected behavior, but user behaviors are very important.

Database Table:

CREATE TABLE [dbo].[CompleteTable]
(
	Id BIGINT IDENTITY(1, 1) PRIMARY
	, ColumnBit BIT
	, ColumnNVarChar NVARCHAR(32)
	, ColumnInt INT
	, ColumnBigInt BIGINT
	, ColumnSmallInt SMALLINT
);

Class Properties:

namespace RepoDb.IntegrationTests.Models
{
	[Map("[dbo].[CompleteTable]")]
	public class EnumCompleteTable
	{
		public Guid SessionId { get; set; }
		public BooleanValue? ColumnBit { get; set; }
		public Direction ColumnNVarChar { get; set; }
		public Direction? ColumnInt { get; set; }
		public Direction? ColumnBigInt { get; set; }
		public Direction? ColumnSmallInt { get; set; }
	}

	[Map("[dbo].[CompleteTable]")]
	public class EnumAsIntForStringCompleteTable
	{
		public Guid SessionId { get; set; }
		[TypeMap(DbType.Int32)]
		public Direction ColumnNVarChar { get; set; }
	}

	[Map("[dbo].[CompleteTable]")]
	public class TypeLevelMappedForStringEnumCompleteTable
	{
		public Guid SessionId { get; set; }
		public Continent ColumnNVarChar { get; set; }
	}
}

And with this Enum-level mapping:

TypeMapper.Map(typeof(Continent), System.Data.DbType.Int16, true);

Link: https://github.com/mikependon/RepoDb/blob/master/RepoDb/RepoDb.Tests/RepoDb.IntegrationTests/Models/EnumClasses.cs

This class is being used on the Integration Tests provided above. The only behaviors to be considered are:

  1. Invalid Number - fetch will succeed; inserts, updates, merge will succeed. But the mapping will not be intact on the class level (let us say: value is 100 in class property, but value is 1 in the DB) - this is expected as Enum.ToObject() will still work -- let me know if we need to change this scenario
  2. Invalid String - fetch will fail; inserts, updates, merge will succeed. This is expected as Enum.Parse() will fail.

@mikependon mikependon added the enhancement New feature or request label Jun 12, 2019
@mikependon
Copy link
Owner

The feature is now implemented at this library at version 1.9.8.

@mikependon
Copy link
Owner

Closing this ticket now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed Feature or bug is deployed at the current release enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants