Hi Oliver, Oliver Tappe schrieb:
attached is a function I have found useful in some of the OTRS-modules I worked on recently. It is called CheckParams() and its purpose is to offer an intuitive and flexible way of checking the params-hash that has been passed into a method.
Currently, many methods contain code like this:
# check all needed objects foreach (qw(ConfigObject LogObject DBObject TimeObject MainObject)) { die "Got no $_" if (!$Self->{$_}); }
plus more checks for additional parameters that must be passed in via the %Params hash.
When using CheckParams, the code looks like this:
# check all needed objects CheckParams($Self, sub { 'ConfigObject' => '!Class=Kernel::Config', 'DBObject' => '!Class=Kernel::System::DB', 'LogObject' => '!Class=Kernel::System::Log', 'MainObject' => '!Class=Kernel::System::Main', 'TimeObject' => '!Class=Kernel::System::Time', });
As you can see, you pass CheckParam() the hash-ref that shall be checked and a specification (for performance reasons this should always be an anonymous subroutine). In the specification, you simply declare the supported parameters, whether or not they are required (!) or optional (?) or if they have to be of a specific class (Class=...).
CheckParams() supports checking of more complex data structures, too:
CheckParams($Params, sub { 'GeneralAttrs' => { 'From' => '!', 'To' => '!', }, 'JobAttrs' => { 'Creator' => '!', 'Changes' => [ { 'Action' => 'm{^(create|change|delete|sync)$}', 'Path' => '!', 'Content' => '?', } ], } });
This code snippet makes sure that the given $Params-hash contains two elements (GeneralAttrs & JobAttrs), each of which has to follow the specified substructures.
As I believe this function could be useful throughout OTRS (now that we have the 2.2-branch), I'd like to add it to the framework. I can see two ways of doing that:
1.) non-OO style: add some kind of helper module (e.g. Utils.pm) and add CheckParams() as a *function* to that. 2.) OO style: add a new module Utils/Params.pm which contains a *class-method* Check(), such that you'd have to invoke it like this Utils::Params->Check($Params, sub { ... });
So what do you think? Do you like the idea in general at all? If so, how should that functionality be integrated (OO- or non-OO-style)?
Please fire any suggestions or questions to this list or directly my way.
-=> Yes, 'die "Got no $_" if (!$Self->{$_});' the die() is not a option. ;) Bit I see different things to integrate it into the current OTRS framework (may be we are talking about two different things?). Anyway, here my points: o) Keep it clean: If we have a check method in the framework, we should replace any other checks from the whole framework with the new check to keep it clean. o) Check it every time?/Performance Issue: "# check all needed objects" - For normal framework, it should not be necessary to check the default objects. The framework should only deliver existing objects. You never should use e. g. $Self->{LogObject} for other perl objects. I also would see a really performance issue if we would check the object content every time. -=> So for the OTRS framework I would expect to not need to check the base objects. o) OO-Style; Only OO-Style. It would be not good to switch to other styles. o) confess(): I have looked into your source code. I never have seen confess() in the OTRS framework bevor. Whats about this? o) Filename: Utils/Params.pm is not on the current directory style. Kernel/System/Params.pm or to extend the existing Kernel/System/CheckItem.pm would sounds a little bit better to me. o) Utils::Params->Check($Params, sub { ... }); looks a little bit strange to me. I would suggest something like "$Self->{ParamsObject}" oder "$Self->{CheckObject}". And a methode like "$Self->{ParamsObject}->Check( 'ConfigObject' => '!Class=Kernel::Config', 'DBObject' => '!Class=Kernel::System::DB', 'LogObject' => '!Class=Kernel::System::Log', 'MainObject' => '!Class=Kernel::System::Main', 'TimeObject' => '!Class=Kernel::System::Time', );" o) One other thing would be to define more via attributes. E. g. 'TimeObject' => '!Class=Kernel::System::Time', Is IMO not so good because you need to use some cpu time to use a regexp for to find out what "!Class=" is. I mean something like e. g. 'TimeObject' => { Operation => '!', Type => 'Class', Match => 'Kernel::System::Time', }, IMO this would be faster and need less cpu time. o Maybe we need something like Kernel::System::Main->Die(). So back to the two different things: IMO we need only a exists-check for objects (not content related) but we could use a new Check() for param validation of e. g. strings or other content. What do you think? :-) Greetings, -Martin
------------------------------------------------------------------------
=item CheckParams()
Utility function that can be used by any method that accepts param-hashes to check if the given parameters actually match the expectations.
Each individual parameter has a specification that describes the expectation that the calling function has towards this param. The following specifications are supported:
* '!' - the parameter is required * '?' - the parameter is optional * 'm{regex}' - the parameter must match the given regex * '!Class=...' - the parameter is required and must be an object of the given class * '?Class=...' - if the parameter has been given, it must be an object of the given class
The function will confess for any unknown, missing, or non-matching param.
=cut
sub CheckParams { my $Params = shift or confess('need to pass in params-hashref!'); my $ParamsSpec = shift or confess('need to pass in params-spec-hashref (or -sub)!');
# TODO: allow to switch off this function via configuration in production # environments, as it is rather heavy
# fetch param-spec from function, if that has been given: if (ref($ParamsSpec) eq 'CODE') { $ParamsSpec = $ParamsSpec->(); }
# print a warning for any unknown parameters that have been given: my @UnknownParams = grep { !exists $ParamsSpec->{$_}; } keys %$Params; if (@UnknownParams) { my $UnknownParamsStr = join ',', @UnknownParams; confess("Enocuntered unknown params: '$UnknownParamsStr'!\n"); }
# check if all required params have been specified: foreach my $Param (keys %$ParamsSpec) { my $Spec = $ParamsSpec->{$Param}; if (ref($Spec) eq 'HASH') { # Handle nested specs by recursion: my $SubParams = $Params->{$Param}; if (!defined $SubParams) { confess("Required param '$Param' is missing!"); } CheckParams($SubParams, $Spec); } elsif (ref($Spec) eq 'ARRAY') { # Handle nested spec arrays by looped recursion: my $SubParams = $Params->{$Param}; if (!defined $SubParams) { confess("Required param '$Param' is missing!"); } elsif (ref($SubParams) ne 'ARRAY') { confess("Value for param '$Param' must be an array-ref!"); } foreach my $SubParam (@$SubParams) { CheckParams($SubParam, $Spec->[0]); } } elsif ($Spec eq '!') { # required parameter: if (!exists $Params->{$Param}) { confess("Required param '$Param' is missing!"); } } elsif ($Spec =~ m{^\!Class=(.+)$}i) { my $Class = $1; # required parameter ... if (!exists $Params->{$Param}) { confess("Required param '$Param' is missing!"); } # ... of specific class if (!$Params->{$Param}->isa($Class)) { confess("Param '$Param' is not a '$Class', but that is required!"); } } elsif ($Spec eq '?') { # optional parameter - nothing to do } elsif ($Spec =~ m{^\?Class=(.+)$}i) { my $Class = $1; # optional parameter ... if (exists $Params->{$Param}) { # ... of specific class if (!$Params->{$Param}->isa($Class)) { confess("Param '$Param' is not a '$Class', but that is required!"); } } } elsif ($Spec =~ m{^m{(.+)}$}) { # try to match given regex: my $Regex = $1; my $Value = $Params->{$Param}; if ($Value !~ m{$Regex}) { confess("Required param '$Param' isn't matching regex '$Regex' (given value was '$Value')!"); } } else { # complain about unknown spec: confess("Unknown param-spec '$Spec' encountered!"); } }
return scalar 1; }