(PDB-5161) engine/parse-dot-query: strictly parse fields

Replace ad-hoc parsing with parse-field.  To do so add :indexes? and
:matches? options to allow treating segments that would otherwise be
recognized as match()es or array indexes as literal named fields,
allowing us to maintain the previous behavior for now.

Author: rbrw

src/puppetlabs/puppetdb/query_eng/engine.clj

@@ -1631,29 +1631,49 @@
       (instance? ArrayBinaryExpression node)
       (instance? ArrayRegexExpression node)))
 
-(defn path->nested-map
-  "Given path [a b c] and value d, produce {a {b {c d}}}"
-  [path value]
-  (reduce #(hash-map (parse/maybe-strip-escaped-quotes %2) %1)
-          (rseq (conj (vec path) value))))
-
 (defn parse-dot-query
   "Transforms a dotted query into a JSON structure appropriate
    for comparison in the database."
   [{:keys [field value] :as node} state]
-  (let [[column & path] (map parse/maybe-strip-escaped-quotes
-                             (parse/dotted-query->path field))]
-    (if (some #(re-matches #"^\d+$" %) path)
-      ;; This path is taken for match() operators that match indexes,
-      ;; i.e.  facts.foo.match("\d+") because by this point they'll be
-      ;; plain integer path components.
-      {:node (assoc node :value ["?" "?"] :field column :array-in-path true)
-       ;; https://www.postgresql.org/docs/11/arrays.html#ARRAYS-INPUT
-       :state (reduce conj state [(jdbc/strs->db-array path)
+  ;; node is JsonContainsExpression
+  (let [[column & path] (parse/parse-field field 0 {:indexes? false
+                                                    :matches? false})
+        ;; FIXME: this is a conflation - one fix might be to parse all
+        ;; fields *once* at the start of query processing, and then
+        ;; pass that representation all the way through.
+        ;;
+        ;; The handling of match() wrt fact_paths complicates that
+        ;; because right now we don't have any (efficient) way of
+        ;; knowing all the types of the path elements we retrieve from
+        ;; the path_array, i.e. for ["foo" "5" "bar"], is "5" a custom
+        ;; fact name, or an array lookup?  The situation could be
+        ;; different if we pushed the path lookups further down, and
+        ;; didn't insinuate them via AST.  At the moment, we preserve
+        ;; backward compatibility by just treating everything as a
+        ;; named field by setting indexes? and matches? above.
+        maybe-index? #(re-matches #"[0-9]+" (:name %))]
+    (if (some maybe-index? path)
+      ;; If this matches, then the path component might be an array
+      ;; access, e.g. via the database-generated paths created by
+      ;; maybe-add-match-function-filter like foo.5.bar, and so we
+      ;; have to be conservative and set array-in-path so that we'll
+      ;; eventually use the right json operators (e.g. #> instead of
+      ;; @>).
+      {:node (assoc node
+                    :value ["?" "?"]
+                    :field (:name column)
+                    :array-in-path true)
+       :state (reduce conj state [(jdbc/strs->db-array (map :name path))
                                   (su/munge-jsonb-for-storage value)])}
-      {:node (assoc node :value "?" :field column :array-in-path false)
-       :state (conj state (su/munge-jsonb-for-storage
-                           (path->nested-map path value)))})))
+      {:node (assoc node
+                    :value "?"
+                    :field (:name column)
+                    :array-in-path false)
+       :state (conj state
+                    (su/munge-jsonb-for-storage
+                     ;; Convert path [a b c] and value d to {a {b {c d}}}
+                     (reduce (fn [result name] (hash-map name result))
+                             (cons value (reverse (map :name path))))))})))
 
 (defn parse-dot-query-with-array-elements
   "Transforms a dotted query into a JSON structure appropriate

src/puppetlabs/puppetdb/query_eng/parse.clj

@@ -55,7 +55,7 @@
 (defn- index-or-name
   "Returns an ::indexed-field-part segment if s is of the form name[digits],
   otherwise a ::named-field-part segment."
-  [s]
+  [s indexes?]
   (if-let [[_ n i] (re-matches #"(?s:(.+)\[(\d+)\])" s)]
     ;; Must be Integer, not Long to avoid pg errors like this:
     ;; "ERROR: operator does not exist: jsonb -> bigint"
@@ -65,7 +65,11 @@
 (defn- parse-field-components
   "Parses the components of a dotted query field that come after the
   first, and conjoins a map describing each one onto the result."
-  [^String s offset result]
+  [^String s offset
+   {:keys [indexes? matches?]
+    :or {indexes? true matches? true}
+    :as opts}
+   result]
   (let [field-m (re-matcher field-rx s)
         match-m (re-matcher match-rx s)
         qfield-m (re-matcher quoted-field-rx s)]
@@ -94,7 +98,7 @@
                         {:kind ::invalid-field-component
                          :field s
                          :offset i}))
-              (find-at match-m i)
+              (and matches? (find-at match-m i))
               (recur (.end match-m)
                      (conj result {:kind ::match-field-part
                                    :pattern (.group match-m 1)}))
@@ -103,10 +107,12 @@
               ;; hopefully a bit easier to follow.
               (find-at qfield-m i)
               (recur (.end qfield-m)
-                     (conj result (index-or-name (.group qfield-m 1))))
+                     (conj result (index-or-name (.group qfield-m 1)
+                                                 indexes?)))
               (find-at field-m i)
               (recur (.end field-m)
-                     (conj result (index-or-name (.group field-m))))
+                     (conj result (index-or-name (.group field-m)
+                                                 indexes?)))
               ;; Probably currently unreachable
               :else (throw
                      (ex-info (format "Don't recognize AST field component at character %d: %s"
@@ -119,8 +125,9 @@
   "Parses an AST field like \"certname\", \"facts.partition[3]\" and
   returns a vector of the field components as maps.  The first
   component will always be a ::named-field-part."
-  ([s] (parse-field s 0))
-  ([s offset]
+  ([s] (parse-field s 0 {}))
+  ([s offset opts]
+   (assert (string? s))
    (when (= offset (count s))
      (throw (ex-info "Empty AST field" {:kind ::invalid-field :field s})))
    (let [field-m (re-matcher top-field-rx s)]
@@ -132,7 +139,7 @@
                   :offset 0})))
      ;; Q: OK to disallow an initial quoted-field?
      (let [result [{:kind ::named-field-part :name (.group field-m)}]]
-       (parse-field-components s (.end field-m) result)))))
+       (parse-field-components s (.end field-m) opts result)))))
 
 (defn- quote-path-name-for-field-str [s]
   (when (re-matches #"(?s:.*\..*\\)" s)