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

Already on GitHub? Sign in to your account

Switching from friend 0.2.0 to 0.2.1 in friend-ui breaks friend-ui #113

Closed
sveri opened this Issue Jun 18, 2014 · 8 comments

Comments

Projects
None yet
4 participants

sveri commented Jun 18, 2014

I am developing on friend-ui (https://github.com/sveri/friend-ui) and when I switch to the new version of friend, fire up my server, then my requests to the server return "null" and chrome shows this error message:

Resource interpreted as Document but transferred with MIME type application/json: "http://localhost:3000/".

There is no stacktrace and if someone has an idea how to provide one I would be happy.

You can find my route definitions here: https://github.com/sveri/friend-ui/blob/master/src/clj/de/sveri/friendui/routes/user.clj

User settings can be found here: https://github.com/sveri/friend-ui/blob/master/src/clj/de/sveri/friendui/models/user.clj

And this is how I use them in my application:

(defn add-secured-route [route]  (friend/authenticate route user/friend-settings))

(def app
    (app-handler
           ;; add your application routes here
           ;secured-routes
            [(add-secured-route user-routes)
;...adding more routes]
           ;; add custom middleware here
           :middleware [
                        middleware/template-error-page
                        ;middleware/force-response-evaluation
                        ;middleware/log-request
                        ]
           ;; add access rules here
           :access-rules []
           ;; serialize/deserialize the following data formats
           ;; available formats:
           ;; :json :json-kw :yaml :yaml-kw :edn :yaml-in-html
           :formats [:json-kw :edn]))

sveri commented Jun 25, 2014

It seems like there are two problems here. I am using a luminus generated website.

  1. I wrapped single route definitions into (friend/authenticate routes friend-settings). Which are then parsed by the luminus provided middleware wrappers.
    If I wrap the whole luminus app definition into friend/authenticate I can load my webpage. However, there seems to be a middleware missing in luminus.
  2. Missing middleware in luminus (could not find out which one) which causes POST requests to lack parameters. When I wrap all routes together into one routes defintion:
(defroutes allroutes
           (friend-routes (lweb-friend/FrienduiStorageImpl))
...
           app-routes)
(def app
  (compojure.handler/site
    (friend/authenticate
      allroutes
      friend-settings)))

And load them with compojure.handler/site I got it all working.

So from my point of view it can be closed, as I got it working. However, the fact that a simple minor version change can break existing usages of friend could make this worth to remain opened and fixed.

Owner

cemerick commented Jun 25, 2014

compojure.handler/site only adds a set of standard Ring middlewares, some of which Friend definitely requires (as noted in the README).

Perhaps you changed the version of Luminous you were using at the same time you changed Friend's version, and the former stopped including certain standard Ring middlewares?

sveri commented Jun 25, 2014

I missed that section about the required middleware. Thank you, I guess that will solve my problem number two.

But number one still remains (maybe I just used friend in the wrong way).
I set up a standard luminus webproject, added friend 0.2.0. Configured it as basic as possible and the webapp works.
Now, when I switch to 0.2.1 (only change I made) it stops working.

You can clone this webapp here: https://github.com/sveri/friendversion. There is nothing new inside it besides friend and all the stuff happens in handler.clj (besides the version change -> project.clj).

Contributor

u-phoria commented Jul 6, 2014

I have what looks like a related issue in my web app, and one also exhibited by the 'clean' friendversion project shared by @sveri above.

Basically switching from 0.2.0 to 0.2.1 broke serving of static files (css / js in the example).

git bisect suggests this commit introduced the relevant change in behaviour: 41a99e5

Specifically the update-in on line 251.

What happens there is when response is empty, which seems to happen for static files etc, update-in will now replace it with a map ({:friend/ensure-identity-request nil}), this then affects downstream behaviour.

Putting a (when response ...) around the update-in restores previous behaviour for affected files.

Hope this makes sense?

katox commented Aug 4, 2014

I have been bitten by the same incompatibility. The problem is that compojure.core/routes macro tries to call handlers one by one until some of them returns a truthy value.

The change introduced in 0.2.1 changes nil value wrapped by friend/authenticate from nil to {}. That means that no other routes are tested for a match.

The extra (when response) clause as suggested above would fix the issue.

Contributor

u-phoria commented Aug 5, 2014

Happy to send a pull request later, or open to better suggestions (we prob don't want conditionals starting to spring up in multiple places to work around the behaviour change introduced in 0.2.1?)

@u-phoria u-phoria added a commit to u-phoria/friend that referenced this issue Aug 5, 2014

@u-phoria u-phoria Fix for issue #113 - stop update-in response from returning {}, being…
… truthy this stops compojure.core/routes from testing other routes for a match and breaks serving of static files
90f45f4

@u-phoria u-phoria added a commit to u-phoria/friend that referenced this issue Aug 5, 2014

@u-phoria u-phoria Fixes #113 - stop update-in response from returning {}, being truthy …
…this stops compojure.core/routes from testing other routes for a match and breaks serving of static files
664675b

@u-phoria u-phoria added a commit to u-phoria/friend that referenced this issue Aug 5, 2014

@u-phoria u-phoria Do not return {} (truthy) since this stops other routes from being ev…
…aluated (fixes #113)
0534c8c
Owner

cemerick commented Sep 9, 2014

Yes, please submit a PR.

Contributor

u-phoria commented Sep 9, 2014

Thanks - covered by PR #115

@cemerick cemerick closed this in bad7825 Mar 13, 2015

@cemerick cemerick added a commit that referenced this issue Mar 13, 2015

@cemerick cemerick Merge pull request #115 from u-phoria/issue-113
Fix for #113 - avoid returning {} since it stops other routes from being evaluated
e5105b5

@cemerick cemerick added this to the 0.2.2 milestone Mar 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment