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.

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

Studio Tagger #3510

Merged
merged 42 commits into from
Jul 30, 2023
Merged

Studio Tagger #3510

merged 42 commits into from
Jul 30, 2023

Conversation

Flashy78
Copy link
Contributor

@Flashy78 Flashy78 commented Mar 7, 2023

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.

@@ -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
Copy link
Contributor Author

@Flashy78 Flashy78 Mar 7, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@WithoutPants WithoutPants left a comment

Choose a reason for hiding this comment

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

Code review is still in progress.

graphql/schema/types/scraper.graphql Outdated Show resolved Hide resolved
graphql/schema/types/scraper.graphql Outdated Show resolved Hide resolved
@WithoutPants WithoutPants added the feature Pull requests that add a new feature label Mar 7, 2023
if obj.Name.Valid {
return obj.Name.String, nil
}
panic("null name") // TODO make name required
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

newStudio := models.Studio{
Checksum: checksum,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed CreatedAt/UpdatedAt are enforced at the sqlite level.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

internal/identify/studio.go Outdated Show resolved Hide resolved
},
nil,
true,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

excluded_fields: input.ExcludeFields,
task_type: Performer,
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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_studio.go Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
}
return nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Flashy78 Flashy78 changed the title Studio image Studio Tagger Mar 7, 2023
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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to test these as they were removed.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Flashy78 and others added 13 commits June 22, 2023 20:22
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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
    }
}

@WithoutPants WithoutPants merged commit a665a56 into stashapp:develop Jul 30, 2023
2 checks passed
@Flashy78 Flashy78 deleted the studio-image branch October 8, 2023 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull requests that add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Studio Tagger with Image Support
3 participants