[pmg-devel] [PATCH pmg-api 07/12] refactor API2::Domains for reuse in DKIMSign

[Dominik Csapak]

looks ok on a high level, some comments inline

On 10/7/19 9:28 PM, Stoiko Ivanov wrote:
> both DKIM Signed Domains and Relay Domains are lists of domains (DKIMSign falls
> back to Relay Domains). By refactoring the method creation we can reuse most
> of the code for the handling of DKIMSign domains
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>   src/PMG/API2/Domains.pm | 363 +++++++++++++++++++++-------------------
>   1 file changed, 195 insertions(+), 168 deletions(-)
> 
> diff --git a/src/PMG/API2/Domains.pm b/src/PMG/API2/Domains.pm
> index 1393475..1fa1b59 100644
> --- a/src/PMG/API2/Domains.pm
> +++ b/src/PMG/API2/Domains.pm
> @@ -14,205 +14,232 @@ use PMG::Config;
>   
>   use base qw(PVE::RESTHandler);
>   
> -__PACKAGE__->register_method ({
> -    name => 'index',
> -    path => '',
> -    method => 'GET',
> -    description => "List relay domains.",
> -    permissions => { check => [ 'admin', 'audit' ] },
> -    proxyto => 'master',
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {},
> -    },
> -    returns => {
> -	type => 'array',
> -	items => {
> -	    type => "object",
> -	    properties => {
> -		domain => { type => 'string'},
> -		comment => { type => 'string'},
> -	    },
> +my @method_args = ('domains', 'relay', 1);

method_args is rather generic, what about something like:
'domain_args'

> +
> +sub index_method {
> +    my ($fileid, $description_text, $run_postmap) = @_;

i would name fileid and description_text differently,
what about 'path' and 'type' ?
i know the 2nd parameter is used for the description, but i was
slightly confused, since 'relay' does not strike me as a 'description_text'

> +    return {
> +	name => 'index',
> +	path => '',
> +	method => 'GET',
> +	description => "List $description_text domains.",
> +	permissions => { check => [ 'admin', 'audit' ] },
> +	proxyto => 'master',
> +	parameters => {
> +	    additionalProperties => 0,
> +	    properties => {},
>   	},
> -	links => [ { rel => 'child', href => "{domain}" } ],
> -    },
> -    code => sub {
> -	my ($param) = @_;
> -
> -	my $domains = PVE::INotify::read_file('domains');
> -
> -	my $res = [];
> -
> -	foreach my $domain (sort keys %$domains) {
> -	    push @$res, $domains->{$domain};
> -	}
> -
> -	return $res;
> -    }});
> -
> -__PACKAGE__->register_method ({
> -    name => 'create',
> -    path => '',
> -    method => 'POST',
> -    proxyto => 'master',
> -    protected => 1,
> -    permissions => { check => [ 'admin' ] },
> -    description => "Add relay domain.",
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {
> -	    domain => {
> -		description => "Domain name.",
> -		type => 'string', format => 'transport-domain',
> +	returns => {
> +	    type => 'array',
> +	    items => {
> +		type => "object",
> +		properties => {
> +		    domain => { type => 'string'},
> +		    comment => { type => 'string'},
> +		},
>   	    },
> -	    comment => {
> -		description => "Comment.",
> -		type => 'string',
> -		optional => 1,
> +	    links => [ { rel => 'child', href => "{domain}" } ],
> +	},
> +	code => sub {
> +	    my ($param) = @_;
> +
> +	    my $domains = PVE::INotify::read_file($fileid);
> +
> +	    my $res = [];
> +
> +	    foreach my $domain (sort keys %$domains) {
> +		push @$res, $domains->{$domain};
> +	    }
> +
> +	    return $res;
> +	}};
> +}
> +
> +__PACKAGE__->register_method(index_method(@method_args));
> +
> +sub create_method {
> +    my ($fileid, $description_text, $run_postmap) = @_;
> +    return {
> +	name => 'create',
> +	path => '',
> +	method => 'POST',
> +	proxyto => 'master',
> +	protected => 1,
> +	permissions => { check => [ 'admin' ] },
> +	description => "Add $description_text domain.",
> +	parameters => {
> +	    additionalProperties => 0,
> +	    properties => {
> +		domain => {
> +		    description => "Domain name.",
> +		    type => 'string', format => 'transport-domain',
> +		},
> +		comment => {
> +		    description => "Comment.",
> +		    type => 'string',
> +		    optional => 1,
> +		},
>   	    },
>   	},
> -    },
> -    returns => { type => 'null' },
> -    code => sub {
> -	my ($param) = @_;
> +	returns => { type => 'null' },
> +	code => sub {
> +	    my ($param) = @_;
> +
> +	    my $code = sub {
>   
> -	my $code = sub {
> +		my $domains = PVE::INotify::read_file($fileid);
>   
> -	    my $domains = PVE::INotify::read_file('domains');
> +		die "Domain '$param->{domain}' already exists\n"
> +		    if $domains->{$param->{domain}};
>   
> -	    die "Domain '$param->{domain}' already exists\n"
> -		if $domains->{$param->{domain}};
> +		$domains->{$param->{domain}} = {
> +		    comment => $param->{comment} // '',
> +		};
>   
> -	    $domains->{$param->{domain}} = {
> -		comment => $param->{comment} // '',
> +		PVE::INotify::write_file($fileid, $domains);
> +
> +		PMG::Config::postmap_pmg_domains() if $run_postmap;
>   	    };
>   
> -	    PVE::INotify::write_file('domains', $domains);
> -
> -	    PMG::Config::postmap_pmg_domains();
> -	};
> -
> -	PMG::Config::lock_config($code, "add relay domain failed");
> -
> -	return undef;
> -    }});
> -
> -__PACKAGE__->register_method ({
> -    name => 'read',
> -    path => '{domain}',
> -    method => 'GET',
> -    description => "Read Domain data (comment).",
> -    proxyto => 'master',
> -    permissions => { check => [ 'admin', 'audit' ] },
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {
> -	    domain => {
> -		description => "Domain name.",
> -		type => 'string', format => 'transport-domain',
> +	    PMG::Config::lock_config($code, "add $description_text domain failed");
> +
> +	    return undef;
> +	}};
> +}
> +
> +__PACKAGE__->register_method(create_method(@method_args));
> +
> +sub read_method {
> +    my ($fileid, $description_text, $run_postmap) = @_;
> +    return {
> +	name => 'read',
> +	path => '{domain}',
> +	method => 'GET',
> +	description => "Read Domain data (comment).",
> +	proxyto => 'master',
> +	permissions => { check => [ 'admin', 'audit' ] },
> +	parameters => {
> +	    additionalProperties => 0,
> +	    properties => {
> +		domain => {
> +		    description => "Domain name.",
> +		    type => 'string', format => 'transport-domain',
> +		},
>   	    },
>   	},
> -    },
> -    returns => {
> -	type => "object",
> -	properties => {
> -	    domain => { type => 'string'},
> -	    comment => { type => 'string'},
> -	},
> -    },
> -    code => sub {
> -	my ($param) = @_;
> -
> -	my $domains = PVE::INotify::read_file('domains');
> -
> -	die "Domain '$param->{domain}' does not exist\n"
> -	    if !$domains->{$param->{domain}};
> -
> -	return $domains->{$param->{domain}};
> -    }});
> -
> -__PACKAGE__->register_method ({
> -    name => 'write',
> -    path => '{domain}',
> -    method => 'PUT',
> -    description => "Update relay domain data (comment).",
> -    protected => 1,
> -    permissions => { check => [ 'admin' ] },
> -    proxyto => 'master',
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {
> -	    domain => {
> -		description => "Domain name.",
> -		type => 'string', format => 'transport-domain',
> -	    },
> -	    comment => {
> -		description => "Comment.",
> -		type => 'string',
> +	returns => {
> +	    type => "object",
> +	    properties => {
> +		domain => { type => 'string'},
> +		comment => { type => 'string'},
>   	    },
>   	},
> -    },
> -    returns => { type => 'null' },
> -    code => sub {
> -	my ($param) = @_;
> -
> -	my $code = sub {
> +	code => sub {
> +	    my ($param) = @_;
>   
> -	    my $domains = PVE::INotify::read_file('domains');
> +	    my $domains = PVE::INotify::read_file($fileid);
>   
>   	    die "Domain '$param->{domain}' does not exist\n"
>   		if !$domains->{$param->{domain}};
>   
> -	    $domains->{$param->{domain}}->{comment} = $param->{comment};
> +	    return $domains->{$param->{domain}};
> +	}};
> +}
> +
> +__PACKAGE__->register_method(read_method(@method_args));
> +
> +sub write_method {
> +    my ($fileid, $description_text, $run_postmap) = @_;
> +    return {
> +	name => 'write',
> +	path => '{domain}',
> +	method => 'PUT',
> +	description => "Update $description_text domain data (comment).",
> +	protected => 1,
> +	permissions => { check => [ 'admin' ] },
> +	proxyto => 'master',
> +	parameters => {
> +	    additionalProperties => 0,
> +	    properties => {
> +		domain => {
> +		    description => "Domain name.",
> +		    type => 'string', format => 'transport-domain',
> +		},
> +		comment => {
> +		    description => "Comment.",
> +		    type => 'string',
> +		},
> +	    },
> +	},
> +	returns => { type => 'null' },
> +	code => sub {
> +	    my ($param) = @_;
>   
> -	    PVE::INotify::write_file('domains', $domains);
> +	    my $code = sub {
>   
> -	    PMG::Config::postmap_pmg_domains();
> -	};
> +		my $domains = PVE::INotify::read_file($fileid);
>   
> -	PMG::Config::lock_config($code, "update relay domain failed");
> +		die "Domain '$param->{domain}' does not exist\n"
> +		    if !$domains->{$param->{domain}};
>   
> -	return undef;
> -    }});
> +		$domains->{$param->{domain}}->{comment} = $param->{comment};
>   
> -__PACKAGE__->register_method ({
> -    name => 'delete',
> -    path => '{domain}',
> -    method => 'DELETE',
> -    description => "Delete a relay domain",
> -    protected => 1,
> -    permissions => { check => [ 'admin' ] },
> -    proxyto => 'master',
> -    parameters => {
> -	additionalProperties => 0,
> -	properties => {
> -	    domain => {
> -		description => "Domain name.",
> -		type => 'string', format => 'transport-domain',
> -	    },
> -	}
> -    },
> -    returns => { type => 'null' },
> -    code => sub {
> -	my ($param) = @_;
> +		PVE::INotify::write_file($fileid, $domains);
>   
> -	my $code = sub {
> +		PMG::Config::postmap_pmg_domains() if $run_postmap;
> +	    };
>   
> -	    my $domains = PVE::INotify::read_file('domains');
> +	    PMG::Config::lock_config($code, "update $description_text domain failed");
> +
> +	    return undef;
> +	}};
> +}
> +
> +__PACKAGE__->register_method(write_method(@method_args));
> +
> +sub delete_method {
> +    my ($fileid, $description_text, $run_postmap) = @_;
> +    return {
> +	name => 'delete',
> +	path => '{domain}',
> +	method => 'DELETE',
> +	description => "Delete a $description_text domain",
> +	protected => 1,
> +	permissions => { check => [ 'admin' ] },
> +	proxyto => 'master',
> +	parameters => {
> +	    additionalProperties => 0,
> +	    properties => {
> +		domain => {
> +		    description => "Domain name.",
> +		    type => 'string', format => 'transport-domain',
> +		},
> +	    }
> +	},
> +	returns => { type => 'null' },
> +	code => sub {
> +	    my ($param) = @_;
>   
> -	    die "Domain '$param->{domain}' does not exist\n"
> -		if !$domains->{$param->{domain}};
> +	    my $code = sub {
> +
> +		my $domains = PVE::INotify::read_file($fileid);
>   
> -	    delete $domains->{$param->{domain}};
> +		die "Domain '$param->{domain}' does not exist\n"
> +		    if !$domains->{$param->{domain}};
>   
> -	    PVE::INotify::write_file('domains', $domains);
> +		delete $domains->{$param->{domain}};
> +
> +		PVE::INotify::write_file($fileid, $domains);
> +
> +		PMG::Config::postmap_pmg_domains() if $run_postmap;
> +	    };
>   
> -	    PMG::Config::postmap_pmg_domains();
> -	};
> +	    PMG::Config::lock_config($code, "delete $description_text domain failed");
>   
> -	PMG::Config::lock_config($code, "delete relay domain failed");
> +	    return undef;
> +	}};
> +}
>   
> -	return undef;
> -    }});
> +__PACKAGE__->register_method(delete_method(@method_args));
>   
>   1;
> 

i think a better structure would be to have all methods on top of the 
file, and all uses at the bottom (like you use it in the next patch)

this way it is not as distracting

maybe even put the generator methods somewhere else ? (PMG::Domains ?)