DapperLib
option to throw when result set has unmapped columns
Here's a full description of the problem: https://stackoverflow.com/questions/28678442/how-can-i-make-dapper-net-throw-when-result-set-has-unmapped-columns
The solution seems really hacky. This would be a great feature to have to proactively surface bugs in queries.
This gets my upvote, even a debug only option would be helpful. With larger projects and teams this is a constant issue with Dapper.
Bad idea. Really bad idea. You want exception to be thrown? Use Automapper. Don't pollute decent micro-ORM due to someone's possible sloppiness.
Pollute? That is a specious argument. Making software work by surfacing errors because of "sloppiness" isn't a bad idea, it helps make frameworks usable.
If you want full- or medium-ORM functionality, consider Entity Framework or linq2db.
Using EF. Dapper is "a simple object mapper", not an ORM. It is the object mapping feature that would be easier to use and support if failures are not always silent.
Need I say more?
If it were so important, wouldn't a better way be to write a wrapper to perform such a check and use it in your unit tests?
Now your being just silly @DarekDan, keep it professional. An object mapper that fails to map throwing an exception doesn't come anywhere close to a SRP violation. Thanks for the suggestion @BenJury, but if the framework did this my unit tests would be far simpler and more likely to fail when someone gets "sloppy".
@DarekDan - Yes, actually you do need to say more. What does Query<T> do? It maps objects AND executes a query. If you're an absolute purist about SRP then the fact that there's an AND in the prior statement means that Query<T> is a violation of SRP. But practically speaking, it's convenient to do those two things together. It's also convenient to provide some mechanism to inform the caller that there are unmapped columns. There is certainly a fine line; Query<T> and others shouldn't become a dumping ground for features. It is definitely in a spirit of Dapper to keep things simple. But in this particular case, I would argue that checking for unmapped columns actually furthers Dapper's goal of simplicity. Checking for unmapped columns is a key missing step in the existing workflow, and having to remember that that capability does not exist actually feels unnatural and introduces complexity into the consuming code. The workflow in my head, divorced from Dapper's specific implementation, is:
- map object to SQL statement
- validate that mapping isn't making a false assumption
- execute the query.
Also, and as a side note, I too would encourage you to keep it professional. Comments like "Bad idea, really bad idea" discredit you. I'm always suspicious when people talk in absolutes. Maybe you mean "in my experience, this kind of feature has in retrospect been a bad idea..." which would allow for others to chime-in with their experience and ideas.
It should not be responsibility of Dapper to make sure that all data points requested are mapped to properties of a POCO. That is silly and unprofessional, dear @JimPiquant and @SurajGupta. And Dapper is not an object mapper. Automapper is. Dapper only provides convenience extensions to map, but you could use it entirely with dynamic.
Dapper is not an object mapper? Why is it then that the title in the Readme is "Dapper - a simple object mapper for .Net"?
I do recall many refer to it as micro-ORM. Object Relational Mapping. If you browse it on NuGet it reads:
A high performance Micro-ORM supporting Sql Server, MySQL, Sqlite, SqlCE, Firebird etc..
Can you use it to map ClassA to ClassB, I don't believe so. Can you use it to map relational data to a POC, yes, you can, hence a micro-ORM. We can split hairs here, deciding for StackExchange what it should be called. Bottom line, I will not support the idea of asking StackExchange to implement "properties mapping completeness".
Have you considered this approach instead? @SurajGupta http://stackoverflow.com/questions/7778216/automapper-or-similar-allow-mapping-of-dynamic-types
Come on guys, pretty sure Dapper goal isn't simplicity rather it was performance so to implement this will introduce unnecessary overhead, which we all know happen in all full featured ORM
@anton89 how so? If you add a bool parameter (i.e. throwIfUnmappedColumns) then you would only check for unmapped columns if throwIfUnmappedColumns=true You could set the default value to false for backward-compatibility. Performance would be equal to what it is right now. If you want to perform the check, then you take on the performance hit - which you should expect.
Performance is not a significant concern for this scenario, since the materialization strategy is computed per shape/type combination, and that would be the only time it would need to be checked. I would have concerns, though - if having spare columns (without properties) is a problem: is having spare properties (without columns) a problem? Is this an "exact shape match" check? If so: are different - but mappable - types an error? (int vs byte, for example)
Dapper tries to just be pragmatic - and from my own experience, the
ability to add columns to the database without breaking the app is a huge
advantage for deployment. I don't have a strong opinion on this, and I know
for a fact that I'd never use it. But it could conceivably be added to
CommandFlags. One problem of course being that every extra feature is
extra things to support and maintain.
On 31 March 2015 at 18:29, Suraj Gupta [email protected] wrote:
@anton89 https://github.com/anton89 how so? If you add a bool parameter (i.e. throwIfUnmappedColumns) then you would only check for unmapped columns if throwIfUnmappedColumns=true You could set the default value to false for backward-compatibility. Performance would be equal to what it is right now. If you want to perform the check, then you take on the performance hit - which you should expect.
— Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/254#issuecomment-88179477 .
Regards,
Marc
Yes the performance hit should be negligible and backward compatibility can easily be achieved, but I still think you should roll your own ITypeMap, I'm pretty sure it was there to be used like that, not hacky at all.
edit: also agreed with @mrgravell point
@mgravell - those are good questions. My example of the bool implementation was to counter the point on performance; to be clear I don't think it's a good implementation. CommandFlags is nice in that multiple checks can be added neatly within the enum. We'd have to guard against bloat and ask the basic question of whether we're butchering the intent of that type (if yes, then maybe a Flags enum specific to checking columns should be added). Do folks have thoughts on that?
Let's enumerate the cases (based on your questions) and see what opinions other people have?
- Column is missing property.
- Not all properties are used/mapped to a column
- Types are not mappable (does Dapper throw an exception in this case?)
- Types are different but mappable
I think 1, 3, and 4 are high value add. 2 seems academic but might be nice for completeness (and then you can have an ExactShape enum value which is 1 and 2)
Number 2 (Not all properties are used/mapped to a column) would be a really really useful feature for me.
My objects are using properties named "Id" and my tables have columns like "GoalId" and its easy to miss the "AS Id" when coding it up which doesn't get caught until integration testing or worse missed by a lack of testing.
The CommandFlags seems a reasonable place to add this...
It would be off by default; I can't help but think that this is unlikely to genuinely help many times; you're more likely to forget to turn it on than to have it save you. It feels like "test better" is the real fix here...?
On 31 March 2015 at 20:34, wlscaudill [email protected] wrote:
Number 2 (Not all properties are used/mapped to a column) would be a really really useful feature for me.
My objects are using properties named "Id" and my tables have columns like "GoalId" and its easy to miss the "AS Id" when coding it up which doesn't get caught until integration testing or worse missed by a lack of testing.
The CommandFlags seems a reasonable place to add this...
— Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/254#issuecomment-88219846 .
Regards,
Marc
In my view, failing as close to the error as possible improves your ability to test. If a mapping fails because of a change to a query the unit test will fail. If not, the error goes unnoticed.
I remember talking to Peter Spiro at Microsoft about how they stabilize the core SQL engine so well and he said it was adoption of a "fail fast" approach instead of a "run at any cost" mentality.
I would make it a policy to use the flag at my company. For my team, there would be no good reason to NOT use the flag.
I'm upvoting on this flag. I'm using Dapper for quick projects where there are no test and my own carelessness did cost me quite some time for me to realize that I wrongly name a property every once in a while. So a thrown exception for my own carelessness will really help (because I'm just plain lazy to create tests for quick projects).
I'm also not against the CommandFlags option, though I have to agree on limited practical impact that Marc mentions. If someone wants to submit a PR I'm happy to take a look, otherwise I'll crack at it when time allows (would likely be a while, lots in queue).
The ability to have an exception thrown when not all properties are mapped from a column (number 2 from @SurajGupta) would be beneficial. Since Dapper does the mapping to properties automatically, it is far too easy to mistype an SQL alias and wind up with a null property that may not get caught during testing.
+1 on this feature.
To me it seems clear that the onus for mapping is on the sql side. Isn't that the charm of dapper over larger (bloated) ORMs? It doesn't attempt to abstract away the very powerful SQL language with some mediocre replacement, and instead jumps in later in the mapping process. I think that absolves Dapper from being responsible for doing the mapping. But if that's the case, the query is the one that's responsible and with that the INTENT is to have the returned columns get mapped. If that isn't true than how does Dapper differentiate itself from other mapping tools and why are we not spending all day creating type maps in code?
I suppose you can argue SoC, but I don't think I would buy it. While custom type maps are possible, to me they can be the exception and not the rule. I doubt everybody is creating type maps for every one of their objects, and it would be tough for me to believe that those that aren't are "lucky" that their column names match the object. The column names a query returns can be a decision from its inception and that decision can favor the middle tier.
That said, I wonder if flags is the only option. I do think we're looking for a configurable option and not something set per call. With that, overriding DefaultTypeMap.GetMember seems like a decent place to do this since it is what is responsible for returning the shortened (only matching) member list in GetTypeDeserializer. But then it's a question of overriding the default type map (which isn't clear if its an option now?): https://github.com/StackExchange/dapper-dot-net/pull/132
@justinjstark by the same argument though, this makes it trivial to shoot yourself in the foot by adding a column while the app is live - while it wouldn't throw an error today, it would with this enabled. IMO, there's still not a clear win on either side here - and CommandFlags seems like the most viable option.
It could perhaps be combined with a default set of "global" command flags (set via a static) to give the best of all worlds. Beware though, you can easily screw yourself with or without this functionality - hence the hesitation to add it. @mgravell thoughts on global command flags? I'm thinking public CommandFlags Flags { get { return flags | GlobalFlags; } } would be how it behaves, still thinking through whether it's a good thing though.