-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent empty asset names #1912
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1417,26 +1417,32 @@ sub register_assets_from_settings { | |
|
||
# check assets and fix the file names | ||
for my $k (keys %assets) { | ||
my $a = $assets{$k}; | ||
if ($a->{name} =~ /\//) { | ||
log_info "not registering asset $a->{name} containing /"; | ||
my $asset = $assets{$k}; | ||
my ($name, $type) = ($asset->{name}, $asset->{type}); | ||
unless ($name && $type) { | ||
log_info 'not registering asset with empty name or type'; | ||
delete $assets{$k}; | ||
next; | ||
} | ||
my $f_asset = _asset_find($a->{name}, $a->{type}, \@parents); | ||
if ($name =~ /\//) { | ||
log_info "not registering asset $name containing /"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would change this to error too. While it's not fatal, it's something to keep an eye on... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's an error, it should be reported to the client - and raise an exception. Virtualization uses REPO_0=fixed/SLE... because they use REPO_0 in the pxe bootloader There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can try to propagate this error higher in the stack. That would be a little bit more work because it looks like the method is called 2 times and each not that "close to the user". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @coolo, this is input validation and failures should be reported to the client instead of getting logged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll change this. But for me this is not a reason to postpone merging this because we had this logging before, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on that too @Martchus. Might be worth revisiting input validation in general, since pretty much none of the API controllers do any validation themselves and leave it all to the model layer. Which is quite risky. |
||
delete $assets{$k}; | ||
next; | ||
} | ||
my $f_asset = _asset_find($name, $type, \@parents); | ||
unless (defined $f_asset) { | ||
# don't register asset not yet available | ||
delete $assets{$k}; | ||
next; | ||
} | ||
$a->{name} = $f_asset; | ||
$asset->{name} = $f_asset; | ||
$updated{$k} = $f_asset; | ||
} | ||
|
||
for my $a (values %assets) { | ||
for my $asset (values %assets) { | ||
# avoid plain create or we will get unique constraint problems | ||
# in case ISO_1 and ISO_2 point to the same ISO | ||
my $aid = $self->result_source->schema->resultset('Assets')->find_or_create($a); | ||
my $aid = $self->result_source->schema->resultset('Assets')->find_or_create($asset); | ||
$self->jobs_assets->find_or_create({asset_id => $aid->id}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,15 +40,19 @@ sub register { | |
my ($self, $type, $name, $missingok) = @_; | ||
$missingok //= 0; | ||
|
||
unless ($name) { | ||
log_warning "attempt to register asset with empty name"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, we had "log_info" in other cases and probably same reasoning as above applies: If it's an user error we should propagate to the user, otherwise info or debug, not warn, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just a sanity check which actually could be removed because the PR actually ensures that this method is not called with an empty name. If that would be true, the user hasn't done anything wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems very strange that a sanity check would log a warning instread of throwing an exception. |
||
return; | ||
} | ||
our %types = map { $_ => 1 } qw(iso repo hdd other); | ||
unless ($types{$type}) { | ||
log_warning "asset type '$type' invalid"; | ||
return; | ||
} | ||
unless (locate_asset $type, $name, mustexist => 1) { | ||
if (!$missingok) { | ||
log_warning "no file found for asset '$name' type '$type'"; | ||
} | ||
return 'missing' if ($missingok); | ||
|
||
log_warning "no file found for asset '$name' type '$type'"; | ||
return; | ||
} | ||
|
||
|
@@ -205,14 +209,20 @@ END_SQL | |
# prefetch all assets | ||
my $assets_arrayref = $dbh->selectall_arrayref($prioritized_assets_query); | ||
for my $asset_array (@$assets_arrayref) { | ||
my $id = $asset_array->[0]; | ||
my $id = $asset_array->[0]; | ||
my $name = $asset_array->[1]; | ||
if (!$name) { | ||
OpenQA::Utils::log_warning("asset cleanup: Skipping asset $id because its name is empty."); | ||
next; | ||
} | ||
|
||
my $type = $asset_array->[4]; | ||
my $fixed = $asset_array->[5]; | ||
my $dirname = ($fixed ? $type . '/fixed/' : $type . '/'); | ||
my $max_job = $asset_array->[6]; | ||
my %asset = ( | ||
id => $id, | ||
name => ($dirname . $asset_array->[1]), | ||
name => ($dirname . $name), | ||
t_created => $asset_array->[2], | ||
size => $asset_array->[3], | ||
type => $type, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add the asset type here and upgrade the log level to error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, there is no type - that's the error (in case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought making a different if or (unless) for
$name
and$type
would be a bit over-engineering for this hopefully rare error case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed it is, but actually my comment was about knowing the
type
of asset, rather than knowing the value of$name