-
Notifications
You must be signed in to change notification settings - Fork 50
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
Feature/custom table name mapping #12
Closed
benmccallum
wants to merge
4
commits into
landmarkhw:master
from
benmccallum:feature/custom-table-name-mapping
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
52bccb4
Custom table name mappings PR.
benmccallum fa608ab
Support for custom table names and tests.
benmccallum 6e859e4
Merge branch 'master' into feature/custom-table-name-mapping
benmccallum 38b4c75
Merge branch 'master' into feature/custom-table-name-mapping
dougrday File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using Dapper.GraphQL.Test.Models; | ||
using Xunit; | ||
|
||
namespace Dapper.GraphQL.Test | ||
{ | ||
public class DeleteTests : IClassFixture<TestFixture> | ||
{ | ||
private readonly TestFixture fixture; | ||
|
||
public DeleteTests(TestFixture fixture) | ||
{ | ||
this.fixture = fixture; | ||
} | ||
|
||
[Fact(DisplayName = "DELETE query uses custom table name")] | ||
public void DeleteWithCustomTableName() | ||
{ | ||
// Check generic Delete uses custom table name for Contact as configured in TestFixture | ||
var contact = new Contact(); | ||
|
||
var query = SqlBuilder.Delete<Contact>(contact); | ||
Assert.Equal("DELETE FROM Contacts WHERE ", query.ToString()); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Text; | ||
|
||
namespace Dapper.GraphQL.Test.Models | ||
{ | ||
public class Contact | ||
{ | ||
|
||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
|
||
namespace Dapper.GraphQL | ||
{ | ||
/// <summary> | ||
/// Helper for table related operations. | ||
/// </summary> | ||
public static class TableHelper | ||
{ | ||
private static Dictionary<Type, string> TableNameCache = new Dictionary<Type, string>(); | ||
|
||
/// <summary> | ||
/// Gets a table name for a given type, as configured with a default. | ||
/// </summary> | ||
/// <typeparam name="TEntity">The type to get a table name for.</typeparam> | ||
/// <returns>The configured table name or a generated default.</returns> | ||
public static string GetTableName<TEntity>() | ||
{ | ||
var type = typeof(TEntity); | ||
if (!TableNameCache.ContainsKey(type)) | ||
{ | ||
lock (TableNameCache) | ||
{ | ||
if (!TableNameCache.ContainsKey(type)) | ||
{ | ||
// Generate a default and cache it | ||
TableNameCache[type] = type.Name; | ||
} | ||
else | ||
{ | ||
return TableNameCache[type]; | ||
} | ||
} | ||
} | ||
|
||
return TableNameCache[type]; | ||
} | ||
|
||
public static void AddCustomTableNameMapping<TEntity>(string name) | ||
{ | ||
var type = typeof(TEntity); | ||
if (TableNameCache.TryGetValue(type, out string tableName) && tableName != name) | ||
{ | ||
throw new InvalidOperationException($"Dapper.GraphQL - Cannot add multiple custom table names for type '${type.Name}'"); | ||
} | ||
else | ||
{ | ||
lock (TableNameCache) | ||
{ | ||
TableNameCache[type] = name; | ||
} | ||
} | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should probably be a service that's dependency injected with the rest of the dependencies. Something like:
Then, this class would be dependency injected in the
AddDapperGraphQL()
extension method.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally up to you. I just followed the pattern you'd used for the PropertyHelper, but I agree it'd be nice to have the flexibility to let others supply their own implementation if need be.
Should we leave the caching up to their implementation or have a base class that can/must be used that does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change it to a service that's dependency injected. The
ParameterHelper
is different because it's more a caching interface between .NET and GraphQL -- it doesn't matter who uses it to benefit from the cache, whereas it may matter what database you're connecting to for theTableHelper
to perform properly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started playing around with this and the problem is all the static methods that create Insert/Query/Delete/Update contexts would need to accept an INameService because it can't be automatically resolved. Potentially this needs a bigger shift to use DI completely to new up these contexts. Have stopped for now as I'd like to hear your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can take a look over the weekend. Thanks Ben!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was just poking around Dapper.Contrib and I wonder if it's something we can depend on our take advantage of. They do some stuff around table name mapping.
https://github.com/StackExchange/Dapper/blob/f29981a77b27a8b4ae1fdb674a081da2fa2ef19f/Dapper.Contrib/SqlMapperExtensions.cs#L39
https://github.com/StackExchange/Dapper/blob/f29981a77b27a8b4ae1fdb674a081da2fa2ef19f/Dapper.Contrib/SqlMapperExtensions.cs#L283
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like they have a default implementation and then a delegate you can supply to override the default if needed. We could look at an approach like that instead of dependency injection potentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Why is it still not merged?
I also want to have an ability to specify table name because the default table name is not always suitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to change code to use Dapper.Contrib functionality?