Catalyst Actions, Part 2 (Details and Clarification)

Last night (NYC time at least) I was able to have a great brainstorming conversation with rafl, confound and others regarding new ways to declare Catalyst Actions.  This conversation was set off by a blog post and my response.

A few things came up in the conversation that warranted me stepping back and pondering the best way to clarify my thoughts and my position.  The first has to do with my feelings about how controllers and actions should require minimum (or ideally no) additional syntax knowledge, above what MooseX::Declare and Moose expects, in order for a developer to be productive.  This is what I meant by 'Controller Baby Talk', and I think it's important since we have a goal of making it easy to get started and perform the common cases.  It's related to the reason why I think query and body parameters should be expressed in the method signature as well. The idea is less method calls, more declaration and more useful defaults. Someone just starting should be able to make decent controllers off the top with a minimum of learning.  They might not make great controllers, but they will make workable controllers.

The second issue has to done with Catalyst Chained Actions which I refer you to the link if you are not familiar.  Chained actions are great in that they help reduce a lot of repeated code, as well as having a very elegant feel to them.  However, in practical life I've not found using them to be very beginner friendly.  I've even seen intelligent, intermediate and above level developers struggle with them.  I hesitate to criticise here, since I'm not contributing code to try and solve these problems (yet) but in general my experience with Chained actions leads me to conclude that they belong in the hands of at least intermediate or higher programmers, people with the experience to realize they need more and the ability to go learn how to do it correctly.  I realize you may feel differently, so please aim your thoughts into comments.  I'm saying this based on my experiences as a team lead building a large Catalyst driven website where I spent too much time struggling to help other developers get their chained actions working correctly, as well as debugging trouble with using $c->uri_for against action chains.

Based on this, I left chained actions out of my discussion.  I do think whatever we create to replace the current method attribute system must address the need to make Chained actions work and work better, I just think it's not a button I want newcomers to the framework to have access to on day one.  That's why I originally left out any discussing of chained actions out of my blog post.

Based on the IRC chat and the feedback I've recieved, I think that you should be able to easily 'scale up' to chained actions as you need them.  In particular, I like the idea, mentioned a few times in last night's chat, that all actions are reall chained actions, just some are very short chains.  Under this thinking, everything is an action chain, but we have some shortcut types to enable some simple cases (such as what the 'Path' attribute is now used for).  These special cases are just shortcuts that behind the curtain uses chaining.  In this case we'd all be using chained actions all the time, but for the simple cases the work would be hidden and the end developer doesn't even know it, until they rise in skill and have that wonderful, "AH HA!" moment of realization!  I can see that could make our underlying code base more clean, as well as remove a lot of special case processing.  As a side effect, it would probably make action chains more robust, and probably solve some my concerns.  That of course would require a major rewrite of the dispatcher code, which is something for another blog.

However, I think we should be careful to move Catalyst controllers too far away from plain old Perl / Moose classes.  Although I don't love the method attribute syntax of the current system, one thing I do love is that Catalyst Controllers are just plain classes with a bit of decoration.  That makes it easy for me to apply all my knowledge regarding designing and developing classes to Catalyst Controllers.  It's something I would not want to see disappear.

Given this idea, I want to offer another thought experiment that would outline the growth from simple case controllers up to chained controllers, trying to follow the intelectual growth of a new developer.  Again, I want to keep the new syntax to a minimum.  Some of the syntax here comes out of the IRC conversation I've mentioned, but no code is written and many more use cases need to be written and expressed before we can clarify the specification.

Let's start with a cleaned up version of the controller I from my last post.  This is a controller that offfer URI endpoints to view a Gallery of People, see details on a particular person (by ID or Email) and create a new person.  Let's assume the Person entity has the following attributes: ID, Name, Birthdate, Email with ID and Email as unique and all fields are required.  Let's also for the moment assume that ID is auto generated by the storage system, such as a database.  I think a webpage like this is a pretty common ask for a developer, and many websites are just more complex versions of this simple case.

    use Catalyst::Declare::Controller;

    class People {
        method gallery($ctx: Int :$page = 1, Int :$rows = 1) {
            my $people_rs = $ctx->
                model('People')->
                search({},{page=>$page, rows=>$rows});
            $ctx->stash(
                people_rs = $people_rs,
            );
        }
        multi method person($ctx: Int $id) {
            if(my $person = $ctx->model('People')->find(id=>$id)) {
                $ctx->stash(person=>$person);
            } else {
                $ctx->forward('not_found', ["Person of id: $id doesn't exist"])
            }
        }
        multi method person($ctx: Email $email) {
            if(my $person = $ctx->model('People')->find(email=>$email)) {
                $ctx->stash(person=>$person);
            } else {
                $ctx->forward('not_found', ["Email: $email doesn't exist"])
            }       
        }
        method create($ctx: Dict[name=>Str, email=>Email, birthdate=>DateTime] $post) {
            my $validated_post = $ctx->model('ValidatePersonCreatePost')->validate($post);
            if($validated_post->is_valid) {
                $ctx->model('People')->create($validated_post);
                $ctx->forward('CreatedNewPerson', ["I created person with $validated_post"]);
            } else {
                $ctx->stash(template=>'people/create_has_errors');
            }
        }
        method prepare_database(HashRef $opts) {
            ## just a plain old method, move along
        }   
    }

I added a bit more of the kind of logic you might expect to see in this kind of controller.  For this case, I assumed we are storing all our data in DBIC, so I used that syntax.  However, I want to point out it's not good practice to put this kind of logic in your controllers!  I'm just doing it this way to make it easier for you to follow along and I will offer one last version at the end which will reflect this best practice.  This will make it easier later on to see the value of chained actions as well.  This controller would map the following URLs:

{hostname}/people/gallery?page={page}&rows={rows}
{hostname}/people/person/{id}
{hostname}/people/person/{email}
{hostname}/people/create

Remember, we suggested in the previous blog that if the last argument in the signature is a reftype, such as something inheriting from ArrayRef, HashRef or even Object, we assume this action requires HTTP Post and attempts to coerce the post entity against the given type constraint.

For the purposes of clarity, I skipped all the MooseX::Types imports, just assume they are there and that useful coercions exists, such as something to parse strings into DateTime objects and something to take a urlencoded post entity and convert it to a HashRef.  For the purposes of making it easier for newcomers, we might even wish to consider automatically importing a bunch of the most common case MooseX::Types and coercions.

One thing that I changed from the last example is I've removed the 'action' keyword, relying exclusively on 'method' which we get from MooseX::Declare.  Basically what I am saying here is that an action is just a method that expects to be part of a context, so just requiring "$ctx:" in the method signature invocant should be enough to tell the underlying system this is an action, not an ordinary method.  I think this could be more intuitive to a newcomer, saying that an action is just a method with a context.  Okay, so that's not the entire story, but again, it's still a pretty big gulp for a new developer.  She can start with that and be very productive with just that amount of knowledge.

I also swapped 'controller' for 'class' which again comes straight out of the MooseX::Declare syntax.  Right now I am not seeing what calling this controller is buying us, except that it a new thing to learn.  Right now it feels gratuitiously different.

So right now you'd have a workable controller and you didn't have to learn anything above and beyond what Moose and MooseX::Declare expect.  I expect you are looking at this and thinking, "Hmm, if we had chained actions, we'd be able to reduce a lot of this repeated model calls."  Also, what happens when you need more complex controllers?  Let's say  the developer gets a request to add the ability to both edit and delete individual Persons within the set of People?  Again, this is a classic argument for Chained actions, since we can have much clearer code.  For the moment though, let's stay with the current knowledge set and see what our developer might write.

    use Catalyst::Declare::Controller;

    class People {
        method gallery($ctx: Int :$page = 1, Int :$rows = 1) {
            my $people_rs = $ctx->
                model('People')->
                search({},{page=>$page, rows=>$rows});
            $ctx->stash(
                people_rs = $people_rs,
            );
        }
        multi method person($ctx: Int $id) {
            if(my $person = $ctx->model('People')->find(id=>$id)) {
                $ctx->stash(person=>$person);
            } else {
                $ctx->forward('not_found', ["Person of id: $id doesn't exist"])
            }
        }
        multi method person($ctx: Email $email) {
            if(my $person = $ctx->model('People')->find(email=>$email)) {
                $ctx->stash(person=>$person);
            } else {
                $ctx->forward('not_found', ["Email: $email doesn't exist"])
            }       
        }
        method create($ctx: Dict[name=>Str, email=>Email, birthdate=>DateTime] $post) {
            my $validated_post = $ctx->model('ValidatePersonCreatePost')->validate($post);
            if($validated_post->is_valid) {
                $ctx->model('People')->create($validated_post);
                $ctx->forward('CreatedNewPerson', ["I created person with $validated_post"]);
            } else {
                $ctx->stash(template=>'people/create_has_errors');
            }
        }
        method edit_person($ctx: Int $id, Dict[name=>Str, email=>Email, birthdate=>DateTime] $post) {
            if(my $person = $ctx->model('People')->find(id=>$id)) {
                my $validated_post = $ctx->model('ValidatePersonEditPost')->validate($post);
                if($validated_post->is_valid) {
                    $ctx->model('People')->update($validated_post);
                    $ctx->forward('UpdatedPerson', ["I updated person $id with $validated_post"]);
                } else {
                    $ctx->stash(template=>'people/create_has_errors');
                }
            } else {
                $ctx->forward('not_found', ["Person $id doesn't exist"]);
            }
        }
        method delete($ctx: Dict[id=>Int]) {
            if(my $person = $ctx->model('People')->find(id=>$id)) {
                $person->delete;
                $ctx->forward('DeletedPerson', ["I deleted person $id"]);
            } else {
                $ctx->forward('not_found', ["Person $id doesn't exist"]);
            }
        }
        method prepare_database(HashRef $opts) {
            ## just a plain old method, move along
        }   
    }

This controller now maps the following URLs:

{hostname}/people/gallery?page={page}&rows={rows}
{hostname}/people/person/{id}
{hostname}/people/person/{email}
{hostname}/people/create   (expects POST entity)
{hostname}/people/edit_person/{id}  (expects POST entity)
{hostname}/people/delete {expects POST entity}

To make this a bit shorter, I said edits and deletes could only occur using a Person ID, even though I allow view access via both the ID and Email.  Now, these are not the most concise and beautiful controllers (there's a bit of repeated code), but they have the advantage of: 1) straightforward for newcomers, and 2) they remain basically plain old Perl / Moose.

At this point, we could offer a refactoring using standard Perl techniques, such as pulling out the repeated code blocks.  This would reflect a developers growing confidence and ability.  I will leave that as an exercise for the reader, since it's offtopic for the current thought experiment.  I only mention it because I want to point out that it would be possible for a developer to gradually increase their ability and get rewarded with better code along a more gradual learning curve.

At this point, now that our developer is starting to become more familiar and confident with the system (given the ease of success so far) she starts exploring the store of community best practices, trying to see what could be done to make these even better and easier.  She comes across chaining, and with some research and help managed to perform the following refactoring (I'm using a possible chained syntax which has not yet been fully research, but hopefully you can get the idea:

    use Catalyst::Declare::Controller;

    class People {

        Person->config(actions => {
            root => {
                name => '';
            },
        });

        method root($ctx:) {
            $ctx->stash(people_rs => $ctx->Model('People'));
        }

        under root($chained_ctx:) {
            my $people_rs = $chained_ctx->stash->{people_rs};
            method gallery($chained_ctx: Int :$page = 1, Int :$rows = 1) {
                   $chained_ctx->stash(
                    people_rs = $people_rs->search({},{page=>$page,rows=>$rows}),
                );
            }
            method create($chained_ctx: Dict[name=>Str, email=>Email, birthdate=>DateTime] $post) {
                my $validated_post = $chained_ctx->model('ValidatePersonCreatePost')->validate($post);
                if($validated_post->is_valid) {
                    $people_rs->create($validated_post);
                    $chained_ctx->forward('CreatedNewPerson', ["I created person with $validated_post"]);
                } else {
                    $chained_ctx->stash(template=>'people/create_has_errors');
                }
            }
            method delete($chained_ctx: Dict[id=>Int]) {
                if(my $person = $result_rs->find(id=>$id)) {
                    $person->delete;
                    $chained_ctx->forward('DeletedPerson', ["I deleted person $id"]);
                } else {
                    $chained_ctx->forward('not_found', ["Person $id doesn't exist"]);
                }
            }
               multi method person($chained_ctx: Int $id) {
                if(my $person = $people_rs->find(id=>$id)) {
                    $chained_ctx->stash(person=>$person);
                } else {
                    $chained_ctx->forward('not_found', ["Person of id: $id doesn't exist"])
                }
            }
            multi method person($chained_ctx: Email $email) {
                if(my $person = $people_rs->find(email=>$email)) {
                    $ctx->stash(person=>$person);
                } else {
                    $ctx->forward('not_found', ["Email: $email doesn't exist"])
                }       
            }
            under person($chained_ctx:) {
                my $person = $chained_ctx->stash->{person};
                method view ($chained_ctx:) {
                    ## just goes to a view template
                }
                method edit($chained_ctx: Int Dict[name=>Str, email=>Email, birthdate=>DateTime] $post) {
                    my $validated_post = $chained_ctx->model('ValidatePersonEditPost')->validate($post);
                    if($validated_post->is_valid) {
                        $person->update($validated_post);
                        $chained_ctx->forward('UpdatedPerson', ["I updated person $id with $validated_post"]);
                    } else {
                        $chained_ctx->stash(template=>'people/create_has_errors');
                    }
                }
            }               
        }
        method prepare_database(HashRef $opts) {
            ## just a plain old method, move along
        }
    }

These map the following URLS:

{hostname}/people/gallery?page={page}&rows={rows}
{hostname}/people/person/{id}/view
{hostname}/people/person/{email}/view
{hostname}/people/person/{id}/edit (expects POST entity)
{hostname}/people/person/{email}/edit (expects POST entity)
{hostname}/people/create   (expects POST entity)
{hostname}/people/edit_person/{id} 
{hostname}/people/delete {expects POST entity}

You probably noticed I changed a lot of the "$ctx:" invocants to "$chained_ctx:"  This is to reflect that the method is part of a chained action context, and different from 'normal' actions.  I think it's a good idea to express this difference clearly, since an action that is part of a Chain does have different characteristics than a stand alone action.  for example, you can't $ctx->forward to it.  I'm not attached to particular invocant name, but I'm just trying to indicate how this could be done without falling back on adding modifiers on the method (such as 'on', 'at' or others).

Overally, this refactor I think is an improvement, since it has less repeated code and the flow across the actions is more clear.  It will make it easier to do even more complicated things later. 

Well, that's mostly it for the thought experiment.  I think I got my basic idea across.

I said earlier in this post that these controller examples have a fatal flaw, which is the amount of logic that rightfully should stay in the model.  My suggestion, if you are using DBIC, is that you treat DBIC methods (search, find, and all that) as sort of private or protected to your result and resultset classes.  In other word, your schema should define an API to expose to the world, and the world should respect that or suffer.  Additionally it's probably suspect to pass a resultset or row object to your Template handler.  This can lead to all sorts of template abuse.  I promised a last refactor, but as I look into the eager eyes of my Akita dog, who is gazing expectly out the window onto a beautiful sunny day, I just know I'll leave that one to another blog :)

Share and Enjoy!

---UPDATES ---

It was pointed out to me that using the method signature invocant in the way I have described doesn't really reflect the true nature of the invocant.  As an alternative we could say that if a method signature has a first, positional parameter which is a Catalyst Context, then the method is an action.  Would look like:

    method myaction(Context $ctx, ....) {}

This would have the intriguing possibility of having subclasses on the Context subtype, allowing multi method polymorphism for the same URL with different information in the context.  This is an alternative to what was described above and may be a better option.

Additionally, with recent builds of MooseX::Declare, methods now support a version of a 'returns' modifier, which is a type constraint on the return value of the method.  Although this itself is not always useful for Catalyst actions, it might be valuable to place a modifier to check the contents of the Stash.  This would add a bit of sanity to the otherwise anarchy of the Catalyst Stash.  Consider:

    method person(Context $ctx, Int $id) stashes(Person :$person ) {
        $ctx->stash(person=>$ctx->model('People')->find($id));
    }

Thoughts welcomed!