Clojurians Log v2

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

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

What will it break?

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

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

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

postgres, at least, allows that happily

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

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

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

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

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

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

Huh?

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.

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.

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.

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

that's fair

I'm just saying this is valid right now:

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

I also agree that it should break

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

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

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

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?!?!"

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

right

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

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

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"

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

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

Thanks for fixing it @seancorfield

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

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