Skip to content
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.

Sign up for GitHub

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

Introduce try/catch for image creation #2717

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce try/catch for image creation #2717

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/Utils/TestCreator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
use App\Models\TestImage;
use CDash\Model\Build;
use CDash\Model\Image;
use ErrorException;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Log;

/**
* This class is responsible for creating the various models associated
Expand Down Expand Up @@ -70,7 +72,13 @@ public function loadImage(Image $image): void

// Decode the data
$imgStr = base64_decode($image->Data);
$img = imagecreatefromstring($imgStr);
try {
$img = imagecreatefromstring($imgStr);
} catch (ErrorException) {
Log::error("Unable to create image object from data in #{$this->testName}");
$image->Checksum = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamjallen, without setting any value for $image->Checksum an additional error message was generated:

[2025-02-14 18:12:11] testing.ERROR: Unable to create image object from data in #my_test {"projectid":9,"buildid":59} 
[2025-02-14 18:12:11] testing.ERROR: ERROR:  null value in column "checksum" of relation "image" violates not-null constraint
DETAIL:  Failing row contains (3, \x633256304b454e5552564e5558314e5056564a445256394553564a46513152..., image/png, null). {"projectid":9,"buildid":59}

I assumed that adding a zero file, rather than denying the entry entirely, would allow the comparison to show that one of them was invalid, as as shown below

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the idea of storing "dummy" data in place of something which wasn't valid to begin with. If a submission is not valid, why shouldn't we just reject it entirely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamjallen, I agree to an extent. There are times where an invalid submission should be wholly rejected, I think that a valid test outcome of an invalid image being sent up for comparison should not be grounds for rejection

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if we don't reject the entire submission, what about just not saving the image in the first place? I'm generally a fan of fail-fast systems, but I agree that there could be a place for handling the error instead here.

My primary concern with not failing the submission is that if we don't cause the submission to fail, how will anyone know that the "image" they're submitting is actually a text file?

return;
}
ob_start();
switch ($image->Extension) {
case 'image/jpeg':
Expand Down