option to throw when result set has unmapped columns

[SurajGupta]

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.

[JimPiquant]

This gets my upvote, even a debug only option would be helpful. With larger projects and teams this is a constant issue with Dapper.

[DarekDan]

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.

[JimPiquant]

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.

[DarekDan]

If you want full- or medium-ORM functionality, consider Entity Framework or linq2db.

[JimPiquant]

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.

[DarekDan]

Need I say more?

[BenJury]

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?

[JimPiquant]

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".

[SurajGupta]

@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.

[DarekDan]

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.

[JimPiquant]

Dapper is not an object mapper? Why is it then that the title in the Readme is "Dapper - a simple object mapper for .Net"?

[DarekDan]

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

[anton89]

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

[DarekDan]

+1 @anton89

[SurajGupta]

@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.

[mgravell]

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 notifications@github.com 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

[anton89]

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

[SurajGupta]

@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)

[wlscaudill]

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...

[mgravell]

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 notifications@github.com 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

[JimPiquant]

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.

[SurajGupta]

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.

[SurajGupta]

@mgravell thoughts on moving forward with this?

[ShamsulAmry]

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).

[SurajGupta]

@mgravell - thoughts on pushing this through?

[wlscaudill]

I would really like the CommandFlags solution proposed by @mgravell.

[NickCraver]

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).

[justinjstark]

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.

[b-levitt]

+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

[NickCraver]

@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.

[justinjstark]

@NickCraver I'm actually advocating for the opposite. An error would be thrown if there is an object property that doesn't have a matching column from the SQL query.

I'm working on an app right now that uses Dapper to query an ERP system. We don't have the infrastructure in place at this time to do integration tests over it. I can write some adhoc tests to query existing data but it isn't repeatable over time. My best option now is to put my SQL queries into stored procedures. Then I can write a test for each object to ensure that each property has a matching column from the stored procedure that loads it. That works but bad aliases would be made more explicit if Dapper threw an exception when a property doesn't have a matching column.

[b-levitt]

@NickCraver - Doesn't the new column argument only apply if you're doing "select ". We don't need to argue the merits of whether that should or should not be done in the regular case, but I can say that many developers, do not do "select " and instead select the individual columns. That reduces the impact. Further, those that opt to use this option I think are implicitly opting to move the mapping concern to the sql itself (one of my points in my original post). And in that case those people perhaps SHOULD be selecting individual columns. Even if they opted not to do so by default, testing the addition of a new column should simply demonstrate that the select or the object itself must be modified and deployed before the additional column is created (if mgravell asserts "test better" in the counter case, it should also apply here).

We could counter here and say what if we rename a column? Should "BalanceDue" suddenly become 0 because we split it between columns PastDue and StatementBalance? I think it's that silent fail that scares the advocates of this feature.

[NickCraver]

many developers, do not do "select *" and instead select the individual columns. That reduces the impact.

Honestly, citation needed here. We use SELECT * in many, many, many cases. When you're doing custom queries and we're adding properties and developing in a pretty agile manner, you don't want to update 1,000 queries in the Stack Overflow codebase that involve the Posts table. The alternative of doing so is having a null field you didn't expect - which is the exact situation that's argued for fixing here.

Please take this as constructive: everyone is viewing this from their experience and not really understanding the other sides fully. As library maintainers, we have to look at the bigger picture for all of the use cases. Adding this feature can and will allow people to shoot themselves in the foot unexpectedly, from one side or the other. The question is: is this better than how you're able to shoot yourself in the foot now? I argue it's not, because now there are two ways with additional area to do so.

[b-levitt]

@NickCraver - This is how my old home-grown mapper worked and I quickly changed it. I had properties that were calculations over other properties and didn't like the idea of an "ignore" attribute. I guess my argument here is that the mapping concern is part of the sql. But your argument is that the mapping concern is part of the object definition itself. I don't agree since I want more flexibility in my objects, but I can see where you are coming from.

[NickCraver]

@justinjstark related: we'll be splitting apart Dapper soon (internally) - @mgravell is working on this now. Once that's in place there's a related issue (#292) where we want to have a Populate<T> method (called inside Query<T>) where you can hook into the DataReader (and access the schema if you want) - this would allow you to quickly write a test method like TestQuery<T>(string procName) that compares the schema and columns if you want.

[NickCraver]

@brrrdog There's also the issue that I'm honestly not sure everyone is on the same page even within this issue. The issue is for throwing on unmapped columns, @justinjstark is for throwing on unmapped properties. There are problems (increased chances of unintended fail) on either side with any additions, memoized properties, etc. I'm not against adding functionality (unless there's a performance cost - then it's a maybe), I'm simply against making things worse overall, and I think a feature in either direction here does that.

[b-levitt]

@NickCraver - I purposely avoided an assertion of "most" to avoid the need for a citation :). I realize that people do select *, but none of the developers I know do :). But in retrospect, does it really matter? We're talking about an elective here, and those that opt to use it know what they are getting into (from my perspective anyway).

I do agree there is some confusion. I do agree that @justinjstark has a separate issue.

But that is why I'll go back to my original post - are flags the right place for this? I see the reference in #135/#110 to override the default type mapper? Wouldn't a custom time mapper allow this to be done out of band?

[wlscaudill]

As a solution to my original question on StackOverflow, CommandFlags is still a perfectly viable option. As @SurajGupta pointed out, it allows for a few different types of checks to be specified together. I'm happy to have to set this and the default behavior NOT performing the check... @brrrdog, a type mapper solutions seems like a lot of overhead for a simple feature that would seemingly benefit a lot of people. As for the SELECT * comment, I typically specify the SQL as "SELECT ColumnNameInDatabase AS PropertyNameInObject, ... " which works well but for areas with missing test coverage it's difficult to find issues with refactoring the property name, so for me the flag to check would be a perfect option that I would set on each of my calls.

[b-levitt]

@wlscaudill - I'm not against CommandFlags. But I offered an olive branch for 3 reasons:

1)#132 is indicated as resolved via #110, but unsealing the default provider is still in question. I think the goal of overriding the default provider would provide a good solution (note: if you look thru #132, somebody posted a simple example of what they are trying to do. I'm NOT suggesting what has already been demonstrated on your SO question by implementing a new ITypeMap).

2) Tracing thru the code the most obvious place I saw to implement this feature was in the GetMembers of the type map provider or in GetTypeDeserializer after the call to GetMembers. In either case I do not see CommandFlags as a parameter there which makes me think this a big change. Maybe I'm wrong, but it seems pretty simple to throw an exception from GetMembers(f) where f wasn't found and it looks like there is already some work in the direction of overriding DefaultTypeMap

3) Putting it in a custom type map, eliminates the conversation of it should or should not be there.

Again, I'm not against the originally proposed command flags, I just thought I'd suggest something that might be more agreeable.

As far as your "select" scenario you do exactly what I do.

[wlscaudill]

I like your suggestions for many of the issues but I maintain that the TypeMap solution feels very heavy weight. The CommandFlags seems more appropriate and a very easy use case; something "CommandFlags.MapExact".

[b-levitt]

@wlscaudill, why do you say this would be 'heavy'? If DefaultTypeMap wasn't sealed, it looks to me that this could be handled in 4 or 5 lines of code to override GetMember (throw an exception instead of null), and one line to globally change the DefaultTypeMap. This seems conceptually sound, and at first glance, easier than propagating CommandFlags to places they don't currently go to.

[NickCraver]

@wlscaudill even in this issue, read up through the various use cases (and confusion). You'd need no less than 2 command flags:

• One for unmapped DB columns
• One for unmapped properties
• (Possibly) a convenience flag for both

That's not going to be very clear - and you'd have to pass it to every method call. Granted, we could do something like GlobalFlags I mentioned earlier, but that doesn't reduce the flag count or confusion.

[Danprot]

a grave problem! up!

[richardissimo]

I agree with the need to highlight when a column returned by the query does not exist on the entity. I don't care about the other way around; but I can see why people might want it.

I have just solved this exact problem for Dapper methods that return IEnumerable of dynamic, like Query and Read here (shameless request: please upvote my question there because people there clearly didn't understand what I was asking, but this request clearly indicates it wasn't a stupid question). And I'm now trying to solve the problem for Query of T and came across this suggestion. And I agree, that the proposed solution given by the original poster isn't really what I'm looking for.

I appreciate there are people who do things like "SELECT *" may not want this; but I would not use that approach, as it prevents SQL from being able to be as efficient as possible. I only return columns from my queries that I intend to use, and I'd want to know if any of the returned columns don't exist on the entity, because (in my case) they always should exist.

I would want this across all usages, so I'd want an easy way of switching it on for everything.

[richardissimo]

I had some more thoughts on this overnight...

@mgravell Re: "It feels like "test better" is the real fix here...?" You may be right. But the whole reason we (and probably you) use Dapper is to reduce having to write boilerplate code to fetch from the database. If I had to write a test that each property was coming back from the database for each query, I would be back to doing the boilerplate. Can I ask whether people at StackOverflow write a test for each query? Based on @NickCraver saying "you don't want to update 1,000 queries in the Stack Overflow codebase that involve the Posts table" I'm guessing that equally, you don't want to update 1000 tests, so I'm guessing you don't have those kind of tests, or you have some magic way of testing (which I would be interested in).

@DarekDan "you could use it entirely with dynamic". Yes you could; but that suffers the same problem. Unlike an out-of-the-box C# 'dynamic', the dynamic that you get back from Dapper does not throw a runtime exception if you try to access a property that isn't there (hence my fix for that mentioned in my post above). And by the way, using that approach would require us to write all the assignments ourselves, defeating the value of the mapping feature of Dapper. (And it is a feature of Dapper, so you can say what you like about Single Responsibility; but until Dapper relinquishes that responsibility #385, then it's an issue with Dapper.)

[richardissimo]

I've provided a solution to this problem on the StackOverflow question linked by the original poster.

[LarrySmith-1437]

Wondering what's become of this, as an integrated part of Dapper. It seems perfectly reasonable to ask Dapper to optionally throw exception on an unmapped column to object property.

[wlscaudill]

@LarrySmith-Parapet I agree but there has been nothing but consistent push back from the Dapper owners and all of the proposed solutions have been less than ideal.

[janmchan]

I would like for this to be an optional feature. That way one can use it in development environments while keeping it backwards compatible in production ones.

[LarrySmith-1437]

I implemented the proposed solution from Richardissimo in an extension to Dapper and it works aces. I highly recommend, AND can attest that it saved me from myself on a few occasions already.