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.

Sign up for GitHub

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

Jump to bottom

Why a hard reference to the column "id" #140

Open
Luuk34 opened this issue Jul 14, 2022 · 5 comments · May be fixed by #158
Open

Why a hard reference to the column "id" #140

Luuk34 opened this issue Jul 14, 2022 · 5 comments · May be fixed by #158

Comments

@Luuk34
Copy link

Luuk34 commented Jul 14, 2022 •
edited
Loading

Why a hard reference to the column "id" on line 112 in below code?:

Dapper.Contrib/src/Dapper.Contrib/SqlMapperExtensions.cs

Line 112 in cf24f6b

var idProp = allProperties.Find(p => string.Equals(p.Name, "id", StringComparison.CurrentCultureIgnoreCase));

Even when this column is NOT in an index.....
@AartBluestoke
Copy link

agreed, i've been bitten by this

@PTwr
Copy link

PTwr commented Jul 27, 2022

Because that's how EntityFramework does it by default and drop-in replacement had to follow this convention, regardless of our personal opinions :)

We'd need something similar to EF ModelBuilder Custom Conventions to override default behavior.

However with Dapper.Contrib being just a bunch of extension methods to DbConnection there is not much to attach configuration to. Distinguish by ConnectionString won't work for multitenancy. Static won't work with multiple databases. And bunch of bool parameters with default values, or overloads, would be ugly, not convenient, and bug prone.

@AartBluestoke
Copy link

I agree that if you are not told a key and have an ID column, ID is a reasonable guess.
The main issue is ID persisting as a hard coded key if another key is declared on the table.

@PTwr
Copy link

PTwr commented Jul 28, 2022

I agree that if you are not told a key and have an ID column, ID is a reasonable guess. The main issue is ID persisting as a hard coded key if another key is declared on the table.

That might trip over case where there is compound Key using that implicit Id column. If suddenly Id will stop being detected update might introduce breaking bug to existing projects.

Maybe [ExplicitKey(isKey: false)] will be enough to fix it?
Having Id column that's not a Key is an edge case, given how Dapper is used as upgrade/replacement to EF, so I'd go for option for disabling default behavior with full backward compatibility.

@AartBluestoke
Copy link

Having Id column that's not a Key is an edge case,

my use case is any integration with microsoft dynamics ; the primary key for entity X is always XId. it's a guid allocated by the system.
i wanted to call the column that was visible to the user "ID". i was using dapper. Hilarity ensued.
Perhaps a global dapper config "idIsImplicitKey" that defaults to True that can be turned off if needed.?

nseguin42 pushed a commit to nseguin42/Dapper.Contrib that referenced this issue Aug 3, 2023
@osc-nseguin
Refactor property info handling, allow keys different from Id property
a2f53a3 
- Only treat a property named 'Id' as a key if there is no Key or ExplicitKey property. (fixes DapperLib#140)
  The new behavior allows manually specifying a key (or explicit key) that is different from the 'Id' property. Any usage that would previously have succeeded (not thrown an exception) is unaffected.

- Introduce PropertyInfoWrapper to hold all the relevant information about a type's properties.

- Move logic about selecting a key into the PropertyInfoWrapper so it can be cached as well.

- Use a single ConcurrentDictionary of PropertyInfoWrappers instead of one for each type of property. Thread-safety is preserved because all mutation happens inside a thread-safe Lazy<T>.

- Add `PropertyNamedId` to distinguish this property from those explicitly annotated with a `KeyAttribute`.
@nseguin42 nseguin42 linked a pull request Aug 3, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor property info handling, allow keys different from Id property nseguin42/Dapper.Contrib
3 participants
@AartBluestoke @Luuk34 @PTwr