From 6f4aa7978d9c75add069090d75558eeedbd16441 Mon Sep 17 00:00:00 2001 From: Yogthos Date: Wed, 18 Feb 2026 11:54:31 +0800 Subject: [PATCH] Fix 8 bugs: NPE on missing EDN target, error map typo, unsafe read-string, silent git failures, dead code, and missing nil checks - Return data unchanged instead of NPE when EDN injection target path missing - Fix :path keyword literal to actual path value in read-files error map - Replace read-string with edn/read-string in git.clj, modules.clj, kit_clj.clj - Re-throw git sync exceptions as ex-info instead of silently printing - Use ex-info instead of Exception in edn-safe-merge - Remove duplicate :xtdb? condition and dead check-conflicts function - Add nil guard in read-config for missing resource files - Add test infrastructure for kit-core and new tests for injections --- libs/kit-core/deps.edn | 8 ++- libs/kit-core/src/kit/config.clj | 4 +- libs/kit-core/test/kit/config_test.clj | 14 +++++ libs/kit-generator/src/kit/generator/git.clj | 12 ++-- .../src/kit/generator/modules.clj | 3 +- .../src/kit/generator/modules/injections.clj | 20 ++++--- .../test/kit_generator/injections_test.clj | 57 +++++++++++++++++++ .../src/leiningen/new/io/github/kit_clj.clj | 14 ++--- 8 files changed, 107 insertions(+), 25 deletions(-) create mode 100644 libs/kit-core/test/kit/config_test.clj create mode 100644 libs/kit-generator/test/kit_generator/injections_test.clj diff --git a/libs/kit-core/deps.edn b/libs/kit-core/deps.edn index defb02d9..04af00d9 100644 --- a/libs/kit-core/deps.edn +++ b/libs/kit-core/deps.edn @@ -1,4 +1,10 @@ {:paths ["src"] :deps {aero/aero {:mvn/version "1.1.6"} integrant/integrant {:mvn/version "1.0.0"} - org.clojure/tools.logging {:mvn/version "1.2.4"}}} + org.clojure/tools.logging {:mvn/version "1.2.4"}} + :aliases {:test + {:extra-paths ["test"] + :extra-deps {io.github.cognitect-labs/test-runner + {:git/tag "v0.5.1" :git/sha "dfb30dd"}} + :main-opts ["-m" "cognitect.test-runner"] + :exec-fn cognitect.test-runner.api/test}}} diff --git a/libs/kit-core/src/kit/config.clj b/libs/kit-core/src/kit/config.clj index 061b7e2e..817a5a69 100644 --- a/libs/kit-core/src/kit/config.clj +++ b/libs/kit-core/src/kit/config.clj @@ -16,6 +16,8 @@ (defn read-config [filename options] (log/info "Reading config" filename) - (aero/read-config (io/resource filename) options)) + (if-let [resource (io/resource filename)] + (aero/read-config resource options) + (throw (ex-info "Config resource not found" {:filename filename})))) (defmethod ig/init-key :system/env [_ env] env) \ No newline at end of file diff --git a/libs/kit-core/test/kit/config_test.clj b/libs/kit-core/test/kit/config_test.clj new file mode 100644 index 00000000..e5c11250 --- /dev/null +++ b/libs/kit-core/test/kit/config_test.clj @@ -0,0 +1,14 @@ +(ns kit.config-test + (:require + [clojure.test :refer [deftest is testing]] + [kit.config :as config])) + +(deftest read-config-missing-resource + (testing "read-config throws ExceptionInfo for nonexistent config file" + (try + (config/read-config "nonexistent-config-file.edn" {}) + (is false "should have thrown") + (catch clojure.lang.ExceptionInfo e + (is (= "nonexistent-config-file.edn" + (:filename (ex-data e)))) + (is (re-find #"Config resource not found" (ex-message e))))))) diff --git a/libs/kit-generator/src/kit/generator/git.clj b/libs/kit-generator/src/kit/generator/git.clj index d6cb0848..85d72a2d 100644 --- a/libs/kit-generator/src/kit/generator/git.clj +++ b/libs/kit-generator/src/kit/generator/git.clj @@ -1,6 +1,7 @@ (ns kit.generator.git (:require [clj-jgit.porcelain :as git] + [clojure.edn :as edn] [clojure.java.io :as jio] [clojure.string :as string] [kit.generator.io :as io]) @@ -20,7 +21,7 @@ (defn git-config [] (if (.exists (jio/file "kit.git-config.edn")) - (read-string (slurp "kit.git-config.edn")) + (edn/read-string (slurp "kit.git-config.edn")) {:name "~/.ssh/id_rsa"})) (defn sync-repository! [root {:keys [name url tag]} & [callback]] @@ -40,8 +41,9 @@ :clone-all? false))) (when callback (callback path)))) (catch org.eclipse.jgit.api.errors.TransportException e - (println (.getMessage e) - "\nif you do not have a key file, set the :name key in kit.git-config.edn to an empty string")) + (throw (ex-info (str (.getMessage e) + "\nif you do not have a key file, set the :name key in kit.git-config.edn to an empty string") + {:error ::transport-error :url url} e))) (catch Exception e - (println "failed to clone module:" url "\ncause:" (.getMessage e)) - (.printStackTrace e)))) + (throw (ex-info (str "failed to clone module: " url) + {:error ::clone-error :url url} e))))) diff --git a/libs/kit-generator/src/kit/generator/modules.clj b/libs/kit-generator/src/kit/generator/modules.clj index 85c4058c..8db4e65e 100644 --- a/libs/kit-generator/src/kit/generator/modules.clj +++ b/libs/kit-generator/src/kit/generator/modules.clj @@ -1,6 +1,7 @@ (ns kit.generator.modules "Module loading and resolution." (:require + [clojure.edn :as edn] [clojure.java.io :as jio] [kit.generator.features :as features] [kit.generator.git :as git] @@ -92,7 +93,7 @@ (file-seq) (keep #(when (= "modules.edn" (.getName %)) (set-module-paths root (assoc - (read-string (slurp %)) + (edn/read-string (slurp %)) :module-root (-> % .getParentFile .getName))))) ;; TODO: Warn if there are modules with the same key from different repositories. (apply merge) diff --git a/libs/kit-generator/src/kit/generator/modules/injections.clj b/libs/kit-generator/src/kit/generator/modules/injections.clj index 8b71a3a2..bc8130d8 100644 --- a/libs/kit-generator/src/kit/generator/modules/injections.clj +++ b/libs/kit-generator/src/kit/generator/modules/injections.clj @@ -115,13 +115,16 @@ zloc) ((edn-merge-value value) zloc)))) (catch Exception e - (throw (Exception. (str "error merging!\n target:" zloc "\n value:" value) e))))) + (throw (ex-info (str "error merging!\n target:" zloc "\n value:" value) + {:error ::merge-error} + e))))) (defn zloc-get-in [zloc [k & ks]] (if-not k zloc - (recur (z/get zloc k) ks))) + (when zloc + (recur (z/get zloc k) ks)))) (defn zloc-conj [zloc value] (-> zloc @@ -137,7 +140,8 @@ (defn z-update-in [zloc [k & ks] f] (if k - (z-update-in (z/get zloc k) ks f) + (when zloc + (z-update-in (z/get zloc k) ks f)) (when zloc (f zloc)))) @@ -155,11 +159,13 @@ (if (empty? target) (zloc-conj data value) (or (z-update-in data target #(zloc-conj % value)) - (println "could not find injection target:" target "in data:" (z/node data)))) + (do (println "could not find injection target:" target "in data:" (z/node data)) + data))) :merge (if-let [zloc (zloc-get-in data target)] (edn-safe-merge zloc value) - (println "could not find injection target:" target "in data:" (z/node data)))) + (do (println "could not find injection target:" target "in data:" (z/node data)) + data))) ;;TODO find a better way to do this z/root-string z/of-string))) @@ -353,9 +359,9 @@ (read-file path) (assoc path->data path)) (catch Exception e - (throw (ex-info (str "Failed to read asset:" path) + (throw (ex-info (str "Failed to read asset: " path) {:error ::read-asset - :path :path} + :path path} e))))) {} paths)) diff --git a/libs/kit-generator/test/kit_generator/injections_test.clj b/libs/kit-generator/test/kit_generator/injections_test.clj new file mode 100644 index 00000000..a7bdcec2 --- /dev/null +++ b/libs/kit-generator/test/kit_generator/injections_test.clj @@ -0,0 +1,57 @@ +(ns kit-generator.injections-test + (:require + [clojure.test :refer [deftest is testing]] + [kit.generator.modules.injections :as injections] + [rewrite-clj.zip :as z])) + +;; Fix 1 — EDN injection NPE on missing target +(deftest inject-edn-missing-target-append + (testing ":append with nonexistent target returns original data unchanged" + (let [data (z/of-string "{:a 1}") + result (injections/inject + {:type :edn + :data data + :target [:nonexistent :path] + :action :append + :value ":new-val"})] + (is (some? result) "should not NPE") + (is (= "{:a 1}" (z/root-string result)))))) + +(deftest inject-edn-missing-target-merge + (testing ":merge with nonexistent target returns original data unchanged" + (let [data (z/of-string "{:a 1}") + result (injections/inject + {:type :edn + :data data + :target [:nonexistent :path] + :action :merge + :value "{:b 2}"})] + (is (some? result) "should not NPE") + (is (= "{:a 1}" (z/root-string result)))))) + +;; Fix 2 — Error map bug in read-files +(deftest read-files-error-map + (testing "read-files with nonexistent path throws ex-info with correct :path" + (let [bad-path "/nonexistent/file.edn"] + (try + (injections/read-files {} [bad-path]) + (is false "should have thrown") + (catch clojure.lang.ExceptionInfo e + (is (= bad-path (:path (ex-data e))) + ":path should be the actual path string, not the keyword :path") + (is (re-find #"Failed to read asset: " (ex-message e)) + "message should have a space after 'asset:'")))))) + +;; Fix 5 — edn-safe-merge should throw ExceptionInfo, not plain Exception +(deftest edn-safe-merge-throws-ex-info + (testing "edn-safe-merge wraps errors in ex-info" + (try + ;; Force an error by passing invalid args that will fail during merge + (injections/edn-safe-merge + (z/of-string "not-a-map") + (z/of-string "{:a 1}")) + (is false "should have thrown") + (catch clojure.lang.ExceptionInfo _e + (is true "threw ExceptionInfo as expected")) + (catch Exception _e + (is false "should throw ExceptionInfo, not plain Exception"))))) diff --git a/libs/lein-template/src/leiningen/new/io/github/kit_clj.clj b/libs/lein-template/src/leiningen/new/io/github/kit_clj.clj index 02d30190..a1abf8b8 100644 --- a/libs/lein-template/src/leiningen/new/io/github/kit_clj.clj +++ b/libs/lein-template/src/leiningen/new/io/github/kit_clj.clj @@ -6,6 +6,7 @@ [leiningen.new.io.github.kit-clj.options.helpers :as helpers] [leiningen.new.io.github.kit-clj.options.sql :as sql] [io.github.kit-clj.deps-template.helpers :refer [generate-cookie-secret]] + [clojure.edn :as edn] [clojure.java.io :as io] [clojure.set :as set] [clojure.walk :as walk])) @@ -23,7 +24,7 @@ (def versions (-> (io/resource "io/github/kit_clj/kit/versions.edn") (slurp) - (read-string) + (edn/read-string) (walk/keywordize-keys))) (defn template-data [name options] @@ -35,7 +36,7 @@ :default-cookie-secret (if (helpers/option? "+override-default-cookie-secret" options) "test-secret" (generate-cookie-secret)) - :xtdb? (or full? (helpers/option? "+xtdb" options) (helpers/option? "+xtdb" options)) + :xtdb? (or full? (helpers/option? "+xtdb" options)) ;; SQL data coercion :sql? (or full? (helpers/option? "+sql" options)) @@ -104,18 +105,11 @@ (when abort? (throw (ex-info "Error: invalid profile(s)" {}))))) -(defn check-conflicts - [options] - #_(when (> (count (filter #{"+full" "+bare"} options)) - 1) - (throw (ex-info "Cannot have both +full and +bare profile present" {})))) - (defn check-options "Check the user-provided options" [options] (doto options - (check-available) - (check-conflicts))) + (check-available))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;