Convert group aliases to list - #6951
Conversation
|
|
||
| // movieResolver.Aliases overrides groupResolver.Aliases to return a single string | ||
| // for backward compatibility with the deprecated Movie type. | ||
| func (r *movieResolver) Aliases(ctx context.Context, obj *models.Group) (*string, error) { |
There was a problem hiding this comment.
Only basic changes done to movie related codebase to ensure existing stuff doesn't break & code compiles correctly.
There was a problem hiding this comment.
This can be repurposed to be on groupResolver when you update the graphql fields.
| } | ||
|
|
||
| // GetAliases provides a mock function with given fields: ctx, relatedID | ||
| func (_m *GroupReaderWriter) GetAliases(ctx context.Context, relatedID int) ([]string, error) { |
There was a problem hiding this comment.
Copied from StudioReaderWriter / PerformerReaderWriter
| `alias` varchar(255) NOT NULL, | ||
| foreign key(`group_id`) references `groups`(`id`) on delete CASCADE | ||
| ); | ||
|
|
There was a problem hiding this comment.
multiple groups can have same aliases, this is not considered a problem, hence no UNIQUE INDEX.
Gykes
left a comment
Gykes
left a comment
There was a problem hiding this comment.
Didn't do a full review, just a quick glance while I have a little time to kill.
Feel free to educate me on anything if I misunderstood your intent.
There is a problem though. We display aliases as comma-separated. Here, even though - B, C, AA - all 3 are different aliases, they are displayed as comma separated. This may be a visual problem when displaying group names containing commas. 
Clearly here |
There was a problem hiding this comment.
Another quick review. Please update any documentation that would be required as well.
EDIT: Per your UI question. I cant think of a good way around it. Users should probably just understand what their aliases are and know what it ascually means. Outside of wrapping aliases in quotes it just seems like a small UI thing people will need to get used to.
| ) SELECT | ||
| `id`, | ||
| `aliases` | ||
| FROM `groups` | ||
| WHERE `aliases` IS NOT NULL AND TRIM(`aliases`) != ''; | ||
|
|
||
| ALTER TABLE `groups` DROP COLUMN `aliases`; No newline at end of file |
There was a problem hiding this comment.
Here you are using !=''. I'm pretty sure SELECT would take the raw value. If the user has leading or trailing whitespace would it not get regected by the ValidateAliases?
Off the top of my head I cant remember if we removed whitespaces from Aliases. I remember doing it for various other things a while ago. Would need some verification.
There was a problem hiding this comment.
For both studios and performers, the aliases are cleaned up, ie, leading & trailing whitespaces removed and duplicates removed.
I will need to check a bit if we really need it in the db query though.
There was a problem hiding this comment.
Updated, TRIM is not really required. Only DB constraint we have is non-null. Others are small issues which will be fixed on next group update anyhow.
There was a problem hiding this comment.
ValidateAliases only runs during create/update. Not during read. During reads, UI will show the aliases without being trimmed. Not an issue in my opinion.
There was a problem hiding this comment.
For migration 42 where we changed performer aliases from string to string list, we had a post-migration to separate comma-delimited aliases into alias entries. It should be applied here as well.
I think adding != '' to the insert where clause is a good sanity check. Perhaps not necessary, but it doesn't hurt.
babechan233
left a comment
babechan233
left a comment
There was a problem hiding this comment.
- Update docs (only single place)
- Updated mockery to allow service layer mocks generation
- Added resolver test for aliases
| filename: "{{.InterfaceName}}.go" | ||
| outpkg: mocks | ||
| interfaces: | ||
| GroupService: |
There was a problem hiding this comment.
More service interfaces exist but mocks for those can be generated as and when required.
There was a problem hiding this comment.
These changes should largely not be necessary if we remove the unit test from the api package.
| github.com/vektah/dataloaden v0.3.0 | ||
| github.com/vektah/gqlparser/v2 v2.5.27 | ||
| github.com/vektra/mockery/v2 v2.10.0 | ||
| github.com/vektra/mockery/v2 v2.21.6 |
There was a problem hiding this comment.
Updated to use packages field in .mockery.yml
DogmaDragon
left a comment
DogmaDragon
left a comment
There was a problem hiding this comment.
Documentation check passed.
| @@ -0,0 +1,40 @@ | |||
| package testutil | |||
There was a problem hiding this comment.
This file was completely generated by LLM. I understand what it is doing but I dont understand if/whether the solution it utilizes is idiomatic or even the "right" solution. Will request greater reviewer attention here.
There was a problem hiding this comment.
I'd remove this and the unit test. We do need to add testing to the graphql layer, but I think it's probably better to do it with integration testing and not something within this scope.
| id: ID! | ||
| name: String! | ||
| aliases: String | ||
| aliases: [String!] |
There was a problem hiding this comment.
This change breaks compatibility with existing external systems. You will need to deprecate the existing aliases field and add a new alias_list field instead. The existing field will need a resolver to build the string value.
We did the same with performer aliases before we removed the deprecated field. See https://github.com/WithoutPants/stash/blob/f65e87773c4b9fb053f4c5f23bcb77c03adbbccd/internal/api/resolver_model_performer.go#L21
Will need to apply this to the other alias field changes.
|
|
||
| // movieResolver.Aliases overrides groupResolver.Aliases to return a single string | ||
| // for backward compatibility with the deprecated Movie type. | ||
| func (r *movieResolver) Aliases(ctx context.Context, obj *models.Group) (*string, error) { |
There was a problem hiding this comment.
This can be repurposed to be on groupResolver when you update the graphql fields.
| @@ -0,0 +1,40 @@ | |||
| package testutil | |||
There was a problem hiding this comment.
I'd remove this and the unit test. We do need to add testing to the graphql layer, but I think it's probably better to do it with integration testing and not something within this scope.
| ) SELECT | ||
| `id`, | ||
| `aliases` | ||
| FROM `groups` | ||
| WHERE `aliases` IS NOT NULL AND TRIM(`aliases`) != ''; | ||
|
|
||
| ALTER TABLE `groups` DROP COLUMN `aliases`; No newline at end of file |
There was a problem hiding this comment.
For migration 42 where we changed performer aliases from string to string list, we had a post-migration to separate comma-delimited aliases into alias entries. It should be applied here as well.
I think adding != '' to the insert where clause is a good sanity check. Perhaps not necessary, but it doesn't hurt.
| return " ORDER BY (" + selectGroupOCountSQL + ") " + direction | ||
| } | ||
|
|
||
| func (qb *GroupStore) GetAliases(ctx context.Context, groupId int) ([]string, error) { |
There was a problem hiding this comment.
Please add a unit test to exercise this function.
| case "tags": | ||
| groupRepository.tags.leftJoin(f, "tags_join", "groups.id") | ||
| f.addWhere("tags_join.group_id IS NULL") | ||
| case "aliases": |
There was a problem hiding this comment.
Please add unit test cases to TestGroupQuery to cover this.
| filename: "{{.InterfaceName}}.go" | ||
| outpkg: mocks | ||
| interfaces: | ||
| GroupService: |
There was a problem hiding this comment.
These changes should largely not be necessary if we remove the unit test from the api package.
|
Branch has merge conflicts. |
* inital version * fix oshash caps * change oshash to sum rather than length * Remove unrelated stash-box generated model changes
* Fix A/V desync after seeking when video is stream-copied (stashapp#7103) When a live transcode stream-copies the video while re-encoding the audio, ffmpeg's accurate seek trims the decoded audio exactly to the -ss position, but the copied video can only start at the preceding keyframe, leaving the audio ahead of the video by up to a GOP for the rest of playback. Disable accurate seek when the selected video codec is copy, so that both streams start together at the keyframe. Playback will resume at the preceding keyframe, up to one GOP before the requested position.
* Use LEFT JOIN when filtering for NULL phash * Add unit test scenario --------- Co-authored-by: WithoutPants <53250216+WithoutPants@users.noreply.github.com>
* Fix case-insensitive scan lookup for files * Fix literal path matching in SQLite lookups
…dary (stashapp#7083) * Fix lightbox jumping to a page's last image when crossing a page boundary When a paginated gallery's lightbox crosses a page boundary, the nav handlers (handleLeft/handleRight) call pageCallback to fetch the adjacent page and reset the index to that page's edge. Two races made this land on the wrong image: - The handlers run on raw keydown/click events that can arrive faster than React re-renders, so they read a stale isSwitchingPage (and index) from their closure — the page-switch guard didn't actually block rapid presses/clicks. - pageCallback was called before the handler set its guard and reset the index. When the adjacent page is already cached it swaps images in synchronously, producing a render with the old index against the new, shorter page; the index-range effect then clamped that stale index to images.length-1, leaving the lightbox on the last image of the new page (e.g. 37/37 instead of 1/37 on a 77-image gallery) rather than its first. Reconcile the index off the page prop instead: the parent updates page and images together, so a page change is a race-free signal to land on the new page's first image (forward) or last (backward); the out-of-range clamp now only runs for a genuine same-page shrink (e.g. a deleted image). Keep a synchronous ref mirroring isSwitchingPage solely to drop rapid inputs mid-switch, and set it (and the target index) before pageCallback so a cached page can't outrun it. Affects both keyboard and on-screen (chevron / image-edge) navigation. * Rework page-switch fix to keep the index handler-controlled Addresses review feedback (@CynicalAtropos): the previous commit reconciled the landing index off the page-number direction, which regressed wraparound and chapter navigation. A wrap moves the page number opposite to the navigation direction (backward from the first image wraps the page forward to the last page, and vice versa), so a page-direction-keyed index lands on the new page's first/last image instead of the gallery's global last/first; and a chapter jump's intended in-page index was overridden too. Keep the landing index under the handlers' control instead, and close the two races directly: - Back the page-switch guard with a synchronous ref (isSwitchingPageRef) and gate handleLeft/handleRight on it, so rapid input mid-switch is dropped even when events arrive faster than React re-renders. - Set the ref and the target index before pageCallback, so an already-cached page that swaps images in synchronously can't outrun them. - Gate the index-range clamp on the same ref, so it can never clamp a stale index while a switch is in flight; it now runs only for a genuine same-page shrink (e.g. a deleted image). This is what makes the clamp-to-last impossible by construction rather than by commit-ordering luck. The -1 sentinel resolves to the new page's last image (backward), 0 to its first (forward), and indexInPage is preserved for chapter jumps, so first/last wraparound and chapter navigation are unaffected. Verified with Playwright across Chromium, Firefox and WebKit: the forward/back/forward boundary round-trip lands on the new page's first image at every timing, and the backward/forward wraps land on the gallery's last/first image. * Trigger the page-switch settle on the page number, not array identity The rework keyed the settle on the images array identity (images !== oldImages.current). That reference can be stale relative to the live images when a switch begins: the initial empty-list open consumes the initial isSwitchingPage flag, and a reopen leaves oldImages pinned to a prior array. The settle then fires prematurely on the old page, keeps the old index, and clears isSwitchingPage; when the new page arrives the flag is already false, so the -1 landing sentinel is never resolved and renders as a blank image at a (page-1)*size + 0 counter (e.g. 40/77 instead of 77/77 on a backward wrap). Trigger the settle on the page NUMBER changing instead — the parent swaps page + images together, so a changed page number reliably means the new page is present — and resolve an explicit landing target ("first"/"last"/a chapter index) recorded by the handler. The target is never derived from the page-number direction, so first/last wraparound and chapter jumps stay correct. No -1 sentinel is pre-set; the index is parked at 0 (valid on any non-empty page) during the switch so the index-range clamp can't fire on a shorter incoming page. * Harden the page-switch settle and gate deletion mid-switch - Settle a page switch only when the page number AND the images array have both changed, so a parent that delivers the page a commit ahead of the images cannot settle the switch against the outgoing page's array. Callers that provide pageCallback without a page prop fall back to the images identity alone instead of freezing. - Clamp resolved chapter targets to the arrived page, so a stale chapter index lands on the page's last image instead of leaking out of range. - Centralize the order-sensitive switch-start sequence in a startPageSwitch helper and the paired ref/state writes in a setSwitching setter, and store the forward landing as index 0 instead of a "first" sentinel. - Ignore the d-d delete shortcut while a switch is in flight (the parked index would target an image the user isn't viewing), and capture the delete dialog's image when it opens so an index change while the dialog is up can no longer retarget the confirmation. - Surface gallery page-load failures via a toast instead of an unhandled rejection (useGalleriesLightbox.loadPage had no .catch). --------- Co-authored-by: void-function865 <void-function865@users.noreply.github.com>
* Moving generate buttons to Set Image control - Removed direct thumbnail generation options from Scene dropdown. - Added onGenerateThumbFromCurrent and onGenerateThumbDefault props to SceneEditPanel for handling thumbnail generation. - Updated ImageInput component to include buttons for generating thumbnails from the current image and a default image. Co-authored-by: Cursor <cursoragent@cursor.com> * Move reset button into SetImage button * Update ImageInput component to use a minimal button style for file selection * Update sceneGenerateScreenshot mutation to return job ID instead of string - Changed return type of sceneGenerateScreenshot from String! to ID! - Updated implementation to return the job ID generated during screenshot creation. * Refactor ImageInput component to replace horizontal rules with styled divs - Updated ImageInput component to use a custom styled div for menu dividers instead of <hr> elements. - Added corresponding styles in index.scss for the new divider class. * Update set-image-menu-divider to include text color * Refactor SceneEditPanel to streamline ImageInput component usage - Removed conditional rendering of ImageInput for editing, simplifying the code. - Updated props for onGenerateDefault and onGenerateCurrent to only trigger when not a new scene. - Adjusted onReset logic to check for cover_image in formik values before allowing reset action. * Add styles for button alignment in set-image popover - Introduced new styles for .btn.minimal to enhance layout and alignment. - Updated .fa-icon styles within the minimal button for better icon presentation. * Enhance Scene component with screenshot job monitoring and refresh functionality - Added useMonitorJob to track screenshot job completion and handle success/error states. - Implemented onRefreshScene callback to refresh scene data after screenshot generation. - Updated SceneEditPanel to conditionally enable reset action based on cover image state. * Add screenshot generated message to en-GB locale - Included a new localization string for "Screenshot generated" in the English (UK) locale file to enhance user feedback after screenshot creation. * Formatting updated for CI validation - Simplified Toast success message formatting in Scene.tsx for better clarity. - Condensed onGenerateDefault prop assignment in SceneEditPanel.tsx to enhance code readability. * Refactor localization in SceneEditPanel and ImageInput components - Replaced intl.formatMessage calls with FormattedMessage components for better integration with React Intl in SceneEditPanel.tsx and ImageInput.tsx. * Refactor SceneEditPanel and ImageInput to support extra actions - Introduced extraActions prop in ImageInput for customizable button actions, replacing onGenerateDefault and onGenerateCurrent. - Updated SceneEditPanel to utilize new extraActions for generating thumbnails, enhancing flexibility and code clarity. - Improved component structure for better maintainability and user experience. * Fix from file option not highlighting on hover --------- Co-authored-by: KennyG <kennyg@kennyg.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: WithoutPants <53250216+WithoutPants@users.noreply.github.com>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.50.0 to 0.55.0. - [Commits](golang/net@v0.50.0...v0.55.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-version: 0.55.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/image](https://github.com/golang/image) from 0.38.0 to 0.41.0. - [Commits](golang/image@v0.38.0...v0.41.0) --- updated-dependencies: - dependency-name: golang.org/x/image dependency-version: 0.41.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.51.0 to 0.52.0. - [Commits](golang/crypto@v0.51.0...v0.52.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.52.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Add saved filter loaded plugin hook * Add filter tag extras plugin hook
Some recommended complexity rules default to info level which does not error out during lint. Changed these to emit errors instead. Fixed existing failing issues.
Shows popover for matched performer and warns on mismatched performers
* Fix lightbox browser history handling * Fix lightbox history dismissal
* fix: normalize macOS NFD filenames to NFC when scanning macOS filesystems report filenames in NFD (decomposed) form, e.g. "が" is stored as "か" plus a combining mark. SQLite compares strings byte-wise, so searching for the composed (NFC) form that users type does not match the decomposed form stored during scanning. Normalize scanned paths to NFC before storing them. This is only done on macOS, whose filesystems are normalization-insensitive, so the on-disk file is still found when accessed via its NFC path. Applying it on normalization-sensitive filesystems (e.g. Linux) would prevent files with NFD names on disk from being found. To keep behaviour consistent and avoid duplicates when rescanning an existing library: - path containment checks (library membership, generated folder) are made normalization-insensitive - root paths are normalized to match the stored paths - file and folder rename/move detection treats an existing entry whose path differs only by normalization as the same entry, so its path is updated in place rather than creating a duplicate Entries inside zip files are matched byte-exact and are left unnormalized. Fixes stashapp#4425
* Add support for sorting images by performer's age * Add image performer age sort test and document date requirement * Move performer age sort docs to Images.md --------- Co-authored-by: Gykes <Gykes@pm.me>
* Make the scene, performer and tag merge modals patchable Registers SceneMergeModal, PerformerMergeModal and TagMergeModal with the plugin API and adds their modules to loadableComponents, so that plugins providing their own pages can load and render them * Remove unused onClose() call in Performer/SceneMergeModal SceneMergeModal and PerformerMergeModal called onClose(id) on success and then onClose() again unconditionally, which is unnecessary so we remove that call
StudioFragment selected only the parent's name and id, so resolveStudio issued a FindStudio query per scene whose studio has a parent. Those are serialised by the client's own rate limiter at one request per 250ms, which dominated the runtime of a multi-scene scrape: around two thirds of scenes have a parented studio, so a 40 scene batch spent roughly seven seconds waiting on lookups it could have avoided. Select the parent's fields in the fragment instead. resolveStudio becomes a pure conversion with no round trips, and a batch costs one query.
17 participants
Description
Group aliases are currently stored as a single string on the
groupstable. This PR migrates them to a dedicated join table (group_aliases), matching the existing pattern used by performers, studios and tags.Changes:
group_aliasesjoin tableGroup.aliasesin the GraphQL schema changed to[String!]Related Issue
Closes #5593
Testing
Screenshots
Before:
After:
Checklist
AI Usage Disclosure
Additional Context
Migration note: I recommend that existing alias be added as-is without comma splitting, as group names can legitimately contain commas. Doing the comma-splitting ourselves adds burden and also messes up the data in case of genuine comma. Users who were storing comma-separated aliases will need to split them manually after upgrading though.