Validating single arg id

[Bill Moseley]

I have a number of methods that start something like this:

sub view : Local Args(1) {

    my ( $self, $c, $id ) = @_;

    my $obj = $c->model( 'DB::Foo' )->find( $id )
       || return $c->res->status( 404 );

If $id is not valid then I might, as in that example, return with a 404 status.

Of course, if $id is suppose to be an integer and a non-integer or an integer out of range is provided then the the database will throw an exception, which I want to prevent.  I want valid ids to return an object and *anything* else to return undef before hitting the database.

This is pretty low-level validation -- just validating primary key.  For more complex validation I use a form validation module.

Obviously, I could do something like

return $c->res->status(404) unless $c->model('DB::Foo')->is_valid_id( $id )

in every method, but that's not very DRY.

What I've done in the past is override the find() or search() method in my model base class so that whatever $id is passed it is validated.  Specific model classes can override the is_valid_id()  method if they use keys that are not a common key format (i.e. different integer range or non-integer key).

What's you approach to validating that $id in situations like this where there's a single id?

Do you just let the database throw the exception?  I prefer to return 404s for invalid ids, regardless of their format (and likewise for ids that point to valid object, but are not owned by the current user instead of a 403).




--
Bill Moseley
moseley@...

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Matt Whipple]

Bill Moseley wrote:

>
> I have a number of methods that start something like this:
>
> sub view : Local Args(1) {
>     my ( $self, $c, $id ) = @_;
>
>     my $obj = $c->model( 'DB::Foo' )->find( $id )
>        || return $c->res->status( 404 );
>
> If $id is not valid then I might, as in that example, return with a
> 404 status.
>
> Of course, if $id is suppose to be an integer and a non-integer or an
> integer out of range is provided then the the database will throw an
> exception, which I want to prevent.  I want valid ids to return an
> object and *anything* else to return undef before hitting the database.
>
> This is pretty low-level validation -- just validating primary key.  
> For more complex validation I use a form validation module.
>
> Obviously, I could do something like
>
> return $c->res->status(404) unless $c->model('DB::Foo')->is_valid_id(
> $id )
>
> in every method, but that's not very DRY.
>
> What I've done in the past is override the find() or search() method
> in my model base class so that whatever $id is passed it is
> validated.  Specific model classes can override the is_valid_id()  
> method if they use keys that are not a common key format (i.e.
> different integer range or non-integer key).
>
> What's you approach to validating that $id in situations like this
> where there's a single id?
>
> Do you just let the database throw the exception?  I prefer to return
> 404s for invalid ids, regardless of their format (and likewise for ids
> that point to valid object, but are not owned by the current user
> instead of a 403).
>

I would probably avoid potentially hitting the database unnecessarily,
and only use the exception in the case of complicated validation.  In
the case of simple validation, why not something simple along the lines of:

sub some_action {
    my ($self, $c) = (shift, shift);
    #Get type checking out of the way
    my $id = check_id(shift, $c));
     
    #Continue on with apparently good data
};

sub check_id {
    my ($id, $c) = @_;
    $c->res->status(404) if ( !validate_id($id) );
    return $id;
};

sub validate_id {
    return 1 if looks_like_number(shift);
    return;
};  

>
>
>
> --
> Bill Moseley
> moseley@... <mailto:moseley@...>
> ------------------------------------------------------------------------
>
> _______________________________________________
> List: Catalyst@...
> Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
> Searchable archive: http://www.mail-archive.com/catalyst@.../
> Dev site: http://dev.catalyst.perl.org/
>  

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[J. Shirley]

On Fri, Oct 16, 2009 at 6:45 AM, Bill Moseley <moseley@...> wrote:

I have a number of methods that start something like this:

sub view : Local Args(1) {

    my ( $self, $c, $id ) = @_;

    my $obj = $c->model( 'DB::Foo' )->find( $id )
       || return $c->res->status( 404 );

If $id is not valid then I might, as in that example, return with a 404 status.

Of course, if $id is suppose to be an integer and a non-integer or an integer out of range is provided then the the database will throw an exception, which I want to prevent.  I want valid ids to return an object and *anything* else to return undef before hitting the database.

This is pretty low-level validation -- just validating primary key.  For more complex validation I use a form validation module.

Obviously, I could do something like

return $c->res->status(404) unless $c->model('DB::Foo')->is_valid_id( $id )

in every method, but that's not very DRY.

What I've done in the past is override the find() or search() method in my model base class so that whatever $id is passed it is validated.  Specific model classes can override the is_valid_id()  method if they use keys that are not a common key format (i.e. different integer range or non-integer key).

What's you approach to validating that $id in situations like this where there's a single id?

Do you just let the database throw the exception?  I prefer to return 404s for invalid ids, regardless of their format (and likewise for ids that point to valid object, but are not owned by the current user instead of a 403).

What database do you use?  In MySQL and SQLite I get no exception at all:

$ perl -Ilib -e 'use MyApp; my $obj = MyApp->model('Schema::User')->find("bogus"); print "defined? " . ( defined $obj ? "yes" : "no" ) . "\n"';
 no
$

I'd be more inclined to use Try::Tiny and wrap it that way if you're up against annoying exceptions.

-J

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[J. Shirley]

On Fri, Oct 16, 2009 at 7:19 AM, Matt Whipple <matt@...> wrote:

I would probably avoid potentially hitting the database unnecessarily, and only use the exception in the case of complicated validation.  In the case of simple validation, why not something simple along the lines of:

sub some_action {
  my ($self, $c) = (shift, shift);
  #Get type checking out of the way
  my $id = check_id(shift, $c));
      #Continue on with apparently good data
};

sub check_id {
  my ($id, $c) = @_;
  $c->res->status(404) if ( !validate_id($id) );
  return $id;
};

sub validate_id {
  return 1 if looks_like_number(shift);
  return;
};  

If you are using DBIx::Class, just define your schema's default resultset class to have this method (safe_find) or something (using 'default_resultset_class' in ->load_namespaces)

package MyApp::Schema::ResultSet;

use Moose;
extends 'DBIx::Class::ResultSet';

sub safe_find {
    my ( $self, $id ) = @_;
    if ( $id =~ /^(\d+)$/ ) {
         return $self->find($1);
    }
    die "I pity the fool";
}

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Iain-5]

Bill Moseley wrote:

>
> I have a number of methods that start something like this:
>
> sub view : Local Args(1) {
>     my ( $self, $c, $id ) = @_;
>
>     my $obj = $c->model( 'DB::Foo' )->find( $id )
>        || return $c->res->status( 404 );
>
> If $id is not valid then I might, as in that example, return with a
> 404 status.
>
> Of course, if $id is suppose to be an integer and a non-integer or an
> integer out of range is provided then the the database will throw an
> exception, which I want to prevent.  I want valid ids to return an
> object and *anything* else to return undef before hitting the database.
>
> This is pretty low-level validation -- just validating primary key.  
> For more complex validation I use a form validation module.
>
> Obviously, I could do something like
>
> return $c->res->status(404) unless $c->model('DB::Foo')->is_valid_id(
> $id )
>
> in every method, but that's not very DRY.
>
> What I've done in the past is override the find() or search() method
> in my model base class so that whatever $id is passed it is
> validated.  Specific model classes can override the is_valid_id()  
> method if they use keys that are not a common key format (i.e.
> different integer range or non-integer key).
>
> What's you approach to validating that $id in situations like this
> where there's a single id?
>
> Do you just let the database throw the exception?  I prefer to return
> 404s for invalid ids, regardless of their format (and likewise for ids
> that point to valid object, but are not owned by the current user
> instead of a 403).
>

We did this sort of thing until we started to use chained actions.
Now we validate once at the start of the chain e.g.

sub start : Chained('/') CaptureArgs(0) {
    my ( $self, $c ) = @_;
   
    # your validation here
    my $obj = $c->model( 'DB::Foo' )->find( $id )
       || return $c->res->status( 404 );

    $c->stash->{obj} = $obj;

    return 1;
}

sub view : Chained('start') Args(0) {
    my ( $self, $c ) = @_;
    # do something with $c->stash->{obj}
    return 1;
}

until we did this we had boilerplate validation at the top of all the
local actions.

Iain.



_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Bill Moseley]

On Fri, Oct 16, 2009 at 7:31 AM, J. Shirley <jshirley@...> wrote:

 

What database do you use?  In MySQL and SQLite I get no exception at all:

Postgresql.
 

$ perl -Ilib -e 'use MyApp; my $obj = MyApp->model('Schema::User')->find("bogus"); print "defined? " . ( defined $obj ? "yes" : "no" ) . "\n"';
 no

 DBIx::Class::ResultSet::find(): DBI Exception: DBD::Pg::st execute failed: ERROR:  invalid input syntax for integer: "bogus" [for Statement "SELECT me.id ...

sub safe_find {
    my ( $self, $id ) = @_;
    if ( $id =~ /^(\d+)$/ ) {
         return $self->find($1);
    }
    die "I pity the fool";
}

Yes, that's one approach.    But, I'm not sure I can think of a reason why I would not want to always do that, so might as well just override find (or really search_rs, as that is what is called by find) and check id (or ids) there.  But, there need to think about "where", "columns", and so on.

--
Bill Moseley
moseley@...

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Aristotle Pagaltzis]

* iain <iainhubbard@...> [2009-10-16 17:30]:
> until we did this we had boilerplate validation at the top of
> all the local actions.

++

Bill, in another thread you asked me for an example of how
Chained helps make things like complex auth checks more DRY. I’ve
been meaning to respond with an example out of my $job codebase,
but until I get around to it, here’s your hands-on example of
something it’d buy your codebase immediately. :-)

Regards,
--
Aristotle Pagaltzis // <http://plasmasturm.org/>

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Bill Moseley]

On Sat, Oct 17, 2009 at 12:50 PM, Aristotle Pagaltzis <pagaltzis@...> wrote:

* iain <iainhubbard@...> [2009-10-16 17:30]:

> until we did this we had boilerplate validation at the top of
> all the local actions.

++

Bill, in another thread you asked me for an example of how
Chained helps make things like complex auth checks more DRY. I’ve
been meaning to respond with an example out of my $job codebase,
but until I get around to it, here’s your hands-on example of
something it’d buy your codebase immediately. :-)

I meant to respond about that.  Perhaps I'm missing something, but the chained example works great if there multiple actions that need to get at that "Foo" object.  They can all chain back to the same root which validates the id and then fetches the Foo object with that id and stuffs it into the stash.

But, its often the case (well, my case) that there's only *one* action that fetches Foo, but there's also single actions for find()ing a number of other objects.  If I'm following the chained example correctly, then there would be root for each -- Foo, Bar, Baz, and so on each running basically the same validation.  I suppose the root can be written in a way to fetch many different object types by inspecting the current action and thus the fetch for Foo, Bar, Baz, etc. objects could all have the same root of the chain.

If I have many actions that do $c->model( $whatever )->find( $id ) and I want to validate id doesn't it seems like find() (or really search_rs() ) would be the method to override?

Perhaps a better approach would be add a "safe_find()" as J. Shirley suggested, but what good are methods if you don't override them. ;)

--
Bill Moseley
moseley@...

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Zbigniew Lukasiak]

On Fri, Oct 16, 2009 at 5:24 PM, iain <iainhubbard@...> wrote:

> Bill Moseley wrote:
>>
>> I have a number of methods that start something like this:
>>
>> sub view : Local Args(1) {
>>    my ( $self, $c, $id ) = @_;
>>
>>    my $obj = $c->model( 'DB::Foo' )->find( $id )
>>       || return $c->res->status( 404 );
>>
>> If $id is not valid then I might, as in that example, return with a 404
>> status.
>>
>> Of course, if $id is suppose to be an integer and a non-integer or an
>> integer out of range is provided then the the database will throw an
>> exception, which I want to prevent.  I want valid ids to return an object
>> and *anything* else to return undef before hitting the database.
>>
>> This is pretty low-level validation -- just validating primary key.  For
>> more complex validation I use a form validation module.
>>
>> Obviously, I could do something like
>>
>> return $c->res->status(404) unless $c->model('DB::Foo')->is_valid_id( $id
>> )
>>
>> in every method, but that's not very DRY.
>>
>> What I've done in the past is override the find() or search() method in my
>> model base class so that whatever $id is passed it is validated.  Specific
>> model classes can override the is_valid_id()  method if they use keys that
>> are not a common key format (i.e. different integer range or non-integer
>> key).
>>
>> What's you approach to validating that $id in situations like this where
>> there's a single id?
>>
>> Do you just let the database throw the exception?  I prefer to return 404s
>> for invalid ids, regardless of their format (and likewise for ids that point
>> to valid object, but are not owned by the current user instead of a 403).
>>
>
> We did this sort of thing until we started to use chained actions.
> Now we validate once at the start of the chain e.g.
>
> sub start : Chained('/') CaptureArgs(0) {
>   my ( $self, $c ) = @_;
>     # your validation here
>   my $obj = $c->model( 'DB::Foo' )->find( $id )
>      || return $c->res->status( 404 );
>
>   $c->stash->{obj} = $obj;
>
>   return 1;
> }
>
> sub view : Chained('start') Args(0) {
>   my ( $self, $c ) = @_;
>   # do something with $c->stash->{obj}
>   return 1;
> }

What is the advantage of this over:

sub view : Local {
  my ( $self, $c, $id ) = @_;
  $self->start( $c, $id );
  # do something with $c->stash->{obj}
  return 1;
}


--
Zbigniew Lukasiak
http://brudnopis.blogspot.com/
http://perlalchemy.blogspot.com/

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Aristotle Pagaltzis]

* Zbigniew Lukasiak <zzbbyy@...> [2009-10-21 14:15]:
> What is the advantage of this over:
>
> sub view : Local {
>  my ( $self, $c, $id ) = @_;
>  $self->start( $c, $id );
>  # do something with $c->stash->{obj}
>  return 1;
> }

Consider `/forum/7/topic/13/post/4/editform`. The end point in
that chain would be

    sub editform : Chained('post') {
        my ( $self, $c ) = @_;
        # ...
    }

The equivalent URI with Local would be `/editpost/7/13/4` and the
action would look like this:

    sub editform : Local {
        my ( $self, $c, $forum, $topic, $post ) = @_;
        $self->load_post( $forum, $topic, $post );
        # ...
    }

(And then of course `load_post` would have to look something like
the following.)

    sub load_post {
        my ( $self, $forum, $topic, $post ) = @_;
        $self->load_topic( $forum, $topic );
        # ...
    }

Notice what’s going on with the argument lists between the two
examples.

Which code structure looks more DRY and which URI structure more
consistent to you?

--
*AUTOLOAD=*_;sub _{s/(.*)::(.*)/print$2,(",$\/"," ")[defined wantarray]/e;$1}
&Just->another->Perl->hack;
#Aristotle Pagaltzis // <http://plasmasturm.org/>

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Iain-5]

Zbigniew Lukasiak wrote:

> What is the advantage of this over:
>
> sub view : Local {
>   my ( $self, $c, $id ) = @_;
>   $self->start( $c, $id );
>   # do something with $c->stash->{obj}
>   return 1;
> }
>
>
>  

For me there are lots of little reasons. I suppose its a personal choice.

I like the URI structure better and chained makes me think about the URI
more.

I prefer /item/1234/view to /item/view/1234 as it lets you add more
functionality in an intuitive way:

/item/1234/edit
/item/1234/delete
etc...

and if the user just deletes the edit/view part of the uri you can do
something sensible.

Other people in the team can just chuck a new action on the URI without
wondering what I called my validation routine.

It prints a nice URI dispatch table in debug mode.

As I said, for me its lots of little reasons that just feel right. But
mainly its about the URI structure.

Iain.

p.s. What Aristotle said.

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Bill Moseley]

On Wed, Oct 21, 2009 at 6:52 AM, Aristotle Pagaltzis <pagaltzis@...> wrote:

* Zbigniew Lukasiak <zzbbyy@...> [2009-10-21 14:15]:

> What is the advantage of this over:
>
> sub view : Local {
>  my ( $self, $c, $id ) = @_;
>  $self->start( $c, $id );
>  # do something with $c->stash->{obj}
>  return 1;
> }

Consider `/forum/7/topic/13/post/4/editform`. The end point in
that chain would be

   sub editform : Chained('post') {
       my ( $self, $c ) = @_;
       # ...
   }

The equivalent URI with Local would be `/editpost/7/13/4` and the
action would look like this:

   sub editform : Local {
       my ( $self, $c, $forum, $topic, $post ) = @_;
       $self->load_post( $forum, $topic, $post );
       # ...
   }

I think that depends on your data.  If $forum, $topic, and $post make up a path on disk then yes, the chain is really useful.
If $post is a primary key and there's a relationship $post->topic->forum then there's no need for those, of course, and confusion if $post->topic is not the same id as passed in the URL for the forum.

Perhaps a good use for the chain there is for access control -- the current user might only have access to some forums so a chain makes it easy to do that kind of validation early in the chain and then detach if access fails.

The chain also allows fetching the $forum and $topic objects and place them in the stash.  But, again if they are related can load the $post with a join and avoid separate calls to the database.  (But, it may be the case that caching the $forum and $topic individually make sense.)

Anyway, my actions often look like this: (a bit oversimplified)

package MyApp::Forum::Post;
sub view : Local {
    my ( $self, $c, $post_id ) = @_;

    $c->stash->{post} = $c->user->fetch_post( $id ) || return $c->res->status( 404 );
}

Kind of ugly calling that on the user object, but it's just an example.  That method, for example, might do a join with the forum and topic tables and also with a permissions table to make sure the user can access the post.

I do prefer the /forum/post/334/view type of URLs, though, but my CRUD actions often end up like this:

   /forum/post - list
   /forum/post/edit  - create (POST)
   /forum/post/edit/22 - view (GET) update (PUT/POST)
   /forum/post/delete/22

And it's very DRY because there's separate model method for each.

The create method may have a different URL structure to specify the topic, or it might be ?topic=123, or it might be part of the post parameters.

--
Bill Moseley
moseley@...

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/

[Aristotle Pagaltzis]

* Bill Moseley <moseley@...> [2009-10-18 01:40]:

> On Sat, Oct 17, 2009 at 12:50 PM, Aristotle Pagaltzis <pagaltzis@...>wrote:
> > * iain <iainhubbard@...> [2009-10-16 17:30]:
> > > until we did this we had boilerplate validation at the top
> > > of all the local actions.
> >
> > ++
> >
> > Bill, in another thread you asked me for an example of how
> > Chained helps make things like complex auth checks more DRY.
> > I’ve been meaning to respond with an example out of my $job
> > codebase, but until I get around to it, here’s your hands-on
> > example of something it’d buy your codebase immediately. :-)
>
> I meant to respond about that. Perhaps I'm missing something,
> but the chained example works great if there multiple actions
> that need to get at that "Foo" object. They can all chain back
> to the same root which validates the id and then fetches the
> Foo object with that id and stuffs it into the stash.

Exactly.

> But, its often the case (well, my case) that there's only *one*
> action that fetches Foo, but there's also single actions for
> find()ing a number of other objects. If I'm following the
> chained example correctly, then there would be root for each --
> Foo, Bar, Baz, and so on each running basically the same
> validation.

OK, that’s a situation in which Chained indeed won’t help (for
this particular aspect of the task).

> I suppose the root can be written in a way to fetch many
> different object types by inspecting the current action and
> thus the fetch for Foo, Bar, Baz, etc. objects could all have
> the same root of the chain.

No no no, that’s not what I was suggesting. That’s no good. If
you need to contort the code into unnatural structures, then
either you’re not using Chained right or it’s not the right tool.
Chained is good, but it’s not end in itself; it’s good because it
helps simplify your code, not just because.

> If I have many actions that do $c->model( $whatever )->find(
> $id ) and I want to validate id doesn't it seems like find()
> (or really search_rs() ) would be the method to override?

Yes, that would indeed be better in your case.

> Perhaps a better approach would be add a "safe_find()" as J.
> Shirley suggested, but what good are methods if you don't
> override them. ;)

That depends on several a lot of very subtle considerations. Does
that method get called internally by its framework?  Do they
depend on its precise semantics? How transparent is your new
interface, in general? If the overridden method’s semantics
change in a future framework version (adding back-compatible
features, most likely), will you have an easy way to provide
those new features to your own code also, or will your overridden
interface make it impossible or difficult?

After a few mildly painful experiences, I have become a bit gun
shy about overriding things, particularly for making them “more
powerful”, and when in doubt, I would avoid it. For some kinds of
making things safer, though, overriding is the better choice.
(Yours might be such a case.) But it needs a lot of deliberation.
Just adding a different method is much less vagarious.

Regards,
--
Aristotle Pagaltzis // <http://plasmasturm.org/>

_______________________________________________
List: Catalyst@...
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@.../
Dev site: http://dev.catalyst.perl.org/