amethyst-framework / amethyst

Amethyst is a Rails inspired web-framework for Crystal language
https://github.com/amethyst-framework
MIT License
648 stars 46 forks source link

before_action not calling method #50

Open drujensen opened 8 years ago

drujensen commented 8 years ago

So, I'm probably doing something wrong, but I have a before_action :authorize in my controller and it doesn't seem to call the def authorize method.

I see the macros defined in the controllers for before_action. Any idea's?

bararchy commented 8 years ago

it works for me , make sure authorize is the last def in the class , its something to do with how the macro is working , if you still have issues I'll add a code sample from a working project

drujensen commented 8 years ago

Thanks for you quick response!

Here is my controller: https://github.com/drujensen/crystal-blog/blob/master/app/controllers/post_controller.cr

When I inspect the @callback_methods, it doesn't have any of the @actions I list. It has the @types instead. Not sure how this works but it seems like you would want to register the actions instead?

drujensen commented 8 years ago

I have a fix but I'm not happy with it. Basically, to register the action in the macro, you need the list of actions that the before filter applies too. Here is the fix in the Base::Controller.

  macro before_action(callback, *actions)
    {% for action in actions %}
      def _before_{{action.id}}_{{callback.id}}
      @before_callbacks["{{action.id}}"] = [] of (-> Bool) unless @before_callbacks["{{action.id}}"]?
      @before_callbacks["{{action.id}}"] << ->{{callback.id}}
      end
    {% end %}
  end

Before, this method was creating callbacks for the @types instead of the @actions.

Let me know if you have a better way of getting the list of actions. Can you grab them from the other macro called action?

bararchy commented 8 years ago

Well, macros aren't my strong side, I'll leave that to @Sdogruyol or @Codcore .

But, I know you have two options regarding before_action

# before_action :method_to_execute_before_actions, only: [:methods, :to, :use, :the, :before, :action]
before_action :authorized?, only: [:execute, :new, :notifications, :dash_board] 

Or

# This will apply "authorized?" to every action 
before_action :authorized?
drujensen commented 8 years ago

I verified that the only: option works. The before_action :authorized? doesn't work.

Specifically:

  macro before_action(callback, only=[] of Symbol)
    {% if only.empty? %}
      {% only = @type.methods.map(&.name.stringify) %}
    {% end %}

If you don't specify the only:, you can see that it tries to get the list for you. The problem is that it is creating the list from @types instead of @actions.

So the callbacks are indexed on any types that are defined in the controller, instead of the actions defined in the other macro called actions. Not sure how to get that list of actions from the other macro.

Is there a way to do this in Crystal?

sdogruyol commented 8 years ago

Hey @drujensen

This may help you understand the actions https://github.com/Codcore/amethyst/blob/master/src/amethyst/base/controller.cr#L54-L60

drujensen commented 8 years ago

Thanks @Sdogruyol . How do I get the list of actions in the other macro called before_action? Can one macro use the results of another?

sdogruyol commented 8 years ago

Well first you have to register it via actions macro and then i think you can access the actions via @actions

drujensen commented 8 years ago

unfortunately, @actions is not available in the scope of the macro.

sdogruyol commented 8 years ago

I see we need to tinker about macro loading order :(

sdogruyol commented 8 years ago

Also we need specs for before_action callbacks

drujensen commented 8 years ago

@Sdogruyol are you saying that it should be available in the macros? Let me try some things if thats true.

sdogruyol commented 8 years ago

Yeah.If you register it as an action first then it should be available.

drujensen commented 8 years ago

EDIT: Nope, false alarm. Its still not available even when I register the action before the before action

sdogruyol commented 8 years ago

We have to rethink about before_callback implementation and have some proper specs :+1:

drujensen commented 8 years ago

@Sdogruyol Ok. Thanks for your help. For now, I will use the only: option and pass in all the actions.

sdogruyol commented 8 years ago

Your welcome @drujensen i'll tackle this issue