godotengine / godot-asset-library

PHP frontend for Godot Engine's asset library
https://godotengine.org/asset-library
MIT License
284 stars 85 forks source link

Issue Approving Assets #319

Closed Gramps closed 4 months ago

Gramps commented 4 months ago

There has been a bug where some assets cannot be approved. Yuri had made some changes that worked on some previously stuck assets; I believe it was a collation problem, don't fully remember. However, I've been digging more into the now five assets that cannot be approved and, until that last PR, just gave a generic database error. Opening this issue to share some notes on my investigation into this.

The assets in question don't really seem to have any wildly different data but they are assets / edits:

After adding more specific error messages to the approval process, it seems that these all fail during the preview insertion process which ultimately stops the approval. The related code lives src/routes/asset_edit.php from lines 792 - 823 (as of 13/2/24); but this code is seemingly fine as it is used for assets that approve smoothly. Obviously the INSERT INTO statements are fine; again, as it works for other assets.

I tried to find a common point between these five asset edits; the only thing they all share are adding new previews (operation 0 / insert). And, again, none have any weird or special characters in their data.

I ran the SQL queries, as they are in the code, manually in a local database and they insert fine with no issues.

I was unable to duplicate the Asset Library locally, however, as the login page localhost:8080/login gives me:

Method not allowed. Must be one of: POST

I ran out of time to try to figure out how to fix that. Ideally I can get this running locally and actually see what data is being passed along and attempting to be inserted as something in there is going wrong.

Hopefully these notes are of some use.

Gramps commented 4 months ago

After @Piralein's info about the missing types and looking back at the database structure, why isn't there a default value for type? Since there is only image or video, why should it ever be null or empty? Obviously, I am not extremely familiar with this system so maybe there is a reason but if that is causing issues, I don't see why it should be allowed to ever be null or empty.

Piralein is also right that there is a mismatch between the as_asset_edit_previews and as_asset_previews tables on the type column; one with null and one without.

Manually correcting the null for edit 10801 allows it to be approved and to make matters a little more confusing to me, there appear to be some assets that have null as their type and were still able to be approved. For instance, edit id 10777 has null for the type but in the as_assets_preview table is is now video. So how did that get changed over to video from null?

From digging through asset_edit.php, this probably falls between lines 273 - 289 where type gets bound. I can see how that field ends up empty or null in this case.

foreach ($c->constants['asset_edit_preview_fields'] as $i => $field) {
    if (!$required || $field == 'thumbnail') { // thumb is not required
        if (isset($preview[$field]) && !(isset($original_preview) && $original_preview[$field] == $preview[$field])) {
            $has_changes = true;
            $query->bindValue(':' . $field, $preview[$field]);
        } elseif (!isset($original_preview)) {
            $query->bindValue(':' . $field, '');
        } else {
            $query->bindValue(':' . $field, null, PDO::PARAM_NULL);
        }

And, again, unless there is a reason for it, I think that adding a condition for type to make sure it is set and, if not, set it to image at least would help.

Looking back at where it gets approved, I have no idea how edit 10777 went from null to video during approval.

Gramps commented 4 months ago

This PR is untested but might work for this I never got the local copy working with login and haven't had time to mess with that more.

Gramps commented 4 months ago

The PR didn't break anything and should fix that in the future. Previously stuck assets were fix by changing the nulls to image so they could be approved.