Studio Tagger #3510

Flashy78 · 2023-03-07T04:07:01Z

Resolves #3460

Adds all of the options from #3460 with the tagger code copied over from the Performer tagger.
Since the Performer backend was refactored recently, I ended up refactoring the Studio backend to match. I found it was easier to copy how Performer was done rather than try to copy the tagger frontend and squish it into the existing backend.

Tested identify, import/export, scene tagger, studio tagger, studio tagger batch functions.. all with or without accompanying parent studio.

…image

Flashy78 · 2023-03-07T04:24:39Z

go.mod

@@ -11,6 +11,7 @@ require (
 	github.com/disintegration/imaging v1.6.0
 	github.com/fvbommel/sortorder v1.0.2
 	github.com/go-chi/chi v4.0.2+incompatible
+	github.com/gofrs/uuid v4.4.0+incompatible


This allows for passing in a search query that can be a name or a stash id, and parse the text to find out which one.
It's the same library used on Stashbox.

internal/desktop/systray_nonlinux.go

WithoutPants

Code review is still in progress.

graphql/schema/types/scraper.graphql

Flashy78 · 2023-03-07T18:23:05Z

internal/api/resolver_model_studio.go

-	if obj.Name.Valid {
-		return obj.Name.String, nil
-	}
-	panic("null name") // TODO make name required


Name is required in the UI, but I don't think it's enforced anywhere. I will make a note to address that later.

Flashy78 · 2023-03-07T18:30:23Z

internal/api/resolver_mutation_studio.go

 	newStudio := models.Studio{
-		Checksum:  checksum,


I moved checksum and the date fields into model_studio.go NewStudio and NewStudioPartial to avoid duplicating it everywhere, but looking now that doesn't stop anyone from just making a new object manually without using those methods and missing out on the fields. I will review this further.

Confirmed CreatedAt/UpdatedAt are enforced at the sqlite level.

Flashy78 · 2023-03-07T18:34:50Z

internal/api/resolver_mutation_studio.go

-	if input.Details != nil {
-		newStudio.Details = sql.NullString{String: *input.Details, Valid: true}
+		rating := models.Rating5To100(*input.Rating)
+		newStudio.Rating = &rating
 	}
 	if input.IgnoreAutoTag != nil {
 		newStudio.IgnoreAutoTag = *input.IgnoreAutoTag
 	}



Image is now downloaded, but the saving to the db has been moved to pkg/sqlite/studio.go to avoid that duplicated block of code.
Same with saving the alias and stashid code blocks.

Flashy78 · 2023-03-07T18:38:14Z

internal/api/resolver_mutation_studio.go

+// This is slightly different to studioPartialFromStudioCreateInput in that Name is handled differently
+// and ImageIncluded is not hardcoded to true
+func studioPartialFromStudioUpdateInput(ctx context.Context, input StudioUpdateInput, id *string, translator changesetTranslator) (*models.StudioPartial, error) {
+	// Populate studio from the input


Probably a better way to avoid duplicating 98% of this code block.

internal/identify/studio.go

Flashy78 · 2023-03-07T18:45:24Z

internal/identify/studio_test.go

-			},
-			nil,
-			true,
-		},


With the removal of the UpdateStashIDs method, this test isn't really applicable.

Flashy78 · 2023-03-07T18:49:01Z

internal/manager/manager_tasks.go

+									excluded_fields: input.ExcludeFields,
+									task_type:       Performer,
+								})
+							}


Small refactor to load the StashIDs here instead of in the individual tasks. This way instead of adding a task for every single item and then deciding in the task if the user wanted to refresh existing or search for untagged, we are filtering here and only creating tasks for those items.

Flashy78 · 2023-03-07T18:50:44Z

internal/manager/manager_tasks.go

@@ -418,3 +440,132 @@ func (s *Manager) StashBoxBatchPerformerTag(ctx context.Context, input StashBoxB

 	return s.JobManager.Add(ctx, "Batch stash-box performer tag...", j)
 }
+
+func (s *Manager) StashBoxBatchStudioTag(ctx context.Context, input StashBoxBatchTagInput) int {
+	j := job.MakeJobExec(func(ctx context.Context, progress *job.Progress) {


Almost a straight up duplication of the above method. I would like to refactor them in a future update.

internal/manager/task_stash_box_tag.go

Flashy78 · 2023-03-07T18:55:26Z

internal/manager/task_stash_box_tag.go

+					r.hookExecutor.ExecutePostHooks(ctx, *updatedStudio.ParentID, plugin.StudioUpdatePost, input, parentTranslator.getFields())
+				}
+				r.hookExecutor.ExecutePostHooks(ctx, updatedStudio.ID, plugin.StudioUpdatePost, input, translator.getFields())
+			*/


Wondering if the hooks should be run during batch operations.
If so, they will need to refactored to be available to this package somehow.

pkg/models/model_scraped_item.go

pkg/models/model_studio.go

Flashy78 · 2023-03-07T19:05:27Z

pkg/models/studio.go

-	UpdateFull(ctx context.Context, updatedStudio Studio) (*Studio, error)
+	Create(ctx context.Context, input StudioDBInput) (*int, error)
+	UpdatePartial(ctx context.Context, input StudioDBInput) (*Studio, error)
+	Update(ctx context.Context, updatedStudio *Studio) error


Updated methods to match Performer, except Create returns the created id instead of accepting the studio as a pointer and updating in place. I've been hurt too many times by that pattern of not realizing my object is getting updated inside a method without me knowing, so I try to avoid it.

Reverted my changed for Create.
Should I revert UpdatePartial as well? What's the use case for passing in the id separate instead of as part of the partial model being updated?

pkg/models/timestamp_test.go

Flashy78 · 2023-03-07T19:09:54Z

pkg/sqlite/repository.go

-	}
-	return nil
-}
-


I guess Studio must have been the only one using this method as the linter wanted me to remove it.

Flashy78 · 2023-03-07T19:20:25Z

pkg/studio/export_test.go

-	mockStudioReader.On("GetStashIDs", ctx, studioID).Return(stashIDs, nil).Once()
-	mockStudioReader.On("GetStashIDs", ctx, noImageID).Return(nil, nil).Once()
-	mockStudioReader.On("GetStashIDs", ctx, missingParentStudioID).Return(stashIDs, nil).Once()
-


No need to test these as they were removed.

Flashy78 · 2023-03-07T19:21:51Z

pkg/studio/import.go

-	if err := i.ReaderWriter.UpdateAliases(ctx, id, i.Input.Aliases); err != nil {
-		return fmt.Errorf("error setting tag aliases: %v", err)
-	}
-


These were moved to pkg/sqlite/studio.go, but creating the image was left here.

Object structs should try to only include data that is used both for creation and querying. ImageBytes was not populated or used when reading studios. Removed from partial for consistency. This results in more code at the client end to set images, but this repeated pattern should probably be abstracted into a studio/service function, rather than included in the sqlite code.

WithoutPants · 2023-07-13T04:27:54Z

I've made a bunch of changes to the backend code.

I've removed the parent field from the create and update inputs. The front end code needs to be updated to separately create/update parent studios. Currently, the tagger hangs when creating a studio with parent info, because the server is returning an error and the error isn't being handled correctly. I'll investigate this myself tomorrow if you don't get to it in the meantime.

I've taken out the image data from the studio object and partial structs. It was a vestigial field on the studio struct, only used during creation. It's also inconsistent with the other objects/partials. It does result in more code on the calling side to set the image, but this can be mitigated in future by combining the create/update object and image code into a service-level function. I could have kept the image in the partial, but this would be inconsistent with the create behaviour.

I reverted some undocumented design decisions such as moving the timestamp setting code into the sqlite package. In principle, the sqlite package should be concerned only with querying and modifying data, and not with the logic. We want fine control of these timestamps outside the data layer, and as such should not have been moved here. The alias uniqueness checking should probably not be in sqlite either, but for now it's fine to stay there. In future it should be moved into the business layer. In future we should unify the create/update functions into a single function in the studio package, which would include this validation.

I also reverted a change that prevented aliases from being used in studio autotag. This looked like it was copied from the performer code, but the reasons for disabling aliases in the performer code do not apply to studios.

In future, I'd prefer if you could separate large refactors from new features. It makes reviewing far more difficult. In future, please also highlight low-level design decisions like the items mentioned above. These have far-reaching impacts on the design and behaviour, and detecting them with static code reviews are difficult.

Flashy78 · 2023-07-14T02:46:34Z

I've made a bunch of changes to the backend code.

In future, I'd prefer if you could separate large refactors from new features. It makes reviewing far more difficult. In future, please also highlight low-level design decisions like the items mentioned above. These have far-reaching impacts on the design and behaviour, and detecting them with static code reviews are difficult.

Thanks for taking the time to get this where it needs to be.

I will avoid new features and stick to bug fixes to avoid creating more work for you.

WithoutPants · 2023-07-14T02:48:55Z

I will avoid new features and stick to bug fixes to avoid creating more work for you.

This wasn't intended to discourage you from making new features! The overall change here is all good, there was just some issues around the refactoring that was done at the same time.

WithoutPants · 2023-07-14T02:51:19Z

I've done some refactoring of the UI code to handle creating/updating parent studios. I'd appreciate if you could give it a test run to ensure I haven't broken anything. It's looking about complete on my end.

Flashy78 · 2023-07-14T18:07:42Z

I was too overconfident in my ability to pick up and understand go. I will lurk longer to learn more about how the codebase is structured/designed (ie sqlite should only read/write with zero changes to the objects). I would rather write features that you can bug check/suggest changes, rather than having to spend days rewriting. In that case it would be more efficient just for you to write the feature whenever it appears on your list.

If I tag a studio, where the parent studio exists but has no stashId, it tries to create the parent instead of updating.

{
    "errors": [
        {
            "message": "inserting into studios: executing `INSERT INTO `studios`....: UNIQUE constraint failed: studios.name",
            "path": [
                "studioCreate"
            ]
        }
    ],
    "data": {
        "studioCreate": null
    }
}

Some cache seems to be getting messed up, which may be the cause above. I tried to reproduce and it wanted to create the parent. Refreshed the page and it correctly matched the parent and assigned the stashid.

Deleted stashId from Studio, deleted parent studio, navigate back to tagger, studio modal thinks the parent still exists and then

{
    "errors": [
        {
            "message": "studio with id 30 not found",
            "path": [
                "studioUpdate"
            ]
        }
    ],
    "data": {
        "studioUpdate": null
    }
}

Flashy78 added 6 commits February 19, 2023 15:17

Studio image and parent studio support in scene tagger

c7266b9

Merge branch 'develop' into studio-image

e8ab7da

Refactor studio backend and add studio tagger

0cdd331

Refactor studio backend and add studio tagger

784656c

Merge branch 'studio-image' of github.com:Flashy78/stash into studio-…

2c8eeca

…image

Merge branch 'develop' into studio-image

49e4621