install addon from file, preserve 'ignore' and 'pin' flags (#434)

* cli, adds test 'install-addon--ignore-then-update-from-file'
  - it recreates what is being seen in issue #433
* core.clj, download-time tests duplicated at install-time.
  - but with different consequences.
* cli.clj, installing an addon from a file now goes through 'core', not 'addon'.
* core, relaxes restriction on what core/install-addon considers installable.
* addon.clj, special handling for nfo flags when installing addon.
  - if an addon is ignored then the new nfo data will inherit the ignore flag.  
  - if an addon is pinned then thew new nfo data will remove the pinned-version flag.
  - it shouldn't be possible to reach this logic right now, core/install-addon should prevent that.
* core.clj, install-addon, replaced 'test-only?' parameter with map of options.
* addon.clj, fixed bug where ignore check fails on single dir addons
  - it assumed all addons had a :group-addons field, but that only exists for addons that have been grouped because there are more than one of them.
  - pretty serious bug.
* core/install-addon, adds option to bypass ignore and pinned flags
* core/remove-many-addons, now refuses to remove ignored addons
* addon/remove-addon, just warns when ignored addon is being removed
* cli/install-addons-from-file, now accepts a map of installation options
* jfx/zip-file-picker, changed default behaviour when installing from file
  - ignored addons are now overwritten
  - pinned addons are now unpinned
* review feedback
* CHANGELOG

CHANGELOG.md

@@ -8,12 +8,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+* error handling for a few cases where uncaught exceptions were being swallowed.
+
 ### Changed
 
+* installing an addon by file now overwrites/updates ignored addons and unpins pinned addons.
+    - this accommodates a use case where Strongbox is used to install a select few addons manually from unsupported sources (like Curseforge) but then the new addons match against older versions in the catalogue from wowi.
+    - please open a feature request if you want this behaviour adjustable.
+
 ### Fixed
 
 * fixed bug in emergency 'short' catalogue generation.
     - it would include addons from BFA instead of stopping at Shadowlands.
+* fixed bug where installing a file manually would bypass ignored and pinned addon checks.
+    - it would tell you it was refusing to install, then install it anyway.
+* fixed bug where ignore check bypassed on ungrouped/single directory addons.
+    - logic assumed all addons were grouped, but grouping only happens when an addon unzip to multiple directories.
+    - pretty big bug!
 
 ### Removed
 

TODO.md

@@ -8,8 +8,45 @@ see CHANGELOG.md for a more formal list of changes by release
 
 ## todo
 
+* issue 433: preserve ignore and pin flags when addon updated manually via strongbox
+    - usecase: 
+        - old addon exists on wowi
+        - new addon exists on curse
+        - user installs new addon from curse and then 'ignores' it so nothing affects it
+        - user installs updated addon from curse and ignore flag is lost
+    - problems:
+        - if the addon was ignored, it shouldn't have been possible to overwrite it to begin with
+        - it seems reasonable to update addons in this way
+
+bug:
+    - attempting to overwrite ignored addon results in "refusing to delete ignored addon: /path/to/addon/dir" with no addon name
+    - it looks like it installed it anyway
+
+
+* Desktop Entry Name should not include "(Flatpak)" 
+    - https://github.com/flathub/la.ogri.strongbox/issues/7
+
 ## todo bucket (no particular order)
 
+* gui, when installing an addon from file, allow the following options
+    - test-only?
+    - unpin-pinned?
+    - overwrite-ignored?
+
+bug: clear button isn't clearing search
+
+bug: 
+    select github + wowi
+    search for 'alto'
+        - 3 results, 2 github, 1 wowi
+    deselect 'github'
+        - no results
+    select gitlab (so wowi and gitlab are selected)
+        - wowi result
+
+bug: search, next and previous buttons should take me to top of search results
+
+
 * patch tooltip, support multiple game versions
 
 * flathub, remove '(Flathub)' from .desktop file

src/strongbox/addon.clj

@@ -79,10 +79,13 @@
   (if (:ignore? addon)
     ;; if addon is being ignored, refuse to remove addon.
     ;; note: `group-addons` will add a top level `:ignore?` flag if any addon in a bundle is being ignored.
-    (error "refusing to delete ignored addon:" install-dir)
+    ;; 2024-08-25: behaviour changed. this is not the place to prevent ignored addons from being removed.
+    ;; see `core/install-addon`, `core/remove-many-addons`
+    ;;(error "refusing to delete ignored addon:" (:label addon))
+    (warn "deleting ignored addon:" (:label addon)))
 
-    (doseq [grouped-addon (flatten-addon addon)]
-      (-remove-addon! install-dir (:dirname grouped-addon) (:group-id addon)))))
+  (doseq [grouped-addon (flatten-addon addon)]
+    (-remove-addon! install-dir (:dirname grouped-addon) (:group-id addon))))
 
 ;;
 
@@ -276,8 +279,13 @@
 (defn-spec install-addon (s/or :ok (s/coll-of ::sp/extant-file), :error ::sp/empty-coll)
   "installs an addon given an addon description, a place to install the addon and the addon zip file itself.
   handles suspicious looking bundles, conflicts with other addons, uninstalling previous addon version and updating nfo files.
-  returns a list of nfo files that were written to disk."
-  [addon :addon/nfo-input-minimum, install-dir ::sp/writeable-dir, downloaded-file ::sp/archive-file]
+
+  relies on `core/install-addon` to block installations that would overwrite ignored or pinned addons.
+  if it's gotten this far ignored/pinned addons will be deleted
+  and the new addon will be unzipped over the top.
+
+  returns a list of nfo files that were written to disk, if any."
+  [addon :addon/nfo-input-minimum, install-dir ::sp/writeable-dir, downloaded-file ::sp/archive-file, opts ::sp/install-opts]
   (let [nom (or (:label addon) (:name addon) (fs/base-name downloaded-file))
         version (:version addon)
 
@@ -288,6 +296,15 @@
 
         zipfile-entries (zip/zipfile-normal-entries downloaded-file)
         toplevel-dirs (zip/top-level-directories zipfile-entries)
+
+        toplevel-nfo (->> toplevel-dirs ;; [{:path "EveryAddon/", ...}, ...]
+                          (map :path) ;; ["EveryAddon/", ...]
+                          (map fs/base-name) ;; ["EveryAddon", ...]
+                          (map #(nfo/read-nfo-file install-dir %))) ;; [`(read-nfo-file install-dir "EveryAddon"), ...]
+
+        contains-nfo-with-ignored-flag (utils/any (map :ignore? toplevel-nfo))
+        contains-nfo-with-pinned-version (utils/any (map :pinned-version toplevel-nfo))
+
         primary-dirname (determine-primary-subdir toplevel-dirs)
 
         ;; let the user know if there are bundled addons and they don't share a common prefix
@@ -306,10 +323,26 @@
                         (let [addon-dirname (:path zipentry)
                               primary? (= addon-dirname (:path primary-dirname))
                               new-nfo-data (nfo/derive addon primary?)
+                              ;; if any of the addons this addon is replacing are being ignored,
+                              ;; the new nfo will be ignored too.
+                              new-nfo-data (if contains-nfo-with-ignored-flag
+                                             (nfo/ignore new-nfo-data)
+                                             new-nfo-data)
+
+                              new-nfo-data (if contains-nfo-with-pinned-version
+                                             (nfo/unpin new-nfo-data)
+                                             new-nfo-data)
+
                               new-nfo-data (nfo/add-nfo install-dir addon-dirname new-nfo-data)]
                           (nfo/write-nfo! install-dir addon-dirname new-nfo-data)))
 
         ;; write the nfo files, return a list of all nfo files written
+        ;; todo: if a zip file is being installed then we can't rely on `remove-addon!` having been called,
+        ;; but `remove-completely-overwritten-addons` will have been called and *may* have removed the
+        ;; addon *if* the new addon is a superset of the old one.
+        ;; this leads to the possibility of a new addon that has dropped a subdir or added a new one (like a rename)
+        ;; being skipped and orphaning the original subdir.
+        ;; this means we could hit `unzip-addon` with the original addon still fully intact.
         update-nfo-files (fn []
                            (mapv update-nfo-fn toplevel-dirs))
 
@@ -343,12 +376,16 @@
     (suspicious-bundle-check)
 
     ;; todo: remove support for v1 addons in 2.0.0 ;; todo!
-    ;; when is it not valid? when importing v1 addons. v2 addons need 'padding' as well :(
+    ;; when is it not valid?
+    ;; * when importing v1 addons. v2 addons need 'padding' as well :(
+    ;; * when installing from a file and we have nothing more than a generated ID value
     (when (s/valid? :addon/toc addon)
       (remove-addon! install-dir addon))
 
     (remove-completely-overwritten-addons)
 
+    ;; `addon/install-addon` is all about installing an addon, not checking whether it's safe to do so.
+    ;; use `core/install-addon` for safety checks.
     (unzip-addon)
     (update-nfo-files)))
 
@@ -388,7 +425,7 @@
 (defn-spec ignored-dir-list (s/coll-of ::sp/dirname)
   "returns a list of unique addon directory names (including grouped addons) that are not being ignored"
   [addon-list (s/nilable :addon/installed-list)]
-  (->> addon-list (filter :ignore?) (map :group-addons) flatten (map :dirname) (remove nil?) set))
+  (->> addon-list (filter :ignore?) (map flatten-addon) flatten (map :dirname) (remove nil?) set))
 
 (defn-spec overwrites-ignored? boolean?
   "returns `true` if given archive file would unpack over *any* ignored addon.

src/strongbox/cli.clj

@@ -312,10 +312,11 @@
                  (let [downloaded-file (core/download-addon-guard-affective addon install-dir)
                        existing-dirs (addon/dirname-set addon)
                        updated-dirs (zipfile-locks downloaded-file)
-                       locks-needed (clojure.set/union existing-dirs updated-dirs)]
+                       locks-needed (clojure.set/union existing-dirs updated-dirs)
+                       opts {}]
                    (swap! new-dirs into updated-dirs)
                    (utils/with-lock current-locks locks-needed
-                     (core/install-addon-affective addon install-dir downloaded-file)
+                     (core/install-addon-guard-affective addon install-dir opts downloaded-file)
                      (core/refresh-addon addon))))]
     (run! #(joblib/create-addon-job! queue-atm % job-fn) updateable-addon-list)
     (joblib/run-jobs! queue-atm core/num-concurrent-downloads)
@@ -363,7 +364,7 @@
           error-messages
           (logging/buffered-log
            :warn
-           (addon/install-addon addon (core/selected-addon-dir) downloaded-file))]
+           (core/install-addon addon (core/selected-addon-dir) downloaded-file))]
       (core/refresh)
       {:label (fs/base-name downloaded-file)
        :error-messages error-messages}))
@@ -372,7 +373,7 @@
   "installs/updates a list of addon zip files in parallel.
   does a clever refresh check afterwards to try and prevent a full refresh from happening.
   very similar code to `install-update-these-in-parallel`."
-  [download-file-list (s/coll-of ::sp/extant-archive-file)]
+  [download-file-list (s/coll-of ::sp/extant-archive-file), opts ::sp/install-opts]
   (let [queue-atm (core/get-state :job-queue)
         install-dir (core/selected-addon-dir)
         current-locks (atom #{})
@@ -389,7 +390,7 @@
                      (let [error-messages
                            (logging/buffered-log
                             :warn
-                            (let [results (addon/install-addon addon install-dir downloaded-file)]
+                            (let [results (core/install-addon addon install-dir downloaded-file opts)]
                               (when-let [installed-addon-dir (some-> results first fs/parent str)]
                                 (core/refresh-addon* installed-addon-dir))))]
 

src/strongbox/core.clj

@@ -490,6 +490,8 @@
       (try
         (logging/with-addon addon
           (apply wrapped-fn addon args))
+        (catch Exception ex
+          (error ex "uncaught error calling wrapped function"))
         (finally
           (-stop-affecting-addon))))))
 
@@ -536,9 +538,11 @@
             (fs/delete downloaded-file)
             (warn "removed bad addon."))
 
+        ;; test is duplicated in `install-addon` as it's possible to just download and check addon without installing it.
         (addon/overwrites-ignored? downloaded-file (get-state :installed-addon-list))
         (error "refusing to install addon that will overwrite an ignored addon.")
 
+        ;; test is duplicated in `install-addon` as it's possible to just download and check addon without installing it.
         (addon/overwrites-pinned? downloaded-file (get-state :installed-addon-list))
         (error "refusing to install addon that will overwrite a pinned addon.")
 
@@ -549,31 +553,66 @@
 
 (defn-spec install-addon (s/or :ok (s/coll-of ::sp/extant-file), :passed-tests true?, :error nil?)
   "downloads an addon and installs it, bypassing checks. see `install-addon-guard`."
-  [addon :addon/installable, install-dir ::sp/extant-dir, downloaded-file (s/nilable ::sp/extant-archive-file)]
+  [addon (s/or :ok :addon/installable, :also-ok :addon/nfo-input-minimum), install-dir ::sp/extant-dir, downloaded-file (s/nilable ::sp/extant-archive-file), opts ::sp/install-opts]
   (when downloaded-file
     (try
-      (addon/install-addon addon install-dir downloaded-file)
+
+      ;; because addons can enter strongbox via downloading or via the user,
+      ;; we can't rely on all checks having happened at download time.
+      ;; this means some checks will be duplicated for downloaded files
+      ;; with different consequences for failing.
+      ;; for example, if the addon is invalid at download time, delete it.
+      ;; if the addon is invalid at install time, ignore it.
+
+      (cond
+        (not (zip/valid-zip-file? downloaded-file))
+        (error "failed to read addon zip file, possibly corrupt or not a zip file.")
+
+        (not (zip/valid-addon-zip-file? downloaded-file))
+        (error "refusing to install, addon zip file contains top-level files or a top-level directory missing a .toc file.")
+
+        (and (addon/overwrites-ignored? downloaded-file (get-state :installed-addon-list))
+             (not (:overwrite-ignored? opts)))
+        (error "refusing to install addon that will overwrite an ignored addon.")
+
+        (and (addon/overwrites-pinned? downloaded-file (get-state :installed-addon-list))
+             (not (:unpin-pinned? opts)))
+        (error "refusing to install addon that will overwrite a pinned addon.")
+
+        :else (addon/install-addon addon install-dir downloaded-file opts))
+
+      (catch Exception ex
+        (error ex "Uncaught exception installing addon"))
+
       (finally
-        (addon/post-install addon install-dir (get-state :cfg :preferences :addon-zips-to-keep))))))
+        ;; future: post-install steps for addons installed manually are skipped because there is no `:name` value,
+        ;; only a `grouped-id` value.
+        (when (:name addon)
+          (addon/post-install addon install-dir (get-state :cfg :preferences :addon-zips-to-keep)))))))
 
-(def install-addon-affective
-  (affects-addon-wrapper install-addon))
+;; 2024-09-01: disabled, I want all installation to go through `install-addon-guard`
+#_(def install-addon-affective
+    (affects-addon-wrapper install-addon))
 
 (defn-spec install-addon-guard (s/or :ok (s/coll-of ::sp/extant-file), :passed-tests true?, :error nil?)
   "downloads an addon and installs it, handling http and non-http errors, bad zip files, bad addons, bad directories."
   ([addon :addon/installable]
-   (install-addon-guard addon (selected-addon-dir) false))
+   (install-addon-guard addon (selected-addon-dir) {}))
 
   ([addon :addon/installable, install-dir ::sp/extant-dir]
-   (install-addon-guard addon install-dir false))
+   (install-addon-guard addon install-dir {}))
+
+  ([addon :addon/installable, install-dir ::sp/extant-dir, opts ::sp/install-opts]
+   (install-addon-guard addon install-dir opts (download-addon-guard addon install-dir)))
 
-  ([addon :addon/installable, install-dir ::sp/extant-dir, test-only? boolean?]
-   (when-let [downloaded-file (download-addon-guard addon install-dir)]
-     (if test-only?
-       ;; addon was successfully downloaded and verified as being sound. stop here.
-       true
-       ;; else, install addon
-       (install-addon addon install-dir downloaded-file)))))
+  ([addon :addon/installable, install-dir ::sp/extant-dir, opts ::sp/install-opts, downloaded-file (s/nilable ::sp/extant-archive-file)]
+   (let [{:keys [test-only?]} opts]
+     (when downloaded-file
+       (if test-only?
+         ;; addon was successfully downloaded and verified as being sound. stop here.
+         true
+         ;; else, install addon
+         (install-addon addon install-dir downloaded-file opts))))))
 
 (def install-addon-guard-affective
   (affects-addon-wrapper install-addon-guard))
@@ -1251,7 +1290,7 @@
               ;; not necessary when updating the user-catalogue.
               _ (if-not attempt-dry-run?
                   true
-                  (or (install-addon-guard addon (selected-addon-dir) true)
+                  (or (install-addon-guard addon (selected-addon-dir) {:test-only? true})
                       (error "failed dry-run installation")))]
 
              ;; if-let* was successful!
@@ -1739,22 +1778,15 @@
       ;; todo: could this be a half-refresh instead?
       (refresh))))
 
-;; todo: move to ui.cli
-#_(defn-spec remove-addon nil?
-    "removes given installed addon"
-    [installed-addon :addon/installed]
-    (logging/with-addon installed-addon
-      (addon/remove-addon! (selected-addon-dir) installed-addon))
-    (refresh))
-
-;; todo: move to ui.cli
 (defn-spec remove-many-addons nil?
   "deletes each of the addons in the given `toc-list` and then calls `refresh`"
   [installed-addon-list :addon/toc-list]
   (let [addon-dir (selected-addon-dir)]
     (doseq [installed-addon installed-addon-list]
       (logging/with-addon installed-addon
-        (addon/remove-addon! addon-dir installed-addon)))
+        (if (addon/ignored? installed-addon)
+          (error "refusing to delete ignored addon:" (:label installed-addon))
+          (addon/remove-addon! addon-dir installed-addon))))
     (refresh)))
 
 ;;

src/strongbox/jfx.clj

@@ -1007,10 +1007,12 @@
   (when-let [abs-path-list (file-chooser {:filters [{:description "ZIP files" :extensions ["*.zip"]}]
                                           :type :open-multi
                                           :initial-dir (core/selected-addon-dir)})]
-    (doseq [{:keys [error-messages label]} (cli/install-addons-from-file-in-parallel abs-path-list)]
-      (when-not (empty? error-messages)
-        (let [msg (message-list (format "warnings/errors while installing \"%s\"" label) error-messages)]
-          (alert :warning msg {:wait? false})))))
+    (let [opts {:overwrite-ignored? true
+                :unpin-pinned? true}]
+      (doseq [{:keys [error-messages label]} (cli/install-addons-from-file-in-parallel abs-path-list opts)]
+        (when-not (empty? error-messages)
+          (let [msg (message-list (format "warnings/errors while installing \"%s\"" label) error-messages)]
+            (alert :warning msg {:wait? false}))))))
   nil)
 
 (defn exit-handler

src/strongbox/nfo.clj

@@ -261,6 +261,11 @@
 
 ;; ignoring
 
+(defn-spec ignore :addon/nfo
+  "add a `ignore?` flag to given `nfo` data."
+  [nfo :addon/nfo]
+  (assoc nfo :ignore? true))
+
 (defn-spec ignore! nil?
   "prevent any changes made by strongbox to this addon. 
   explicitly ignores this addon by setting the `ignore?` flag to `true`."
@@ -286,6 +291,11 @@
   [install-dir ::sp/extant-dir, addon-dirname ::sp/dirname, version :addon/pinned-version]
   (update-nfo! install-dir addon-dirname {:pinned-version version}))
 
+(defn-spec unpin :addon/nfo
+  "remove pin flag from given `nfo` data"
+  [nfo :addon/nfo]
+  (dissoc nfo :pinned-version))
+
 (defn-spec unpin! nil?
   "removes `:pinned-version` from a specific addon's nfo file, if it exists"
   [install-dir ::sp/extant-dir, addon-dirname ::sp/dirname]