Add Fragments (Issue #36)

[kelvinqian00]

GOAL: Add fragments, lines of SQL code that can be defined in .sql files in order to compose larger statements. This has been a long-requested feature; Issue #36 was created in 2016.

USAGE: Fragments are designed to be similar to other parameters (particularly snippets) to a HugSql library user. A user defines a fragment similar to a HugSql function or snippet:

-- :frag select-frag
select id, name

-- :frag from-frag
from test

-- :frag where-frag
where id = :id

and can use it like a snippet:

-- :name frag-query :? :*
:frag:select-frag
:frag:from-frag
:frag:where-frag

Then when frag-query is defined or compiled, the fragments are automatically expanded into the full SQL statement:

select id, name
from test
where id = :id

Fragments can also contain other fragments:

-- :frag where-frag-1
where id = :id

-- :frag where-frag-2
and name = :name

-- :frag where-frag
:frag:where-frag-1
:frag:where-frag-2

Fragments can also be written - and nested - in Clojure expressions:

-- :frag where-frag-1b
and id = :id

-- :frag where-frag-2b
and name = :name

-- :frag where-frag-cond
where 1
--~ (when (:id params) ":frag:where-frag-1b")
--~ (when (:name params) ":frag:where-frag-2b")

-- :name frag-query-cond :? :*
select id, name
from test
--~ (when (or (:id params) (:name params)) ":frag:where-frag-cond")

Note that fragments in Clojure expressions are expanded during runtime, in contrast to other fragments.

DESIGN: Internally, fragments operate very differently from other parameters. This is not only because (non-Clojure expression) fragments are expanded during function definition, but because fragments are not given as parameters to HugSql functions. More details are given in the fragments namespace, including how the fragment registry works and how fragments are registered and expanded.

TESTING: Tests for fragments have been added to the test namespaces. All core tests pass with H2 (and should pass on other DBMSs, though I have not tested that). Let me know if more tests are needed.

EXTRA CHANGE: I also added a line to remove excess whitespace from the final SQL strings. The main motivation was that fragment expansion creates a lot of excess whitespace, but this should help clean up SQL that contains a lot of Clojure expressions as well.

[csummers]

@kelvinqian00 This is excellent! I should have some time to review this in more detail in the next few days to get this merged in, but the first pass through it looks great. I like your careful attention to the fact that fragments differ from other parameters types since they must be considered/expanded before other parameter types. Thanks!

[csummers]

@kelvinqian00 OK, I've had some time to look at this and I have few comments:

I'm definitely seeing some test suite failures locally. To help see these failures in this PR, I've added a github actions workflow to run the test suite, and set up checks for PRs. You'll need to merge in the latest from master to make the checks run.

ERROR in (core) (QueryExecutorImpl.java:2497)
snippets and fragments
expected: (= [{:id 1, :name "A"}] (frag-query db {:id 1, :name "A"}))
  actual: org.postgresql.util.PSQLException: ERROR: argument of AND must be type boolean, not type integer
  Position: 33
 at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse (QueryExecutorImpl.java:2497)
    org.postgresql.core.v3.QueryExecutorImpl.processResults (QueryExecutorImpl.java:2233)
    org.postgresql.core.v3.QueryExecutorImpl.execute (QueryExecutorImpl.java:310)
    org.postgresql.jdbc.PgStatement.executeInternal (PgStatement.java:446)
    org.postgresql.jdbc.PgStatement.execute (PgStatement.java:370)
    org.postgresql.jdbc.PgPreparedStatement.executeWithFlags (PgPreparedStatement.java:149)
    org.postgresql.jdbc.PgPreparedStatement.executeQuery (PgPreparedStatement.java:108)
    clojure.java.jdbc$execute_query_with_params.invoke (jdbc.clj:1079)
    clojure.java.jdbc$db_query_with_resultset_STAR_.invoke (jdbc.clj:1102)
    clojure.java.jdbc$query.invoke (jdbc.clj:1171)
    clojure.java.jdbc$query.invoke (jdbc.clj:1149)
    clojure.lang.AFn.applyToHelper (AFn.java:156)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.core$apply.invoke (core.clj:634)
    hugsql.adapter.clojure_java_jdbc.HugsqlAdapterClojureJavaJdbc.query (clojure_java_jdbc.clj:15)
    hugsql.adapter$eval1103$fn__1166$G__1085__1171.invoke (adapter.clj:3)
    hugsql.adapter$eval1103$fn__1166$G__1084__1177.invoke (adapter.clj:3)
    clojure.lang.Var.invoke (Var.java:394)
    hugsql.core$db_fn_STAR_$y__2489.doInvoke (core.clj:511)
    clojure.lang.RestFn.invoke (RestFn.java:445)
    hugsql.core$db_fn_STAR_$y__2489.invoke (core.clj:501)
    hugsql.core_test$fn__4668$fn__6165$fn__6182.invoke (core_test.clj:479)
    hugsql.core_test$fn__4668$fn__6165.invoke (core_test.clj:478)
    hugsql.core_test/fn (core_test.clj:464)

Secondly, I want to make sure that I understand the fragment registry. Is this registry global to the project? I think that could be an issue for larger projects. At first, it may seem like Clojure expressions have the same issue, but they are identified solely by their hash for the SQL string...which gives a singular compiled function for the SQL that will run. With fragments, which are identified by name, I think we must provide some consideration of the namespace.

This would make the def'ing of the available set of fragments an explicit operation. If users want to have a set of common-fragments.sql fragments that they provide to the namespace via def-db-fns, then, at the cost of some extra typing, it is at least clear where the fragments are being defined. Something like:

(hugsql/def-db-fns "path/to/common-fragments.sql")
(hugsql/def-db-fns "path/to/employees.sql")

I'm not sure what's best, but I'm thinking either:

• consider the current ns in the global registry
• def a var/atom of hugsql fragments in each namespace

[kelvinqian00]

Regarding the failing test case, that's not because of an error in the fragments code, but an error with the test case itself. Specifically, one of the fragments was where 1 instead of where true, and I did not catch that because I only tested it with H2. I should've tested it with the other DBMSs, so I apologize for that.

I was able to run the cases for all DBMSs with the exception for Apache Derby, which I couldn't get working for some reason. For the others, some of the instructions for Postgresql and MySql need to be updated; for example, the MySql user needs to be created before grant all is executed or else you'll get an error. (I'm also programming on a Mac, so perhaps that will also affect things if these instructors were meant for Linux.)

[kelvinqian00]

As for your namespaces question, yes, right now all fragments are stored in a global registry (you can see this as the single atom in fragments.clj). It shouldn't be too hard to implement either of your two potential solutions. In addition, we might want to let users explicitly define the namespace in the fragment name, to match the behavior of namespaced keywords for regular HugSql functions.

The main issue I can think of, though, is how would namespaced fragment names be referenced in the .sql files. Would we want the names to always include the namespaces, or (similar to Clojure code) only need namespaces when referring to fragments from a different file/namespace? The former would be simpler to implement, but the latter might be what users would expect.