duct-framework / core

The core library of the Duct framework
75 stars 21 forks source link

Issue with duct/include reader macro / config.edn #23

Open rune-brinckmeyer opened 5 years ago

rune-brinckmeyer commented 5 years ago

Just started fiddling with duct and integrant, so please bear with a beginner here. I have stumbled upon what I believe may be an issue related to the duct/include reader macro.

I created a standard duct project. It works as expected from the repl:

(dev)
=> :loaded
(prep)
=> :prepped

When I embed the same files in a docker container (clojure:openjdk-8) under /home/runner/backend and run the repl yields this unexpected result:

(dev)
=> :loaded
(prep)
Execution error (FileNotFoundException) at java.io.FileInputStream/open0 (FileInputStream.java:-2).
/home/runner/backend/dev (Is a directory)

This error eventually leads to this line in resources/backend/config.edn:

:duct.profile/dev #duct/include "dev"

Following the implementation of the duct/include reader macro reveals that duct/include processes argument "dev" through config-resource:

(defn- config-resource [path]
 (or (io/resource path)
   (io/resource (str path ".edn"))
   (io/resource (str path ".clj"))))

Investigating this from the repl in the working project I find that:

(io/resource "dev")
=> nil
(io/resource (str "dev" ".edn"))
=> #object[java.net.URL 0x54458573 "file:/home/runeb/backend/dev/resources/dev.edn"]

while inside the container the repl gives me:

(io/resource "dev")
=> #object[java.net.URL 0x7580ef78 "file:/home/runner/backend/dev"]
(io/resource (str "dev" ".edn"))
=> #object[java.net.URL 0x29040c80 "file:/home/runner/backend/dev/resources/dev.edn"]

clearly when in the docker container (io/resource path) maps to the dev directory in the project root which is not a slurp-able resource (slurped in duct/make-include):

(defn- make-include [readers]
  (fn [path]
    (let [opts {:readers (merge-default-readers readers)}]
      (some->> path config-resource slurp (ig/read-string opts)))))

One must conclude that the project root is somehow added to the resource search list inside the container. I sure did not put it there, and my Java-fu is not good enough to perceive how it got there, or if it is supposed to be there in the first place.

Fixing this is easy enough by being more precise with with the include statement in config.edn like this:

:duct.profile/dev     #duct/include "dev.edn"      ;; dev.edn more specific than dev

The questions that arise are:

  1. Is this fix the rigtht way to do it? Is there a reason for duct/include to refer to "dev" instead of "dev.edn" in the config.edn? (Since this is what is resolves to anyway?)
  2. Any clue if there is something inherently wrong with resource path? my project.clj mentions only :resource-paths ["resources" "target/resources"] :resource-paths ["dev/resources"] so I have no clue how the project root (containing the project.clj file) got into the mix..

Any enlightenment is highly appreciated :-)

weavejester commented 5 years ago

Thanks for finding and investigating this issue.

Are you running this via lein repl? I'm surprised that the docker container would add the current directory to what I assume is the system classpath. That sounds like it would have all manner of security concerns.

Could you provide the command you used to run the docker container?

rune-brinckmeyer commented 5 years ago

@weavejester Yes, its run through standard lein repl.

I have set it up through docker-compose, but the corresponding docker command would be

sudo docker run -it backend lein repl
weavejester commented 5 years ago

What image are you using? clojure:openjdk-8-lein?

rune-brinckmeyer commented 5 years ago

Yes. clojure:openjdk-8-lein-2.9.1 to be precise :-)

kwrooijen commented 4 years ago

Hi, I'd like to contribute to this discussion. I was planning on creating a new issue, but stumbled upon this one. I already wrote my issue offline so I'll post it here:

The duct/include reader accepts a string (for example "test"). The reader will then look for a file in resources named "test", "test.edn", and "test.clj", in that order. Currently the convention is to simply type the name of the file without extension (based on the duct-template). For example:

:duct.profile/dev   #duct/include "dev"
:duct.profile/local #duct/include "local"

When trying to add a test profile, I add it the same way. Test profile introduced in 0.8.0

:duct.profile/test #duct/include "test"

But instead of including the test.edn that I created, I include a muuntaja resource from my dependencies:

(clojure.java.io/resource "test")
#object[java.net.URL 0x14688c7 "jar:file:/Users/kevin/.m2/repository/metosin/muuntaja/0.6.3/muuntaja-0.6.3.jar!/test"]

This is really problematic for Ducts design / conventions. This will cause users to trip over this issue, or if you update a dependency you might break your system.

Does duct/include need to be able to include non edn content?

  1. We could make duct/include only include files with an edn extension.
  2. Another option is to include only absolute file names. This would mean we will have to change the template to:
:duct.profile/dev   #duct/include "dev.edn"
:duct.profile/local #duct/include "local.edn"

The second option would be a breaking change for core. However it does have my preference.