Skip to content

feat: preserve OR-group tokens in tags DTO #130

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.

Sign up for GitHub

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

Open
AlejandroAkbal wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: preserve OR-group tokens in tags DTO #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/booru/dto/booru-queries.dto.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,34 +31,34 @@ describe('booruQueryValuesPostsDTO request handling', () => {
await app.close()
})

it('should rely on request parsing for percent-decoding and split tags by pipe', async () => {
it('should rely on request parsing for percent-decoding and preserve comma OR-groups', async () => {
const response = await app.inject({
method: 'GET',
url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=panty_%26_stocking_with_garterbelt%7Crating%3Asafe'
url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=panty_%26_stocking_with_garterbelt%2Cblue_hair'
})

const body = JSON.parse(response.body)

expect(response.statusCode).toBe(200)
expect(body.tags).toEqual(['panty_&_stocking_with_garterbelt', 'rating:safe'])
expect(body.tags).toEqual(['panty_&_stocking_with_garterbelt,blue_hair'])
})

it('should normalize repeated tags query params and split each entry', async () => {
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'])
})
Comment on lines +46 to 56
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

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

🧹 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.


it('should reject empty tags created after pipe splitting', async () => {
it('should reject empty tags produced by repeated query params', async () => {
const response = await app.inject({
method: 'GET',
url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=tag1%7C'
url: '/dto-test/posts?baseEndpoint=gelbooru.com&tags=tag1&tags='
})

expect(response.statusCode).toBe(400)
Expand Down
6 changes: 3 additions & 3 deletions src/booru/dto/booru-queries.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ export class booruQueryValuesPostsDTO extends booruQueriesDTO {
return value
}

return (Array.isArray(value) ? value : [value]).flatMap((tag) =>
typeof tag === 'string' ? tag.trim().split('|') : [tag]
)
return (Array.isArray(value) ? value : [value])
.map((tag) => (typeof tag === 'string' ? tag : String(tag)))
.map((tag) => tag.trim())
Comment on lines +189 to +191
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

})
@IsOptional()
readonly tags: IBooruQueryValues['posts']['tags']
Expand Down