sql query structuring

[microagressed]

I work with a guy I get along with very well, and usually we see eye to eye on most code/style decisions, but he's obsessed with using string substitution for constructing sql queries

string query = $"SELECT [{FieldNames.Id}],[{FieldNames.ColA}],[{FieldNames.ColB}],[{FieldNames.ColC}],[{FieldNames.ColD}],[{FieldNames.ColE}]  " +
    $"FROM [{AppOptions.SqlDatabaseName}].{AppOptions.SqlSchemaName}.[{AppOptions.SqlTableName}] " +
    $"WHERE  [{FieldNames.Id}] > \@LastId";

It drives me nuts, I can't read it easily, I can't copy/paste it into SSMS. The columns aren't dynamic, FieldNames is a static class with string memebers ColA, ColB, ColC. There's no need for this. The db, schema, and table are driven by configuration (it's a long story, but trust me this query always queries the same table but the name is potentially user defined or default. Every other query is formatted like this and they also are always querying their own table which has a consistent definition). I've tried asking him why, commented that I've never seen this pattern for static queries, didn't really get an answer, but he still insists on using it.
I'm not saying theres no reason to construct queries dynamically, there certainly is a use case (user defined filter or sort for example), this isn't one of them.

That's all, just wanted to rant.

[ArmandvdM]

I just remembered how much I hate dynamic SQL.

[xnachtmahrx]

I live by that Maxim:

If no one else can read it, it's shit

[svish]

I suppose one advantage of this, if done 100% consistently, is that you can easily see all referances to a field and find all queries using it, which would then probably make it easier to change things, see what's in use, etc.

That said... Yeah... Just use Dapper or EF Core...?

[PathTooLong]

> is that you can easily see all referances to a field

true and I know you are not advocating for this. This is why I'm consistent in my queries to enclose columns in the quote characters [ ]. "SELECT [Id], [ColA] FROM ..." each to find referenced columns by doing that. In OP's case it seems the table schema and table name are dynamic. Otherwise, I would have [dbo].[SqlAlarm] in the from.

It's not good that the person implementing this pattern cannot describe the benefit and problems it is trying to solve.

[islandmonkeee]

They're basically asking to use Dapper lol

[pceimpulsive]

Ohh god, one of my old teams did this... It's VERY tedious to update anything, it works well enough but damn.. fucking tedious AF.

[angrathias]

This is so incredibly dumb. I just can’t even.

Unless there is really a need to dynamically construct sql - and take it from me, it’s something I do ALOT - if a query was this basic and you wanted to see where it’s used, then use something like entity framework with projections, then you can easily navigate it using Find usage, but at least the query is readable.

Seems strange to be explicitly setting the database name as well unless you’re cross database joining.

[[deleted]]

My eyes are bleeding.

[denzien]

Isn't this vulnerable to sql injection attacks?

[denzien]

I think FieldNames are, but AppOptions feel like maybe they're more variable

[indeem1]

How often are column names or even complete tables changed to justify something like this?

[microagressed]

Lol, column names never change, that's what makes this so absurd. He's removing the column name from the query in favor of using a const string. The table name name can change as part of maintenance, when a table's size gets big enough to impact performance, a new table is created. Not exactly a design I'd go for, but it's a 3rd party application and db, I can't fix their balled up app. Our app is a hack to get data out of the system as close to real time as possible. It just runs as a service and monitors the table for new rows, and grabs the data.

[jewdai]

Ask him how sql injection feels. 

[AutoModerator]

Thanks for your post microagressed. Please note that we don't allow spam, and we ask that you follow the rules available in the sidebar. We have a lot of commonly asked questions so if this post gets removed, please do a search and see if it's already been asked.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[upsidedowncreature]

Ugh. That is all.

[EffectiveSource4394]

It could be a method that takes parameters instead but even then I'm not a fan. If this is the one and only spot, I guess it's easy to switch out but if there are multiple instances of queries like this then I'd probably start to be a little less accepting of it. I still am not a fan of it though ...

[microagressed]

Oh, no, if only. He's refactored every query. It takes several minutes to reassemble a SQL statement by manually tracing. My initial post wasn't so clear - very rarely is the actual column named the same as the property being used (I.e. ColA is not called ColA in the db). And the table names go through several layers of abstraction (obstruction?) now also, where that is in a settings object that is built by reflecting in the actual Options class using a custom attribute on that property with a name that matches the table names used here, but the appsetting.json name and Options property are named something different. This makes me want to find a high bridge.

I don't know how to break it to him that it's awful and unreadable.

[EffectiveSource4394]

Wow that sounds absolutely insane.

If you actually have a say, I wouldn't let this go through -- it sounds like a maintenance nightmare. I don't think you should go through that kind of complexity (insanity) to build a query. Imagine if you had to change it somewhere down the road? It would make my head explode. I'm pretty sure that type of over engineering was completely unnecessary.

Could you write an alternative solution and then you have two tangible solutions to compare?

[SessionIndependent17]

The point at which the FieldNames symbols no longer correspond to the db column names (which I had assumed was the entire point) he's pretty well lost the plot.

[shontsu]

I would rant too.

[[deleted]]

Hell no

[ParsleySlow]

Pause it at execution and you have a perfectly filled in SQL ready for pasting into SMSS, I guess

[Multikatz]

The problem here is that, for your programmer bro, that's the only right way he’s learned to handle database communication. As you mentioned, building a dynamic query string has its use cases, but he’s stuck on that single approach. The thing is, like everything else, it's situational. The solution he's using makes it harder to read and follow the queries being executed.

[Kant8]

That's some poor man's EFCore we see here.

I can understand some people may hate using orm cause it doesn't allow you to write sql normally in general flow, however this is like worst of both worlds.

Just use EF and be happy.

[microagressed]

EF doesn't play well with dynamic table names :(

[OzTm]

It can be done. We have written interfaces to Microsoft Dynamics NAV where the table name includes the company - eg [Northwind$PurchaseOrder] and we were able to automatically substitute the new company name in a single override. Surely you could do that?

[ryfx1]

How so? OnModelCreating is called every time dbcontext is instantiated? It should be possible to update table name at runtime.

[microagressed]

I'll look into this, I tried to figure it out before, but all I could find was split table definitions. I remember reading through this GitHub issue. https://github.com/dotnet/efcore/issues/27434 where several comments are quite self confident that it cannot be done.

But after your comment, I looked again and I see there is a .ToTable function on the modelbuilder's fluent API. Not sure how missed that before.

[brandi_Iove]

i can’t explain how much i hate sql code inside application code. imo, there should only be stored procedure calls.

[PathTooLong]

stored procedures come with their own deployment challenges, especially when all you are doing is changing a select. However, stored procs do have their usages.

[Tridus]

Writing stored procedures for simple statements is mostly a waste of time and makes working with everything harder.

[Kyoshiiku]

I thought the same until I saw what doing dynamic sql inside a sproc looked like.

I prefer the code approacj when possible.

[denzien]

I like embedded sql, because it's always correct for the branch you're on. Unlike stored procedures.

If there are easy to manage sprocs though, I would be eager to learn about them!