Fixed #13296 Generating Custom Report Error #13297

inietov · 2023-07-12T00:39:02Z

Description

In some edge cases the Custom Report fails when trying to retrieve a belongsTo() relationship over asset models. ~~I added a check to evaluate if the model exists before trying to return that relationship, like in other relationships from the Asset model.~~

Take 2: I added a default blank item in the asset->models relationship, so when it's called in asset->models->depreciation relationship it found one and doesn't fails. I can set a default model with, for example, a name attribute with not found as value or something like that, but I think a blank value is equally expressive in this case.

Fixes #13296

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Test Configuration:

PHP version: 8.1
MySQL version: 8.0.31
Webserver version: PHP dev server
OS version: Debian 11

what-the-diff · 2023-07-12T00:39:14Z

PR Summary

Modification in Asset Model
The depreciation method in the Asset model has been adjusted. Now, it evaluates whether the model for the asset exists before initiating its related attributes. This acts as a safeguard to catch any potential errors and ensure smoother functioning of the system.

snipe · 2023-07-12T14:31:52Z

Wouldn't it make more sense to check for depreciation being valid/present in the report itself?

jayavman · 2023-07-12T23:16:44Z

Hi @snipe pretty hard to do If I cant download the report in the first place. Why would it spit those errors in any case?

252819441-3d534264-96bf-4924-974f-c49f3f8a2917

[2023-07-12 09:50:21] production.ERROR: Error: Call to a member function belongsTo() on null in /home/server/public_html/asset/app/Models/Asset.php:393
Stack trace:
#0 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(539): App\Models\Asset->depreciation()
#1 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(491): Illuminate\Database\Eloquent\Model->getRelationshipFromMethod('depreciation')
#2 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(440): Illuminate\Database\Eloquent\Model->getRelationValue('depreciation')
#3 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(2029): Illuminate\Database\Eloquent\Model->getAttribute('depreciation')
#4 /home/server/public_html/asset/app/Http/Controllers/ReportsController.php(822): Illuminate\Database\Eloquent\Model->__get('depreciation')
#5 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Concerns/BuildsQueries.php(51): App\Http\Controllers\ReportsController->App\Http\Controllers{closure}(Object(Illuminate\Database\Eloquent\Collection), 1)
#6 /home/server/public_html/asset/app/Http/Controllers/ReportsController.php(881): Illuminate\Database\Eloquent\Builder->chunk(20, Object(Closure))
#7 /home/server/public_html/asset/vendor/symfony/http-foundation/StreamedResponse.php(109): App\Http\Controllers\ReportsController->App\Http\Controllers{closure}()
#8 /home/server/public_html/asset/vendor/symfony/http-foundation/Response.php(394): Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
#9 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Support/HigherOrderTapProxy.php(34): Symfony\Component\HttpFoundation\Response->send()
#10 /home/server/public_html/asset/public/index.php(53): Illuminate\Support\HigherOrderTapProxy->__call('send', Array)
#11 {main}
[2023-07-12 09:50:21] production.ERROR: Call to a member function belongsTo() on null {"userId":1,"exception":"[object] (Error(code: 0): Call to a member function belongsTo() on null at /home/server/public_html/asset/app/Models/Asset.php:393)
[stacktrace]
#0 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(539): App\Models\Asset->depreciation()
#1 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(491): Illuminate\Database\Eloquent\Model->getRelationshipFromMethod('depreciation')
#2 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php(440): Illuminate\Database\Eloquent\Model->getRelationValue('depreciation')
#3 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(2029): Illuminate\Database\Eloquent\Model->getAttribute('depreciation')
#4 /home/server/public_html/asset/app/Http/Controllers/ReportsController.php(822): Illuminate\Database\Eloquent\Model->__get('depreciation')
#5 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Database/Concerns/BuildsQueries.php(51): App\Http\Controllers\ReportsController->App\Http\Controllers\{closure}(Object(Illuminate\Database\Eloquent\Collection), 1)
#6 /home/server/public_html/asset/app/Http/Controllers/ReportsController.php(881): Illuminate\Database\Eloquent\Builder->chunk(20, Object(Closure))
#7 /home/server/public_html/asset/vendor/symfony/http-foundation/StreamedResponse.php(109): App\Http\Controllers\ReportsController->App\Http\Controllers\{closure}()
#8 /home/server/public_html/asset/vendor/symfony/http-foundation/Response.php(394): Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
#9 /home/server/public_html/asset/vendor/laravel/framework/src/Illuminate/Support/HigherOrderTapProxy.php(34): Symfony\Component\HttpFoundation\Response->send()
#10 /home/server/public_html/asset/public/index.php(53): Illuminate\Support\HigherOrderTapProxy->__call('send', Array)
#11 {main}
"}

snipe · 2023-07-13T08:45:37Z

@jayavman My last comment was meant for @inietov, not you :) Meaning I think I’d like to see this fix handled in a slightly different way.

jayavman · 2023-07-13T10:54:40Z

@snipe sorry, I'll leave you to work your magic :>

inietov · 2023-07-14T02:39:20Z

@snipe I had not thought of that but I just test it and didn't work, I think it is because the call to asset->getDepreciatedValue() is still made.

We should never have an asset without a model, so I don't think adding a default model is disruptive or can be a potential risk on other places where this relation is needed. But do let me know if you think I need to adjust something here.

snipe · 2023-07-15T11:40:33Z

@inietov so this would fx an error that happens in depreciation in the custom report when there is no model?

snipe · 2023-07-15T11:48:09Z

[2023-07-12 09:50:21] production.ERROR: Error: Call to a member function belongsTo() on null in /home/server/public_html/asset/app/Models/Asset.php:393

@jayavman this definitely sounds like a data issue to me.

Line 393 is the stuff in this method:

public function depreciation()
    {
        return $this->model->belongsTo(\App\Models\Depreciation::class, 'depreciation_id');
    }

Which implies that there is a model an asset belongs to that is associated with an invalid depreciation or there is an invalid model to start with.

jayavman · 2023-07-15T23:18:55Z

hey @snipe anyway to find this easily? I have 14 000 assets?

inietov · 2023-07-17T16:29:04Z

@inietov so this would fx an error that happens in depreciation in the custom report when there is no model?

@snipe yes, it really should not happen that we end up with an asset having assigned an inexistent model (I had to edit the asset via the database to reproduce the error), but I see this fix as some of the conditions we sometimes add to models getters like precisely the depreciation one:

So the system doesn't crash and users can fix their data if needed.

snipe · 2023-07-18T12:28:58Z

That conditional should never fire though, since we check for $this->model, no?

inietov · 2023-07-18T16:49:53Z

That conditional should never fire though, since we check for $this->model, no?

Yeah, no. It was an example: even if we should never end up with assets with non existent models assigned, we check in other parts of the code so the system doesn't hard crash. The code that triggers the error is in the ReportsController:

And that was the case I was 'protecting' with the default model if no valid model is found. Sorry if I'm not being clear, I promise in my head everything made sense.

uberbrady

I would take this - ideally, we should never allow people to delete a model that is still in use, but obviously it has happened with at least one user, and I would prefer to not break their reports if it has.

There is the possibility of returning a closure that creates a pseudo-model with attributes of your choosing - and we could return a 'name' of "Deleted Model" in that case (or trans('deleted_model') or something else like that). That'd be nice too, but this, right now, seems to me to at least solve a real-world problem that a real user is actually having. (And, yes, I'd love to catch how people managed to do that, but in the end, we cannot prevent someone saying DELETE FROM models WHERE id=123 - which would cause something like this issue.

It's true that we don't do this elsewhere. But I still feel OK with it, I think.

snipe · 2023-07-20T16:34:06Z

@inietov what if we tried something like this instead:

Current:

snipe-it/app/Models/Asset.php

Lines 391 to 394 in d26bc19

    
           public function depreciation() 
        
           { 
        
               return $this->model->belongsTo(\App\Models\Depreciation::class, 'depreciation_id'); 
        
           }

Proposed:

    public function depreciation()
    {
        return $this->hasOneThrough(\App\Models\Depreciation::class,\App\Models\AssetModel::class,'id','id','model_id','depreciation_id');
    }

I don't have data enough to test this, but when I use model_id that doesn't exist, this just leaves the model blank. I need to test and see that it doesn't break the other places depreciation happens through.

If this looks like a better option, @inietov and @uberbrady, I can put up a PR for testing.

uberbrady · 2023-07-20T16:42:31Z

@inietov what if we tried something like this instead:

This looks more correct to me. We might even be able to get rid of all the get_depreciation() calls completely and just ask for $asset->depreciation too (without parentheses, since this is a 'normal' relation now) which will be a little bit less code, and will look a little cleaner.

…e model is busted Related: #13297 Signed-off-by: snipe <snipe@snipe.net>

inietov · 2023-07-20T16:46:49Z

If it leaves the model blank it works exactly as adding withDefault(), I'm not trying to be purposely dense, I am genuinely curious about why hasOneTrough() is a better solution?

snipe · 2023-07-24T10:36:10Z

hey @snipe anyway to find this easily? I have 14 000 assets?

@jayavman I'd do a full custom report and look for blank fields where they shouldn't be. A blank asset model, for example. Since asset models are required, it would be weird to have a blank value there. Should be pretty easy in Excel to sort by that column and see what looks funny.

Check if model exist before trying to return relationship

89e69dc

inietov requested review from uberbrady and snipe July 12, 2023 00:39

probot-autolabeler bot added the backend label Jul 12, 2023

inietov added 2 commits July 11, 2023 20:47

Add default value to asset-model relationship

a5553a9

Remove guard clause in asset-model-depreciation relationship

26e066c

uberbrady approved these changes Jul 19, 2023

View reviewed changes

snipe mentioned this pull request Jul 20, 2023

Check that $this->get_depreciation() returns #13346

Merged

snipe added a commit that referenced this pull request Jul 20, 2023

Possible alternative to fixing #13296 - custom report failing when th…

540b09c

…e model is busted Related: #13297 Signed-off-by: snipe <snipe@snipe.net>

snipe mentioned this pull request Jul 20, 2023

Possible alternative to fixing #13296 - custom report failing when th… #13347

Merged

inietov closed this Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #13296 Generating Custom Report Error #13297

Fixed #13296 Generating Custom Report Error #13297

inietov commented Jul 12, 2023 •

edited

Loading

what-the-diff bot commented Jul 12, 2023

snipe commented Jul 12, 2023

jayavman commented Jul 12, 2023 •

edited

Loading

snipe commented Jul 13, 2023

jayavman commented Jul 13, 2023

inietov commented Jul 14, 2023

snipe commented Jul 15, 2023

snipe commented Jul 15, 2023

jayavman commented Jul 15, 2023

inietov commented Jul 17, 2023

snipe commented Jul 18, 2023

inietov commented Jul 18, 2023

uberbrady left a comment

snipe commented Jul 20, 2023

uberbrady commented Jul 20, 2023

inietov commented Jul 20, 2023

snipe commented Jul 24, 2023

Fixed #13296 Generating Custom Report Error #13297

Fixed #13296 Generating Custom Report Error #13297

Conversation

inietov commented Jul 12, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

what-the-diff bot commented Jul 12, 2023

PR Summary

snipe commented Jul 12, 2023

jayavman commented Jul 12, 2023 • edited Loading

snipe commented Jul 13, 2023

jayavman commented Jul 13, 2023

inietov commented Jul 14, 2023

snipe commented Jul 15, 2023

snipe commented Jul 15, 2023

jayavman commented Jul 15, 2023

inietov commented Jul 17, 2023

snipe commented Jul 18, 2023

inietov commented Jul 18, 2023

uberbrady left a comment

Choose a reason for hiding this comment

snipe commented Jul 20, 2023

Current:

Proposed:

uberbrady commented Jul 20, 2023

inietov commented Jul 20, 2023

snipe commented Jul 24, 2023

inietov commented Jul 12, 2023 •

edited

Loading

jayavman commented Jul 12, 2023 •

edited

Loading