honeysql

Discussion of https://github.com/seancorfield/honeysql :slightly_smiling_face:
2018-09-03T01:35:20.000100Z

bja 2018-09-03T01:40:03.000100Z

@seancorfield this assert should be marked as BREAKING in the changelog I think

bja 2018-09-03T01:43:14.000100Z

or is the rationale that isn't it's not really a valid thing it doesn't need BREAKING?

seancorfield 2018-09-03T01:48:02.000100Z

What will it break?

seancorfield 2018-09-03T01:48:44.000100Z

In what world is (h/select [:field]) or (h/select [:a :b :c]) actually valid?

bja 2018-09-03T01:49:00.000100Z

any jr person's code who wrote (helpers/sql [:foo]) and then accessed the resulting row with nil

bja 2018-09-03T01:49:24.000100Z

technically SELECT 1 as NULL returns a resultset with a column called NULL

bja 2018-09-03T01:49:30.000200Z

postgres, at least, allows that happily

bja 2018-09-03T01:50:07.000100Z

let me load up a honeysql project and write a test case for you

seancorfield 2018-09-03T01:50:16.000100Z

Sure, but who actually writes that and intends to get SELECT somefield AS NULL... ?

bja 2018-09-03T01:50:19.000100Z

I'm also not saying you should EVER have done that

bja 2018-09-03T01:50:28.000100Z

I'm saying a jr might easily just guess and test

seancorfield 2018-09-03T01:50:41.000100Z

I'd rather it blew up and someone asked/complained about it...

bja 2018-09-03T01:51:03.000100Z

they see that result of (h/select [:foo]) means they need to (map #(get % nil) to access their foo

seancorfield 2018-09-03T01:51:26.000100Z

Huh?

seancorfield 2018-09-03T01:53:36.000100Z

There is no world in which that is correct code. If anyone has that sort of nonsense in production, it should break. It should never have worked.

seancorfield 2018-09-03T01:55:35.000100Z

So, like I say, with that assert in there, if it actually breaks anyone's production code, I want them to speak up and explain why they need it to work.

seancorfield 2018-09-03T01:56:28.000100Z

Also (h/select [:a :b :c]) silently produced SELECT a AS b and ignore c so that should also break, IMO. It was never defined behavior.

seancorfield 2018-09-03T01:57:19.000100Z

Several people have complained recently that this happens so I figure it's worth making it blow up.

bja 2018-09-03T01:59:23.000200Z

that's fair

bja 2018-09-03T01:59:50.000100Z

I'm just saying this is valid right now:

(map :null (clojure.java.jdbc/query db ["select 1 as null"]))

bja 2018-09-03T02:00:09.000100Z

I also agree that it should break

bja 2018-09-03T02:00:29.000100Z

in fact, if you rely on this stupid behavior, you should be forced to defend your [:foo nil] alias under code review

bja 2018-09-03T02:01:03.000100Z

I'm coming from the viewpoint of someone who has seen exactly this mistake (although I caught it code review)

seancorfield 2018-09-03T02:02:00.000100Z

Well, if you explicitly say (h/select [:foo nil]) that won't blow up. That will very specifically produce SELECT foo AS NULL... right?

bja 2018-09-03T02:03:33.000100Z

right, the migration path for people would be [:foo] becomes [:foo nil], but "did you really mean to alias foo as NULL to begin with?!?!"

seancorfield 2018-09-03T02:05:12.000100Z

The root of this is that if you don't use the helpers, then {:select [:field] ...} is fine and does what you expect (`SELECT field ...`) but if you use the helpers and do (h/select [:field]) by mistake, you get SELECT field AS null ... which folks have complained about so...

bja 2018-09-03T02:05:27.000100Z

right

bja 2018-09-03T02:05:48.000100Z

(although I suspect that what we really want is a set of specs to enforce, I'm just not willing to write that code)

bja 2018-09-03T02:07:01.000100Z

given that this is an issue all over the place in the honeysql helpers vs underlying AST

bja 2018-09-03T02:13:57.000100Z

a spec still doesn't necessarily help since we can't really disambiguate between (h/select [:foo :bar]) intending "SELECT foo as bar" from "SELECT foo, bar"

seancorfield 2018-09-03T02:26:06.000100Z

Yeah, the underlying difference between the helpers and DSL is frustrating.

seancorfield 2018-09-03T02:26:45.000100Z

I've been wrestling with the inline/raw thing for a few weeks...

2018-09-03T07:29:29.000100Z

Thanks for fixing it @seancorfield

2018-09-03T07:29:56.000200Z

And I don't agree it's a breaking change, whoever was relying it surely just had a silly bug in their code

2018-09-03T07:30:15.000100Z

Which they will happy to find out, and it's quite clear from the error anyway