Clojure code for generating HAL JSON responses

Question

I wanted to use hal in one of my personal Clojure projects, but unfortunately the only library I could find (halresource) was somewhat out of date when compared to the current specification. So, I decided to roll my own, which works fine for me personally, but I'd like to make it publicly available. However, I don't have a good grasp on whether the code is good and the functions are straightforward and clear to others - hence this posting.

The code uses maps as representations of hal resources, and since json maps very well to Clojure, the maps can be directly serialized to JSON with Cheshire. The intended usage would be something like:

(-> (new-resource "somewhere.com")
    (add-property :king "The King Of Somewhere")
    (add-properties {:country "The Kingdom Of Somewhere" 
                     :city "Some City"})
    (add-link "embassy" "/embassy")
    (add-embedded-resources :adjacent_countries
                            (new-resource "nowhere.com")
                            (-> (new-resource "azurecity.com")
                                (add-property :ruler "Lord Shojo"))))

which would produce:

{
  "_embedded": {
    "adjacent_countries": [
      {
        "_links": {
          "self": {
            "href": "nowhere.com"
          }
        }
      },
      {
        "ruler": "Lord Shojo",
        "_links": {
          "self": {
            "href": "azurecity.com"
          }
        }
      }
    ]
  },
  "city": "Some City",
  "country": "The Kingdom Of Somewhere",
  "king": "The King Of Somewhere",
  "_links": {
    "embassy": {
      "href": "\/embassy"
    },
    "self": {
      "href": "somewhere.com"
    }
  }
}

Aside from general advice on best practices for writing Clojure code, I'd like to know:

• Is my usage of {:pre} sane and not abusive? Is there a better way to ensure that the input into my functions conforms to the hal specification without using defrecord or some other custom non-map data structure? Or, should I just go ahead and use defrecord, build a custom data types, and require you pass those in? Or, is it more idiomatic to just trust whatever input comes in and pass it through?
• Are the inputs to the functions well-chosen? For example, the add-properties function can be used with a map of key/value pairs, a collection of the form (:key value), or any number of inputs in the same form as map - for example, (add-properties res :a "b" :c "d"). Are there better function signatures I could expose?
• Are the docstrings helpful/clear?
• Does anything in here make you very confused and/or horrified?

I'm really liking Clojure, but I'm not entirely sure of how idiomatic/clear my code is, so any input you can give would be much appreciated.

(ns clj-hal.core
  (:require [cheshire.core :as json]))

(defn templated-href-valid?
  "Ensures that a link is minimally templated; full documentation is here:
  http://tools.ietf.org/html/rfc6570"
  [link]
  (if (:templated (second (first link)))
      (re-find #"\{.+\}" (:href (second (first link))))
      true))

(def link-properties
  #{:href :templated :type :deprecation :name :profile :title :hreflang})
(def reserved-resource-properties
  #{:_links :_embedded})

(defn is-resource
  [resource]
  (-> resource :_links :self :href))

(defn new-resource
  "Creates a new resource. Resources are maps with reserved keys _links and
  _embedded, as well as a mandatory :self link."
  [self]
  {:pre [self
         (not (empty? self))]}
  {:_links {:self {:href self}}})

(defn new-link
  "Creates a new link. Links are maps of the form {rel href & properties}.
  Properties are keyword/value pairs. If the :templated property is true, 
  the href must be minimally templated."
  [rel href & properties]
  {:post [(not= (keyword (ffirst %)) :curies)
          (every? link-properties (keys (second (first %))))
          (templated-href-valid? %)]}
  {(keyword rel) (apply hash-map :href href properties)})

;;; Creates a new curie
(defn new-curie 
  "Creates a new curie. Curies are a special form of links of the form
  {:curies {:name name :href href :templated true & properties}}.
  The properties :templated or :rel are fixed, and cannot be set."
  [name-value href & properties]
  {:pre [(not-any? #(= :rel (keyword %)) (take-nth 2 properties))
         (not-any? #(= :templated (keyword %)) (take-nth 2 properties))]
   :post [(every? link-properties (keys (second (first %))))
          (templated-href-valid? %)]}
  {:curies (apply hash-map :name (name name-value)
                           :href href 
                           :templated true 
                           properties)})

(defn add-link
  "Adds a new link, optionally creating. If there already exists in links the
  specified rel, it will turn it into a multi-link and add the new link.
  Attempting to add a curie will cause an error."
  ([resource link]
    {:pre [(not= :curies (keyword (ffirst link)))
           (every? link-properties (keys (second (first link))))
           (templated-href-valid? link)]}
    (update-in resource 
               [:_links (ffirst link)]
               #(let [contents (second (first link))]
                 (cond
                   (nil? %) contents
                   (map? %) (conj [] % contents)
                   :else (conj % contents)))))
  ([resource rel href & properties] 
    (add-link resource (apply new-link rel href properties))))

;;; Takes multiple links
(defn add-links
  "Adds a variable number of links to the resource, merging links into rel. 
  Attempting to add a link with rel=\"curies\" will cause an error."
  [resource & links]
  (if (and (= 1 (count links)) (not (map? (first links))))
      (reduce add-link resource (first links))
      (reduce add-link resource links)))

(defn add-curie
  "Creates and adds a new curie. Attempting to add a curie whose name already
  exists will cause an error."
  ([resource curie]
    {:pre [(= (ffirst curie) :curies)
           (every? link-properties (keys (second (first curie))))
           (:templated (second (first curie)))
           (templated-href-valid? curie)
           (not-any? #(= (:name (second (first curie))) (:name %))
                     (-> resource :_links :curies))]}
    (update-in resource 
               [:_links :curies] 
               #((fnil conj []) % (second (first curie)))))
  ([resource name href & properties]
    (add-curie resource (apply new-curie name href properties))))

(defn add-curies
  "Adds multiple curies. Attempting to add a curie whose name already exists
  will cause an error."
  [resource & curies]
  (if (and (= 1 (count curies)) (not (map? (first curies))))
      (reduce add-curie resource (first curies))
      (reduce add-curie resource curies)))

(defn add-property 
  "Adds a single property to the resource. If there already exists a property
  with name, will overwrite the existing property. Attempting to add the
  properties _links or _embedded will cause an error."
  ([resource [name value]]
    (add-property resource name value))
  ([resource name value]
    {:pre [(keyword name) 
           (not ((keyword name) reserved-resource-properties))]}
    (conj resource [(keyword name) value])))

(defn add-properties 
  "Adds multiple properties to the resource. Can take a map of properties, a 
  collection, or a variable number of :key :value parameters. Existing 
  properties sharing names with the new properties will be overwritten. 
  Attempting to add the properties _links or _embedded will cause an error."
  [resource & properties]
  (if (= 1 (count properties))
      (let [props (first properties)
            pairs (if (map? props) props (partition 2 props))]
        (reduce add-property resource pairs))
      (add-properties resource properties)))

(defn add-embedded-resource
  "Adds a single embedded resource mapped to the given rel in _embedded. If
  there already exists one or more embedded resources mapped to rel, converts
  the rel to a group of embedded resources and appends to it."
  [resource rel embedded-resource]
  {:pre [(keyword rel)
         (is-resource resource)
         (is-resource embedded-resource)]}
  (update-in resource 
             [:_embedded (keyword rel)] 
             #(cond
               (not %) embedded-resource
               (map? %) (conj [] % embedded-resource)
               :else (conj % embedded-resource))))

(defn add-embedded-resources 
  "Adds multiple embedded resources as members of a resource group mapped to
  rel. If there already exist resources mapped to rel, adds the new resources
  to the existing resource group. Takes the resources as:
    * Single resource
    * Collection of resources
    * Variable-number arguments"
  ([resource rel embedded-resources]
    {:pre [embedded-resources]}
    (reduce #(add-embedded-resource %1 rel %2)
            resource 
            (if (map? embedded-resources)
                [embedded-resources]
                embedded-resources)))
  ([resource rel embedded & more-embedded]
    (add-embedded-resources resource rel (cons embedded more-embedded))))

(defn to-json [resource]
  "Serializes to a json string using Cheshire."
  (json/generate-string resource))

Answer 1

First of all, great idea -- you should consider contributing to the halresource library to bring it up to date!

Here are my critiques:

Structure of data

The structure you have chosen to represent links, while truer to their HAL representations, leads to unnecessarily complicated code. What you have is something like this:

{"embassy" {:href "/embassy" :templated true}}

The halresource version of this, which I think is better because it's simpler, is this:

{:rel "embassy" :href "/embassy" :templated true}

The difference might look negligible, but the halresource version leads to simpler code because you're not having to deal with nested map structures. You can easily grab any value from a one-level map by its keyword. Say you want to grab the href field of the link above -- for the second example it's (:href link), but for the first example you have to do something like (get-in link ["embassy" :href]) or (:href (get link "embassy")). It's also confusing because you should generally avoid using strings as keys in a map -- prefer keywords instead.

I think I understand why you have opted for the first method -- it looks more like HAL's representation of links. But the key thing here is that you can transform your data structures however you'd like, once it comes time to translate your Clojure data structure to HAL format; until then, you're doing a lot of work on the data while it's still in the form of Clojure data structures, so I think it makes more sense to have it in a simpler, more intuitive form.

Accessing map fields

While I'm on the topic of maps, I need to point out that you are using them incorrectly. You're trying to treat maps as sequential structures, which they are not. A map is an unsorted (i.e. non-sequential) data structure; it doesn't make sense to grab the "first" or "second" item of a map, as a map does not have any particular order. Clojure won't throw an error when you treat maps this way, but the results that you get will be unreliable. Instead, you need to grab items from a map by key. For example, to get the href value of link, use (:href link) or (link :href) (in Clojure, you can conveniently use a map as a function to look up its argument, or a keyword as a function to look itself up in its argument).

Pre-conditions

I think that using pre-/post-conditions is a good idea here, at least for your add-* functions. I like pre-/post-conditions because they tend to make code more concise and they make it extra clear in reading the code what kind of arguments are supposed to be passed into each function, so they also serve the purpose of self-documenting code.

That being said, I think you could benefit from modifying your approach to validating links slightly. This goes hand-in-hand with having a simpler representation of links as ordinary maps, as I just noted above. The way you have it now, you're validating links via specific pre-conditions attached to the new-link function. The problem with this approach is that it assumes new-link is used to create each and every link! If the user decides to create his own link manually via a map literal, then there is nothing in place to validate that link.

I think a better solution would be to get rid of your new-link function altogether and let links be represented as ordinary, one-level maps. Validation should be done as a part of the add-link function, which ought to validate links before it adds them -- you can of course do this with a pre-condition. I would also simplify your pre-condition down to a single helper function, valid-link?, and do something like this:

(defn minimally-templated?
  "Checks whether a given href is minimally templated; full documentation is here:
   http://tools.ietf.org/html/rfc6570"
  [href]
  (re-find #"\{.+\}" href))

(defn valid-link?
  "Checks that a given link is valid. Links are represented as maps, e.g.,

      {:rel 'embassy', :href '/embassy', :templated true}

   See the 'link-properties' set above for other possible properties.

   If the :templated property is true, the href must be minimally templated."
  [link]
  (and
    (every? link-properties (keys link)) 
    (or 
      (not (:templated link))
      (minimally-templated? (:href link)))))

; (Note: you don't actually have to check whether the link has a `:curies` property,      
; because `:curies` is not included in your set of acceptable `link-properties`.)

(defn add-link
  "Docstring foo bar baz & quux." 
  ([resource link]
    {:pre [(valid-link? link)]}
    (update-in resource [:_links] #((fnil conj []) % link)))
  ([resource rel href & properties]
    {:pre [(even? (count properties))]}
    (reduce add-link resource (partition 2 properties))))

(A note about the above code: in trying to figure out the best way to refactor your add-link function, I ended up settling on (update-in resource [:_links] #((fnil conj []) % link))), which is essentially identical to the code used in halresource. halresource's solution is clearly simpler and more elegant, but it works in a slightly different, more simplified way than your code. When halresource's add-link function adds a link to a resource, if there are no existing links, rather than updating the resource's _links to equal the new link itself, it conj's the link into an empty vector []. In other words, if a resource has a _links attribute, it is always going to be a vector, even if the vector only contains one link. This simplicity allowed the writer of the halresource library to avoid writing a cond statement like you had to do, making his code simpler and easier to read.)

You'll notice that I've greatly simplified your add-link function, which brings me to my next point...

Function inputs

Re: add-property/add-properties, add-link/add-links, and add-curie/add-curies: I think it's great that you have multiple ways that users can add stuff. Flexibility in the ways that you allow people to use your functions is generally a good thing, as it will make your library easier to use -- the user won't have to remember whether or not they can pass a map to your add-properties function, for example. If someone wants to, they can even use add-properties to add only one property, and I think that's great. The only issue I have is that your add-links, add-curies and add-properties are unnecessarily complicated. They ought to be a simple wrapper around (reduce add-whatever ... Furthermore, you could easily simplify these functions by pulling out the explicit checks like (if (= 1 (count ... and (map? ... and instead use pre-conditions and arity overloading to make your code easier to read. For example, your add-properties function could look something like this:

(defn add-properties 
  "Adds multiple properties to the resource. rest of doc string etc."
  ([resource properties-map]
    {:pre [(map? properties-map)]}
    (reduce add-property resource properties-map))
  ([resource & properties]
    {:pre [(even? (count properties))]}
    (reduce add-property resource (partition 2 properties))))

As you can see, this version of add-properties uses the built-in logic used by Clojure functions to determine what code executes, rather than explicitly checking it with an if statement. The user can pass in either a map or the property key/vals as separate arguments, and arity overloading ensures that the right code will execute for the right situation. If only one argument is given for the properties, then a simple pre-condition checks to make sure that that argument is a map. Also, notice the pre-condition that I added for the function that takes [resource & properties] as arguments -- it checks to make sure that there are an even number of arguments after the resource. With that out of the way, all the function has to do is reduce the function you've already written, add-property, over the properties being provided as arguments. You can and should do the same thing with your add-curies and add-links function. Just remember: the "singular" functions should be doing all the work; the "plural" functions are just there as a convenience to the users of this library, and they should basically just be reduce-ing the "singular" function over the resource and properties, and nothing else.

Docstrings

Are your docstrings helpful/clear? For the most part, yes. As someone with zero HAL experience, I could easily understand what your functions do just by reading the docstrings. I only have 2 minor critiques:

1. You should consider require-ing cheshire.core as just cheshire. That way it is clear wherever you use a JSON-related function in your code, without having to scroll all the way back up to the namespace declaration, what library you are using. Then your to-json function could read like this:

   (defn to-json [resource]
     "Serializes to a JSON string."
     (cheshire/generate-string resource))
   

   (The fact that you're using Cheshire to generate the string is now self-evident.)

2. In the docstring for new-link, it says, "Links are maps of the form {rel href & properties}. I find this a little confusing... are rel and href both keys in the map? Or are the values for rel and href the name of a key and its value, respectively? You might consider rephrasing this as "Links are represented as maps, containing the keys..."

In conclusion...

Does Clojure really need another HAL library?

Clearly your library does exactly what halresource aspires to do. To be honest, due to my lack of experience with HAL, I don't have a clue what exactly is out of date about halresource, but whatever it is, I don't think it's something so major that it warrants there being a completely new library to deal with HAL.

Why not take the work that halresource's author started and improve on it? I think this is a great opportunity for you to fork the halresource source on GitHub and change only the bits that need updating, according to current HAL specifications, then do a pull request, and I'm sure the author of halresource will incorporate your changes and give you credit for your work in updating the library. Not that writing your own library is a waste of time by any means, because clearly it's excellent practice, it was fun for me to review this code, and I hope you've learned a lot from the experience!