Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ":base" and ":merge-with" support for resources #95

Closed

Conversation

@sritchie
Copy link
Contributor

sritchie commented Dec 27, 2013

What?

This pull request lets liberator users define maps of "base" resource behaviors; other resources can inherit from these and share keys. For example:

(def base-resource
  {:handle-not-acceptable "That just wasn't acceptable!"
   :handle-created "You created something."})

(defresource my-resource
  :base base-resource
  :handle-ok "All's well.")

Expands out to this before compilation:

(defresource my-resource
  :handle-not-acceptable "That just wasn't acceptable!"
  :handle-created "You created something."
  :handle-ok "All's well.")

These base resources can stack nicely, as the tests show. The base maps will unfold from an arbitrary nesting depth.

You can also specify a :merge-with entry that shows how to combine clashes in keys. (by default, the new replaces the entry in the base.)

For example, the following resource will try to handle the not acceptable path, but delegate to its parent if the handler returns nil or false. This particular example only overrides a json response, but allows the parent to handle everything else (predefined HTML, for example.)

(def base-on-falsey
  (fn [l r] (some-fn r l)))

(defresource my-resource
  :base base-resource
  :merge-with {:handle-not-acceptable base-on-falsey}
  :handle-not-acceptable (media-typed
                          {"application/json"
                           {:success false
                            :message "No acceptable resource available"}})
  :handle-ok "All's well.")

(media-typed is another function I wrote, similar to by-method, that delegates on content type.)

Why?

I'm using this in the https://paddleguru.com codebase to define default :handle-not-acceptable and :handle-not-found returns:

(def guru-base
  "Base for all guru resources.

   Due to the way liberator's resources merge, these base definitions
   define a bunch of content types, even if the resources that inherit
   from them don't. The defaults are here to provide reasonable text
   error messages, instead of returning big slugs of html."
  (let [not-found (comp rep/ring-response
                        (route/not-found shared/status-404-page))
        base {"text/html" not-found}]
    {:handle-not-acceptable
     (->> {"application/json" {:success false
                               :message "No acceptable resource available"}
           "text/plain" "No acceptable resource available."}
          (with-default "text/plain")
          (media-typed base))

     :handle-not-found
     (->> {"application/json" {:success false
                               :message "Resource not found."}
           "text/plain" "Resource not found."}
          (with-default "text/plain")
          (media-typed base))}))

The HUGE benefit I gained from this inheritance was the ability to declare protected or authenticated resources using @cemerick's Friend library. Friend controls the authorized? and handle-unauthorized decision points; if the user isn't logged in, Friend pulls the user out of the decision tree, attempts authentication, and then - this is the cool part - injects the user back in to the liberator tree at the next decision point.

(def friend-resource
  "Base resource that will handle authentication via friend's
  mechanisms. Provide an authorization function and you'll be good to
  go."
  {:base guru-base
   :handle-unauthorized
   (media-typed {"text/html" (fn [req]
                               (friend/unauthorized!
                                (-> req :resource :allowed?)
                                req))
                 "application/json"
                 {:success false
                  :message "Not authorized, visit /login"}
                 :default (constantly "Not authorized.")})})

That call to friend/unauthorized! signals Friend to pick up at the allowed? decision point once the user authenticates, only if authorizations passes.

Hope you guys find this useful! I've included midje tests for my merging, but wasn't sure how to convert the above examples into tests in your current framework.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 3, 2014

Ping! Any thoughts on this?

@ordnungswidrig

This comment has been minimized.

Copy link
Member

ordnungswidrig commented Jan 3, 2014

Hi Sam,

I'm sorry, that I could not answer earlier -- my family had a new child process spawn on Dec 26th :-)

The patch looks solid and like a valuable contribution. I looked at the source but I had not the time to actually try this out. Unfortunately, when I looked at the whole picture, I got the impression that it does too much. The merge-with functionally is sophisticated by also adds a lot of logic to the "simple" definition of a resource. What does this gain when compared to enabling the function resource to accept a map of definitions?

Something like this:

(def base {:handle-not-acceptable "go-away" 
           :available-media-types ["text/html" "application/json"]})

(def foo (resource (merge base { :handle-ok "this-is-foo"})))
(def bar (resource (merge base { :handle-ok "this-is-bar"})))

(Currently you'd need to do (apply resource (apply concat (merge base {...}))))

This approach is more explicit than having a :base key but you can use whatever expression you want to build the resource definition map.

Don't get me wrong: I see value in your proposed patch, I just want to keep liberator as minimalistic as possible.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 3, 2014

That's totally a fine solution; the complexity of mine came in because I still wanted to support the "defresource" binding form. The simplest way to integrate the resource merging with the def form seemed to be to add an extra key.

@ordnungswidrig

This comment has been minimized.

Copy link
Member

ordnungswidrig commented Jan 3, 2014

When the defresource macro is changed to accept a single map as well would this then be sufficient?

(defresource foo 
  (assoc base
    :handle-ok "this is foo"))
@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 3, 2014

Yup, if you made that API change, that would definitely work. Looks good to me!

@leonardoborges

This comment has been minimized.

Copy link

leonardoborges commented Jan 20, 2014

+1

I've looking into writing my own macro to do this with the current version of liberator so I'd jump all over this release if this makes it in.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 20, 2014

@ordnungswidrig, do you really think the semantics of my update are that complicated? I like your solution, but I'm a a little worried that we haven't seen a release of liberator in two months - I'm still waiting on a new release with bugfixes that you guys have merged. Maybe the feature that's coded and tested is better than a slightly different (but functionally equivalent) version?

@ordnungswidrig

This comment has been minimized.

Copy link
Member

ordnungswidrig commented Jan 20, 2014

Sam, don't be worried, I'll prepare a release soon. I only think that the implementation is a little complicated, not in a bad way, it's more "much code". I agree on you, and the feature looks reasonable. I will merge the request in the next days. With your great blog post on integrating liberator and friend you created some kind of zugzwang for the project :-)

Would you mind to update the doc site after the patch? I have only very limited time to spend on liberator, these days and this would be a great help then.

One change that I'd like to see, can you rename :merge-with to :base-merge-with, so one can see, that these keyword belong together?

@ghost ghost assigned ordnungswidrig Jan 20, 2014
@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 20, 2014

Haha, I didn't mean to do that - I think your solution is excellent too, and would provide the same functionality. If you want to go that route I'm happy to update the post as well.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 20, 2014

Updated the pull req with :base-merge-with. Happy to help with the doc site, if you give me some guidance as to where you want the updates, where it is and how to iterate on the site.

It'd be great to hear an update on the project status - is anyone working on new stuff, or is liberator on hold for the moment?

Thanks, @ordnungswidrig! Happy to help wherever, I think the lib is great.

@ordnungswidrig

This comment has been minimized.

Copy link
Member

ordnungswidrig commented Jan 20, 2014

Sam, you can update the gh-pages branch in your forked repository and send a pull request for the updates, too.

Liberator is not on hold. I am working on the exception / error handling at the moment, which will address #96, #94 et. al.

I'm glad for every contributor at the moment. So please keep up contributing.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Jan 20, 2014

Sounds great to me, just wanted to check in. I'll add the doc updates to my list, let me see if I can get to those soon.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Feb 6, 2014

Ugh, haven't had time for this... can we merge the feature so I don't block a release, and then get the documentation in?

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Feb 6, 2014

Also, is there any way you could do a release with the bugfixes we pushed a while ago?

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Feb 22, 2014

@ordnungswidrig

This comment has been minimized.

Copy link
Member

ordnungswidrig commented Feb 23, 2014

Hi, #97 was merged which implements an optional parameter which can hold a map with default values.

@sritchie

This comment has been minimized.

Copy link
Contributor Author

sritchie commented Feb 23, 2014

Awesome, works for me. Any chance of a release?

@sritchie sritchie deleted the sritchie:feature/base_resource branch Feb 23, 2014
@ordnungswidrig

This comment has been minimized.

Copy link
Member

ordnungswidrig commented Feb 23, 2014

Sam, I plan to release today evening. Documentation must be updated to match the new features...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.