Allow multiple pipelines to be added with one block

[marksiemers]

Description of the Change

This allows doing the following in a routes file:

  pipeline :web, :auth do # This change allows multiple pipelines to be setup at once
    plug Amber::Pipe::Error.new
    ...
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    plug Authenticate.new
  end

Alternate Designs

@eliasjpr suggested allowing this syntax:

 pipeline :web do
    plug Amber::Pipe::Error.new
    ...
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    use :web    
    plug Authenticate.new
  end

The two approaches are not mutually exclusive - implementing this PR doesn't prevent implementing the use or another keyword approach.

Comparing the two - this PR provides a clean way to setup two pipelines at the same time.

The use approach allows you to create an additional pipeline "around" and existing one. If that is needed in the future, the alternate design will also need to be implemented.

Benefits

Keeps route code dry when different pipelines share a lot of the same pipes.

Possible Drawbacks

None that I'm aware of.

[marksiemers]

@amberframework/core-team

[eliasjpr]

I'm good with this. Can be merge

[veelenga]

I guess this should be

plugs.first?.should be_a Pipe::Logger
plugs[1]?.should be_a Pipe::Error

.first and .[] may raise an error instead of expectation failure.

Dismissing to request another spec use case

[eliasjpr]

Was looking at the specs in more detail and noticed that it was missing the use case that prompted this changed in the specs. What do you think if we add a spec for the following case?

‘’’crystal
Amber::Server.configure do |app|
pipeline :custom, :web do
plug Pipe::Logger.new
plug Pipe::Error.new
end

pipeline :web do
plug Auth.new
end
end
‘’’

[elorest]

I like what @eliasjpr suggested.

 pipeline :web do
    plug Amber::Pipe::Error.new
    ...
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    use :web    
    plug Authenticate.new
  end

as it allows for one pipeline to inherit from another.

I'm not quite sure what the use case of defining two pipelines with the exact same handlers in them would be? Could you enlighten me on that? @marksiemers

[marksiemers]

@elorest
This:

  pipeline :web, :auth do
    plug Amber::Pipe::Error.new
    ...
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    plug Authenticate.new
  end

and this:

 pipeline :web do
    plug Amber::Pipe::Error.new
    ...
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    use :web    
    plug Authenticate.new
  end

Are functionally the same. The auth pipeline has all the same pipes as web - plus Authenticate.new

It may not be as memory efficient, as I think the approach in this PR will double the instances of the shared pipes.

Our small sample size of 2 people on gitter seemed to prefer the syntax of this approach (allowing two pipes setup at the same time).

[elorest]

I just seems like a really unorthodox inheritance pattern to me. Typically you create A and then inherit from it when creating B. I've never seen the pattern of creating A and B then adding to B.

Similar to this in Ruby, Crystal

class A, B
  def hello
    puts "hello world"
  end
end

class B
  def hello_name(name)
    puts "hello #{name}."
  end
end

[marksiemers]

I wasn't thinking of it as inheritance - there is no class being defined or instantiated for web or auth - they are both pipelines, which is more of a functional programming concept.

[marksiemers]

Hmm, I uncovered something, not sure if it is a bug or desired behavior, put if you call pipeline twice with the same name, it will overwrite the pipeline rather than append - at least based on my testing.

Would we rather have it overwrite or append?

This PR will not work if we stick with overwriting (not for the use case specified).

[marksiemers]

This fixes what I think is a bug. Hash#key? expects a value as an argument, so this has been re-initializing the @pipeline[valve] every time. Essentially over-writing any existing pipes.

[eliasjpr]

I incorporated the same change in another branch! Nice Catch

[elorest]

We didn't assume anyone would have a reason to define the same pipeline twice, so this was intentional. I still can't think of why we should expect a pipeline to be defined twice.

[elorest]

@marksiemers to me it seems like a similar thing to inheriting blocks of yaml into new yaml blocks.
https://crystal-lang.org/api/0.21.0/toplevel.html#record%28name%2C%2Aproperties%29-macro

Whether we think of it as inheritance or merging of hashes/arrays it still seems strange to create 2 at once and then add to one later instead of merging an existing one into a new one. Is this standard somewhere else? I don't recall phoenix allowing this for pipelines but I might not have dug in far enough.

To me @eliasjpr method of using use :default to merge an existing pipeline into a new one makes a lot more sense. Also with that method it's fine if the same pipeline can't be defined twice.

WDYT?

[eliasjpr]

The way I visualized the use keyword to be implemented to act as some sort of flag/check in the pipeline. Lets say that the pipeline linked list finds a use link, it diverts to run the pipeline specified by use and then continue the original link list avoiding to create x number of arrays holding possibly duplicated plugs

-->error -->looger-->use :web (runs web pipeliine )-->other handler

This way use can be define in any order

pipeline :api do
  use :common
  plug SomePlug
  use :web
  plug Auth
end

WDYGT?

[elorest]

@Eliasjp I really like that.

…

On Nov 5, 2017 7:17 PM, "Elias Perez" ***@***.***> wrote: The way I visualized the use keyword to be implement is some sort of flag/check where is define in the pipeline. So for say that the pipeline linked list finds a use link, it diverts to run the pipeline the specified by use and then continue the original link list avoiding to define the pipeline alltogether -->error -->looger-->use :web (runs web pipeliine )-->other handler This way use can be define in any order pipeline :api do use :common plug SomePlug use :web plug Authend WDYGT? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#359 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHmpF_v3JaXcSNjmBxdkxaQUl7hSODtks5szmw_gaJpZM4QSPnK> .

[marksiemers]

A few things in this comment:

1. A full example of what brought this PR up
2. Thoughts about this approach vs a use approach
3. Implement both?

Full Example

A user was creating a "reddit clone" so for the PostController, the index and show actions were to be public, all others required a user to be signed in (authenticated).

With the current version of crystal, 5 plugs had to be duplicated for :web and :auth:

Amber::Server.configure do |app|
  pipeline :web do
    plug Amber::Pipe::Error.new
    plug Amber::Pipe::Logger.new
    plug Amber::Pipe::Session.new
    plug Amber::Pipe::Flash.new
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    plug Amber::Pipe::Error.new
    plug Amber::Pipe::Logger.new
    plug Amber::Pipe::Session.new
    plug Amber::Pipe::Flash.new
    plug Amber::Pipe::CSRF.new
    plug Authenticate.new
  end

  # All static content will run these transformations
  pipeline :static do
    plug Amber::Pipe::Error.new
    plug HTTP::StaticFileHandler.new("./public")
    plug HTTP::CompressHandler.new
  end

  routes :static do
    # Each route is defined as follow
    # verb resource : String, controller : Symbol, action : Symbol
    get "/*", Amber::Controller::Static, :index
  end

  routes :web do
    resources "/posts", PostController, only: [:index, :show]
    get "/signin", SessionController, :new
    post "/session", SessionController, :create
    get "/signout", SessionController, :delete
    get "/signup", RegistrationController, :new
    post "/registration", RegistrationController, :create
    get "/", PostController, :index
  end

  routes :auth do
    resources "/posts", PostController, except: [:index, :show]
  end
end

This seemed like a clean syntax for DRY'ing up the code, and one that would be approachable

...
  pipeline :web, :auth do
    plug Amber::Pipe::Error.new
    plug Amber::Pipe::Logger.new
    plug Amber::Pipe::Session.new
    plug Amber::Pipe::Flash.new
    plug Amber::Pipe::CSRF.new
  end

  pipeline :auth do
    plug Authenticate.new
  end
...

The place I've seen this style of syntax before is with ActiveRecord validations - if you have the same validation for more than one field, you can "share" that validation:

validates :name, :email, presence: true
validates :email, uniqueness: true

See: http://guides.rubyonrails.org/active_record_validations.html#presence

ActiveRecord also has the pattern with a block for validates_each
See: http://guides.rubyonrails.org/active_record_validations.html#validates-each

This approach vs. a use approach

The use approach is more powerful, I think it is a good idea and it should be implemented.

It does require that users learn a new keyword and how to use it (pun unavoidable), and it will take some time for us to build out.

This approach (allowing multiple args) I think is intuitive coming from ruby, or even using variable arguments in Crystal.

I read it as "For the web and auth pipeline, plug these pipes. Also for auth pipeline plug this pipe"

Lastly, this approach is done and tested.

Implement Both?

I vote that we implement both.

The two approaches are not mutually exclusive and both are backward compatible changes. They can both be used in the same app.

I don't see the downside to this approach. Unless there are some consequences to having double the instances of the pipes.

This is a separate conversation, but should the pipes be modules rather than classes? Most of them don't need state, and the ones that do, it looks like it is IO - which could be a server setting.

[eliasjpr]

I am wondering if there is a way to keep 1 copy of the handlers, and use something like union at the time of processing. I am happy with this first implementation since it is simple and achieves the desired expectation, but would like to open a ticket to mark it as something that we can maybe improve in the future.

common_handlers[:web,:api] = [ ...array of common handlers... ]
pipelines[:api] = [ Authenticator.new ]
common_handlers[:api] | pipelines[:api]

https://crystal-lang.org/api/0.23.1/Array.html#%7C%28other%3AArray%28U%29%29forallU-instance-method

[marksiemers]

@elorest @eliasjpr
If we are worries about duplicating pipes, a line like the one below could make sure the pipes are not duplicated and stored while added them to a pipeline:

(pipelines.values.flatten.find{|p| p.class == pipe.class } || pipe)

See full example here: https://play.crystal-lang.org/#/r/31pq

In the case of this PR though, it would probably be better to take care of it in the macro, which now that I think about it, should be possible.

[elorest]

Personally I think this syntax is a bit weird, and would rather wait until we actually have more that 5 pipes that end up duplicated across pipelines before trying to find a solution here. Active Record validations don't seem like a parallel to this to me, but I can see the similarities.

I really liked @eliasjpr use syntax, but if he's fine with this I'll approve it too.

If I stare at it for a long time it sort of grows on me.

👍

[marksiemers]

I agree it is sub-optimal. For now, I think the pros (DRY route file, decent tests added for pipelines) outweigh the cons (questionable design pattern, duplicate instances of pipes when used).

[marksiemers]

@amberframework/core-team - Some changes based on the commits from master that were merged in. Please re-approve.

[eliasjpr]

👍 Feel free to merge this