df/load! makes query invalid for components with a union query

[scizo]

First off, thanks for Fulcro. We have just been getting started with it, but it is clear the that a lot of care and thought have gone into it!

When using df/load! with a component that has a union query, the query gets altered into an invalid query before being sent to server. Here is a diff for the data fetch specs with a failing test to demonstrate.

diff --git a/src/test/com/fulcrologic/fulcro/data_fetch_spec.cljc b/src/test/com/fulcrologic/fulcro/data_fetch_spec.cljc
index cf188b31..acea2b73 100644
--- a/src/test/com/fulcrologic/fulcro/data_fetch_spec.cljc
+++ b/src/test/com/fulcrologic/fulcro/data_fetch_spec.cljc
@@ -30,12 +30,22 @@
    :ident         [:parent/by-id :x]
    :query         [:x :z {:child (comp/get-query InitTestChild)}]})

+(defsc UnionComponent [this props]
+  {:initial-state (fn [params] {:a/id 1 :a/name "Alice"})
+   :ident (fn [] (cond
+                   (:a/id props) [:a/id (:a/id props)]
+                   (:b/id props) [:b/id (:b/id props)]
+                   :else nil))
+   :query (fn [] {:a/id [:a/id :a/name]
+                  :b/id [:b/id :b/another-prop]})})
+
 (def app (app/fulcro-app))

 (specification "Load parameters"
   (let [
         query-with-params       (:query (df/load-params* app :prop Person {:params {:n 1}}))
-        ident-query-with-params (:query (df/load-params* app [:person/by-id 1] Person {:params {:n 1}}))]
+        ident-query-with-params (:query (df/load-params* app [:person/by-id 1] Person {:params {:n 1}}))
+        union-query (:query (df/load-params* app :prop UnionComponent {:without #{}}))]
     (assertions
       "Accepts nil for subquery and params"
       (:query (df/load-params* app [:person/by-id 1] nil {})) => [[:person/by-id 1]]
@@ -48,7 +58,9 @@
       (:target (df/load-params* app :prop Person {:target [:a :b]})) => [:a :b]
       "Constructs a JOIN query (with params on join and prop)"
       query-with-params => `[({:prop ~(comp/get-query Person)} {:n 1})]
-      ident-query-with-params => `[({[:person/by-id 1] ~(comp/get-query Person)} {:n 1})]))
+      ident-query-with-params => `[({[:person/by-id 1] ~(comp/get-query Person)} {:n 1})]
+      "Properly handles components with a union query"
+      union-query => [{:prop (comp/get-query UnionComponent)}]))
   (behavior "can focus the query"
     (assertions
       (:query (df/load-params* app [:item/by-id 1] Item {:focus [:name {:comments [:title]}]}))

This results in the following failure:

FAIL in com.fulcrologic.fulcro.data-fetch-spec/__Load-parameters__ (data_fetch_spec.cljc:49)
union-query => [{:prop (comp/get-query UnionComponent)}]
expected: [{:prop {:a/id [:a/id :a/name], :b/id [:b/id :b/another-prop]}}]
  actual: [{:prop [[:a/id [:a/id :a/name]] [:b/id [:b/id :b/another-prop]]]}]
╭───── Test output ───────────────────────────────────────────────────────
│ 24-12-07 22:09:56 neva.local WARN [com.fulcrologic.fulcro.data-fetch:110] - Data load targets of two elements imply that you are targeting a table entry. That is probably incorrect. Normalization targets tables. Targeting is for creating missing edges, which are usually 3-tuples. See https://book.fulcrologic.com/#warn-data-load-targets-table
╰─────────────────────────────────────────────────────────────────────────

The root of the problem is that by default, all component queries pass through the df/elide-query-nodes function with #{} as the node predicate. df/elide-query-nodes converts the query into the ast representation in order to filter out nodes and then converts it back into the query format. Converting from a query into ast and back to a query assumes that it is a valid standalone query. From what I can tell union queries are not valid at the top level of an eql query. It seems that at the top level eql queries must be a vector. Round tripping the standalone union query through the ast turns the query into one that is invalid.

This can be shown directly with the following code:

(def union-query {:a/id [:a/id] :b/id [:b/id]})
(def query-with-root [{:root/property union-query}]

(-> query-with-root
    edn-query-langauage.core/query->ast
    edn-query-language.core/ast->query
    (= query-with-root)) ; => true

(-> union-query
    edn-query-language.core/query->ast
    edn-query-language.core/ast->query
    (= union-query)) ; => false

A straightforward and safe way to fix this bug should be to wrap queries in a temporary root and then unwrap them, since it cannot be safely assumed that the queries being passed in are valid on their own.

Here is a simple patch that fixes the above test and demonstrates taking this approach.

diff --git a/src/main/com/fulcrologic/fulcro/data_fetch.cljc b/src/main/com/fulcrologic/fulcro/data_fetch.cljc
index 190feb6f..52f169c0 100644
--- a/src/main/com/fulcrologic/fulcro/data_fetch.cljc
+++ b/src/main/com/fulcrologic/fulcro/data_fetch.cljc
@@ -66,11 +66,17 @@
           new-ast
           (dissoc new-ast :children))))))

+(defn add-temp-root [query]
+  [{::temp-root query}])
+
+(defn remove-temp-root [query]
+  (get-in query [0 ::temp-root]))
+
 (defn elide-query-nodes
   "Remove items from a query when the query element where the (node-predicate key) returns true. Commonly used with
    a set as a predicate to elide specific well-known UI-only paths."
   [query node-predicate]
-  (-> query eql/query->ast (elide-ast-nodes node-predicate) eql/ast->query))
+  (-> query add-temp-root eql/query->ast (elide-ast-nodes node-predicate) eql/ast->query remove-temp-root))

 (defn load-params*
   "Internal function to validate and process the parameters of `load` and `load-action`."

If this all feels reasonable to you, I would happily put together a pull request with this test and some version of this solution. Let me know and please give me any style preferences or guidance if you have some.

Thanks in advance for taking the time to look at this and for Fulcro!

[awkay]

Oops. Yes, in EQL a union is not a valid top-level query. df/load composes the prop and component into a join, which would make it a legal EQL, but it the union is at the component being "handled", then you're right that the EQL transform will muck it up.

I think the proper fix would be to apply the elide after load has added in the join. That should correct the problem I think?

[awkay]

Well, perhaps your fix is better, given that there are a lot of "conditionals" within the transform logic :/

[scizo]

I originally was going down the path of applying the elide after, but I wasn't as confident that it wouldn't mess around with other things. For example, the focus must happen before the join has been added, otherwise it would be a breaking change. I wasn't sure all of the consequences of moving it, so I just figured the temp wrapper makes it legal and correct without effecting anything outside of it.