feat: preserve OR-group tokens in tags DTO#130
Open
AlejandroAkbal wants to merge 1 commit intomainfrom
Open
Conversation
Contributor
WalkthroughThe tag parsing logic in booru queries is simplified by removing pipe-character splitting. Tags are now normalized to trimmed strings without expanding on pipes, treating comma-delimited groups as single tokens. Test cases updated to reflect the new parsing behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Stop splitting tags on | so that OR-group tokens (e.g. cat|dog) are
forwarded intact to the Universal Booru Wrapper, which applies the
correct engine-specific OR syntax per booru engine.
Pipelines the change through the full stack:
App → | (user types 'cat OR dog')
API → ['cat|dog'] (kept as OR-group token)
Wrapper → '{ ~ cat ~ dog }' (Gelbooru) / '~cat ~dog' (Danbooru/e621)
AlejandroAkbal
force-pushed
the
auto-triage/2-feature-or-search-api
branch
from
29ca6e6 to
ca0f45d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/dto/booru-queries.dto.spec.ts`:
- Around line 46-56: Add a new test in src/booru/dto/booru-queries.dto.spec.ts
that asserts pipe characters are not split when parsing tags: create an it()
case (e.g., "preserves pipe characters inside a single tag token") which sends a
GET to /dto-test/posts with a tags param containing an encoded pipe (e.g.,
tags=artist%3Afoo%7Cbar) and assert response.statusCode is 200 and body.tags
equals ['artist:foo|bar']; this will directly cover the regression for pipe
non-splitting and complement the existing "should normalize repeated tags..."
test.
In `@src/booru/dto/booru-queries.dto.ts`:
- Around line 189-191: The mapper currently coerces non-string tags via
String(tag) which masks bad inputs; update the transformation so non-strings are
left unchanged and only strings are trimmed. Concretely, replace the map that
does (typeof tag === 'string' ? tag : String(tag)) with logic that returns tag
as-is when typeof tag !== 'string', and only apply .trim() when typeof tag ===
'string' (e.g. map(tag => typeof tag === 'string' ? tag.trim() : tag)) so
`@IsString`({ each: true }) can properly reject non-string values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cb6b83f-28cc-4a71-bb2d-822fc840b34e
📒 Files selected for processing (2)
src/booru/dto/booru-queries.dto.spec.tssrc/booru/dto/booru-queries.dto.ts
Comment on lines
+46
to
56
| it('should normalize repeated tags query params and keep each OR-group token intact', async () => { | ||
| const response = await app.inject({ | ||
| method: 'GET', | ||
| url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%7Crating%3Asafe&tags=score%3A%3E100' | ||
| url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%2Crating%3Asafe&tags=score%3A%3E100' | ||
| }) | ||
|
|
||
| const body = JSON.parse(response.body) | ||
|
|
||
| expect(response.statusCode).toBe(200) | ||
| expect(body.tags).toEqual(['artist:foo', 'rating:safe', 'score:>100']) | ||
| expect(body.tags).toEqual(['artist:foo,rating:safe', 'score:>100']) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a direct regression test for pipe non-splitting
Given this PR removes pipe splitting, add one explicit case to assert | is preserved within a single tag token.
Suggested additional test
+ it('should not split tags containing pipe characters', async () => {
+ const response = await app.inject({
+ method: 'GET',
+ url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%7Crating%3Asafe'
+ })
+
+ const body = JSON.parse(response.body)
+
+ expect(response.statusCode).toBe(200)
+ expect(body.tags).toEqual(['artist:foo|rating:safe'])
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should normalize repeated tags query params and keep each OR-group token intact', async () => { | |
| const response = await app.inject({ | |
| method: 'GET', | |
| url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%7Crating%3Asafe&tags=score%3A%3E100' | |
| url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%2Crating%3Asafe&tags=score%3A%3E100' | |
| }) | |
| const body = JSON.parse(response.body) | |
| expect(response.statusCode).toBe(200) | |
| expect(body.tags).toEqual(['artist:foo', 'rating:safe', 'score:>100']) | |
| expect(body.tags).toEqual(['artist:foo,rating:safe', 'score:>100']) | |
| }) | |
| it('should normalize repeated tags query params and keep each OR-group token intact', async () => { | |
| const response = await app.inject({ | |
| method: 'GET', | |
| url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%2Crating%3Asafe&tags=score%3A%3E100' | |
| }) | |
| const body = JSON.parse(response.body) | |
| expect(response.statusCode).toBe(200) | |
| expect(body.tags).toEqual(['artist:foo,rating:safe', 'score:>100']) | |
| }) | |
| it('should not split tags containing pipe characters', async () => { | |
| const response = await app.inject({ | |
| method: 'GET', | |
| url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=artist%3Afoo%7Crating%3Asafe' | |
| }) | |
| const body = JSON.parse(response.body) | |
| expect(response.statusCode).toBe(200) | |
| expect(body.tags).toEqual(['artist:foo|rating:safe']) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/booru/dto/booru-queries.dto.spec.ts` around lines 46 - 56, Add a new test
in src/booru/dto/booru-queries.dto.spec.ts that asserts pipe characters are not
split when parsing tags: create an it() case (e.g., "preserves pipe characters
inside a single tag token") which sends a GET to /dto-test/posts with a tags
param containing an encoded pipe (e.g., tags=artist%3Afoo%7Cbar) and assert
response.statusCode is 200 and body.tags equals ['artist:foo|bar']; this will
directly cover the regression for pipe non-splitting and complement the existing
"should normalize repeated tags..." test.
Comment on lines
+189
to
+191
| return (Array.isArray(value) ? value : [value]) | ||
| .map((tag) => (typeof tag === 'string' ? tag : String(tag))) | ||
| .map((tag) => tag.trim()) |
Contributor
There was a problem hiding this comment.
Avoid coercing non-string tags before validation
Line 190 converts non-string values with String(tag), which can let malformed inputs pass validation as "[object Object]". Keep non-strings as-is so @IsString({ each: true }) can reject them.
Suggested fix
- return (Array.isArray(value) ? value : [value])
- .map((tag) => (typeof tag === 'string' ? tag : String(tag)))
- .map((tag) => tag.trim())
+ return (Array.isArray(value) ? value : [value]).map((tag) =>
+ typeof tag === 'string' ? tag.trim() : tag
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return (Array.isArray(value) ? value : [value]) | |
| .map((tag) => (typeof tag === 'string' ? tag : String(tag))) | |
| .map((tag) => tag.trim()) | |
| return (Array.isArray(value) ? value : [value]).map((tag) => | |
| typeof tag === 'string' ? tag.trim() : tag | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/booru/dto/booru-queries.dto.ts` around lines 189 - 191, The mapper
currently coerces non-string tags via String(tag) which masks bad inputs; update
the transformation so non-strings are left unchanged and only strings are
trimmed. Concretely, replace the map that does (typeof tag === 'string' ? tag :
String(tag)) with logic that returns tag as-is when typeof tag !== 'string', and
only apply .trim() when typeof tag === 'string' (e.g. map(tag => typeof tag ===
'string' ? tag.trim() : tag)) so `@IsString`({ each: true }) can properly reject
non-string values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
API pass-through change for Feature: OR search (73 votes).
Enables OR-group tags — encoded as comma-separated alternatives (e.g.
cat%2Cdog) — to pass through thetagsDTO transform as single array elements, so the wrapper can translate them to engine-specific OR syntax.How it works
The
tags@Transformpreviously split on|to get individual tag strings. That split has been removed; each pipe-delimited segment in the URL is already a distincttagsquery parameter value. The transform now only handles trimming and URL-decoding per element.An OR group like
cat,dogarrives as a singletags[]element and is forwarded intact to the wrapper, which emits{ ~ cat ~ dog }for Gelbooru-family engines (or~cat ~dogfor e621/Danbooru-family).Changes
src/booru/dto/booru-queries.dto.ts— remove.flatMap(split('|')), keep trim + URL-decodesrc/booru/dto/booru-queries.dto.spec.ts— update OR-group tests to use comma encodingDepends on
Part of
Summary by CodeRabbit
Release Notes