Skip to content

Commit 1d40531

Browse files
yogthosyogthos
authored
Merge pull request #174 from kit-clj/bug-fixes
Fix 8 bugs: NPE on missing EDN target, error map typo, unsafe read-string, silent git failures, dead code, and missing nil checks
2 parents 8b8cc1a + 6f4aa79 commit 1d40531

8 files changed

Lines changed: 107 additions & 25 deletions

File tree

‎libs/kit-core/deps.edn‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
11
{:paths ["src"]
22
:deps {aero/aero {:mvn/version "1.1.6"}
33
integrant/integrant {:mvn/version "1.0.0"}
4-
org.clojure/tools.logging {:mvn/version "1.2.4"}}}
4+
org.clojure/tools.logging {:mvn/version "1.2.4"}}
5+
:aliases {:test
6+
{:extra-paths ["test"]
7+
:extra-deps {io.github.cognitect-labs/test-runner
8+
{:git/tag "v0.5.1" :git/sha "dfb30dd"}}
9+
:main-opts ["-m" "cognitect.test-runner"]
10+
:exec-fn cognitect.test-runner.api/test}}}

‎libs/kit-core/src/kit/config.clj‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
(defn read-config
1717
[filename options]
1818
(log/info "Reading config" filename)
19-
(aero/read-config (io/resource filename) options))
19+
(if-let [resource (io/resource filename)]
20+
(aero/read-config resource options)
21+
(throw (ex-info "Config resource not found" {:filename filename}))))
2022

2123
(defmethod ig/init-key :system/env [_ env] env)

‎libs/kit-core/test/kit/config_test.clj‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
(ns kit.config-test
2+
(:require
3+
[clojure.test :refer [deftest is testing]]
4+
[kit.config :as config]))
5+
6+
(deftest read-config-missing-resource
7+
(testing "read-config throws ExceptionInfo for nonexistent config file"
8+
(try
9+
(config/read-config "nonexistent-config-file.edn" {})
10+
(is false "should have thrown")
11+
(catch clojure.lang.ExceptionInfo e
12+
(is (= "nonexistent-config-file.edn"
13+
(:filename (ex-data e))))
14+
(is (re-find #"Config resource not found" (ex-message e)))))))

‎libs/kit-generator/src/kit/generator/git.clj‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
(ns kit.generator.git
22
(:require
33
[clj-jgit.porcelain :as git]
4+
[clojure.edn :as edn]
45
[clojure.java.io :as jio]
56
[clojure.string :as string]
67
[kit.generator.io :as io])
@@ -20,7 +21,7 @@
2021

2122
(defn git-config []
2223
(if (.exists (jio/file "kit.git-config.edn"))
23-
(read-string (slurp "kit.git-config.edn"))
24+
(edn/read-string (slurp "kit.git-config.edn"))
2425
{:name "~/.ssh/id_rsa"}))
2526

2627
(defn sync-repository! [root {:keys [name url tag]} & [callback]]
@@ -40,8 +41,9 @@
4041
:clone-all? false)))
4142
(when callback (callback path))))
4243
(catch org.eclipse.jgit.api.errors.TransportException e
43-
(println (.getMessage e)
44-
"\nif you do not have a key file, set the :name key in kit.git-config.edn to an empty string"))
44+
(throw (ex-info (str (.getMessage e)
45+
"\nif you do not have a key file, set the :name key in kit.git-config.edn to an empty string")
46+
{:error ::transport-error :url url} e)))
4547
(catch Exception e
46-
(println "failed to clone module:" url "\ncause:" (.getMessage e))
47-
(.printStackTrace e))))
48+
(throw (ex-info (str "failed to clone module: " url)
49+
{:error ::clone-error :url url} e)))))

‎libs/kit-generator/src/kit/generator/modules.clj‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
(ns kit.generator.modules
22
"Module loading and resolution."
33
(:require
4+
[clojure.edn :as edn]
45
[clojure.java.io :as jio]
56
[kit.generator.features :as features]
67
[kit.generator.git :as git]
@@ -92,7 +93,7 @@
9293
(file-seq)
9394
(keep #(when (= "modules.edn" (.getName %))
9495
(set-module-paths root (assoc
95-
(read-string (slurp %))
96+
(edn/read-string (slurp %))
9697
:module-root (-> % .getParentFile .getName)))))
9798
;; TODO: Warn if there are modules with the same key from different repositories.
9899
(apply merge)

‎libs/kit-generator/src/kit/generator/modules/injections.clj‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -115,13 +115,16 @@
115115
zloc)
116116
((edn-merge-value value) zloc))))
117117
(catch Exception e
118-
(throw (Exception. (str "error merging!\n target:" zloc "\n value:" value) e)))))
118+
(throw (ex-info (str "error merging!\n target:" zloc "\n value:" value)
119+
{:error ::merge-error}
120+
e)))))
119121

120122
(defn zloc-get-in
121123
[zloc [k & ks]]
122124
(if-not k
123125
zloc
124-
(recur (z/get zloc k) ks)))
126+
(when zloc
127+
(recur (z/get zloc k) ks))))
125128

126129
(defn zloc-conj [zloc value]
127130
(-> zloc
@@ -137,7 +140,8 @@
137140

138141
(defn z-update-in [zloc [k & ks] f]
139142
(if k
140-
(z-update-in (z/get zloc k) ks f)
143+
(when zloc
144+
(z-update-in (z/get zloc k) ks f))
141145
(when zloc
142146
(f zloc))))
143147

@@ -155,11 +159,13 @@
155159
(if (empty? target)
156160
(zloc-conj data value)
157161
(or (z-update-in data target #(zloc-conj % value))
158-
(println "could not find injection target:" target "in data:" (z/node data))))
162+
(do (println "could not find injection target:" target "in data:" (z/node data))
163+
data)))
159164
:merge
160165
(if-let [zloc (zloc-get-in data target)]
161166
(edn-safe-merge zloc value)
162-
(println "could not find injection target:" target "in data:" (z/node data))))
167+
(do (println "could not find injection target:" target "in data:" (z/node data))
168+
data)))
163169
;;TODO find a better way to do this
164170
z/root-string
165171
z/of-string)))
@@ -353,9 +359,9 @@
353359
(read-file path)
354360
(assoc path->data path))
355361
(catch Exception e
356-
(throw (ex-info (str "Failed to read asset:" path)
362+
(throw (ex-info (str "Failed to read asset: " path)
357363
{:error ::read-asset
358-
:path :path}
364+
:path path}
359365
e)))))
360366
{} paths))
361367

‎libs/kit-generator/test/kit_generator/injections_test.clj‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
(ns kit-generator.injections-test
2+
(:require
3+
[clojure.test :refer [deftest is testing]]
4+
[kit.generator.modules.injections :as injections]
5+
[rewrite-clj.zip :as z]))
6+
7+
;; Fix 1 — EDN injection NPE on missing target
8+
(deftest inject-edn-missing-target-append
9+
(testing ":append with nonexistent target returns original data unchanged"
10+
(let [data (z/of-string "{:a 1}")
11+
result (injections/inject
12+
{:type :edn
13+
:data data
14+
:target [:nonexistent :path]
15+
:action :append
16+
:value ":new-val"})]
17+
(is (some? result) "should not NPE")
18+
(is (= "{:a 1}" (z/root-string result))))))
19+
20+
(deftest inject-edn-missing-target-merge
21+
(testing ":merge with nonexistent target returns original data unchanged"
22+
(let [data (z/of-string "{:a 1}")
23+
result (injections/inject
24+
{:type :edn
25+
:data data
26+
:target [:nonexistent :path]
27+
:action :merge
28+
:value "{:b 2}"})]
29+
(is (some? result) "should not NPE")
30+
(is (= "{:a 1}" (z/root-string result))))))
31+
32+
;; Fix 2 — Error map bug in read-files
33+
(deftest read-files-error-map
34+
(testing "read-files with nonexistent path throws ex-info with correct :path"
35+
(let [bad-path "/nonexistent/file.edn"]
36+
(try
37+
(injections/read-files {} [bad-path])
38+
(is false "should have thrown")
39+
(catch clojure.lang.ExceptionInfo e
40+
(is (= bad-path (:path (ex-data e)))
41+
":path should be the actual path string, not the keyword :path")
42+
(is (re-find #"Failed to read asset: " (ex-message e))
43+
"message should have a space after 'asset:'"))))))
44+
45+
;; Fix 5 — edn-safe-merge should throw ExceptionInfo, not plain Exception
46+
(deftest edn-safe-merge-throws-ex-info
47+
(testing "edn-safe-merge wraps errors in ex-info"
48+
(try
49+
;; Force an error by passing invalid args that will fail during merge
50+
(injections/edn-safe-merge
51+
(z/of-string "not-a-map")
52+
(z/of-string "{:a 1}"))
53+
(is false "should have thrown")
54+
(catch clojure.lang.ExceptionInfo _e
55+
(is true "threw ExceptionInfo as expected"))
56+
(catch Exception _e
57+
(is false "should throw ExceptionInfo, not plain Exception")))))

‎libs/lein-template/src/leiningen/new/io/github/kit_clj.clj‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
[leiningen.new.io.github.kit-clj.options.helpers :as helpers]
77
[leiningen.new.io.github.kit-clj.options.sql :as sql]
88
[io.github.kit-clj.deps-template.helpers :refer [generate-cookie-secret]]
9+
[clojure.edn :as edn]
910
[clojure.java.io :as io]
1011
[clojure.set :as set]
1112
[clojure.walk :as walk]))
@@ -23,7 +24,7 @@
2324

2425
(def versions (-> (io/resource "io/github/kit_clj/kit/versions.edn")
2526
(slurp)
26-
(read-string)
27+
(edn/read-string)
2728
(walk/keywordize-keys)))
2829

2930
(defn template-data [name options]
@@ -35,7 +36,7 @@
3536
:default-cookie-secret (if (helpers/option? "+override-default-cookie-secret" options)
3637
"test-secret"
3738
(generate-cookie-secret))
38-
:xtdb? (or full? (helpers/option? "+xtdb" options) (helpers/option? "+xtdb" options))
39+
:xtdb? (or full? (helpers/option? "+xtdb" options))
3940
;; SQL data coercion
4041
:sql? (or full?
4142
(helpers/option? "+sql" options))
@@ -104,18 +105,11 @@
104105
(when abort?
105106
(throw (ex-info "Error: invalid profile(s)" {})))))
106107

107-
(defn check-conflicts
108-
[options]
109-
#_(when (> (count (filter #{"+full" "+bare"} options))
110-
1)
111-
(throw (ex-info "Cannot have both +full and +bare profile present" {}))))
112-
113108
(defn check-options
114109
"Check the user-provided options"
115110
[options]
116111
(doto options
117-
(check-available)
118-
(check-conflicts)))
112+
(check-available)))
119113

120114

121115
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

0 commit comments

Comments
 (0)