Skip to content

fix(api): parsing of custom string types#4180

Merged
GAlexIHU merged 2 commits intomainfrom
fix/api-parsing
Apr 27, 2026
Merged

fix(api): parsing of custom string types#4180
GAlexIHU merged 2 commits intomainfrom
fix/api-parsing

Conversation

@GAlexIHU
Copy link
Copy Markdown
Contributor

@GAlexIHU GAlexIHU commented Apr 20, 2026

Overview

Adds support for parsing filter strings into custom string types

Notes for reviewer

Summary by CodeRabbit

  • New Features

    • Filter parsing now accepts pointer-to-named-string fields as valid filter targets; empty values leave those pointers unset. Operator-style filter keys for these fields are rejected.
  • Tests

    • Added tests verifying named-string pointer parsing, nil-on-absent behavior, and rejection of operator-style keys for both named-string pointers and existing string-pointer fields.

@GAlexIHU GAlexIHU requested a review from a team as a code owner April 20, 2026 10:11
@GAlexIHU GAlexIHU added the release-note/bug-fix Release note: Bug Fixes label Apr 20, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5e130bed-06b2-4cbc-b983-e3f330c01660

📥 Commits

Reviewing files that changed from the base of the PR and between 26c114d and 947bb28.

📒 Files selected for processing (2)
  • api/v3/filters/parse.go
  • api/v3/filters/parse_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • api/v3/filters/parse.go
  • api/v3/filters/parse_test.go

📝 Walkthrough

Walkthrough

parseFiltersValue now accepts pointer-to-named-string fields (e.g., *T where T's underlying kind is string) and parses filter[field]=value into a newly allocated typed pointer. Operator-style keys like filter[field][op] are detected and rejected for these fields.

Changes

Cohort / File(s) Summary
Core parsing logic
api/v3/filters/parse.go
Added parseStringPtrTyped to parse pointer-to-named-string fields. parseFiltersValue now recognizes *T (named string) and routes parsing to the new helper; added hasOperatorStyleKeys to detect and reject operator-style keys for these fields.
Test coverage
api/v3/filters/parse_test.go
Added type testStringType string and TxType *testStringType to testFilter. Added TestParse_NamedStringType to verify parsing and nil behavior, and TestParse_StringPtrOperatorRejected to assert operator-style keys are rejected for *string fields.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • turip
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding support for parsing custom string types in filters.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/api-parsing

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@GAlexIHU GAlexIHU enabled auto-merge (squash) April 20, 2026 10:12
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
api/v3/filters/parse_test.go (1)

307-320: Tiny coverage add: lock down the empty-value branch.

The new helper intentionally leaves the typed pointer unset for filter[tx_type]=; adding that subtest would make the behavior explicit.

🧪 Proposed test addition
 func TestParse_NamedStringType(t *testing.T) {
 	t.Run("named string type filter value", func(t *testing.T) {
 		var f testFilter
 		require.NoError(t, Parse(url.Values{"filter[tx_type]": {"funded"}}, &f))
 		require.NotNil(t, f.TxType)
 		assert.Equal(t, testStringType("funded"), *f.TxType)
 	})
 
+	t.Run("nil when filter value is empty", func(t *testing.T) {
+		var f testFilter
+		require.NoError(t, Parse(url.Values{"filter[tx_type]": {""}}, &f))
+		assert.Nil(t, f.TxType)
+	})
+
 	t.Run("nil when no filter key present", func(t *testing.T) {
 		var f testFilter
 		require.NoError(t, Parse(url.Values{}, &f))
 		assert.Nil(t, f.TxType)
 	})
 }

As per coding guidelines, **/*_test.go: “Make sure the tests are comprehensive and cover the changes.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/v3/filters/parse_test.go` around lines 307 - 320, Add a subtest inside
TestParse_NamedStringType to cover the empty-value branch: call Parse with
url.Values{"filter[tx_type]": {""}} against a fresh testFilter, require no
error, and assert that f.TxType is nil (similar to the existing "nil when no
filter key present" case). This makes the behavior for Parse, testFilter, TxType
and the named type testStringType explicit when the filter key is present but
has an empty value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/v3/filters/parse.go`:
- Around line 168-176: The branch that handles named string pointer fields (the
block that calls parseStringPtrTyped when fieldVal.Kind()==reflect.Pointer &&
fieldVal.Type().Elem().Kind()==reflect.String) must not silently accept
operator-style keys like filter[tx_type][eq]; detect if the query string map
contains any keys of the form name+"[...]" (i.e., keys that start with the field
name plus '[') and if so return a clear error (e.g., fmt.Errorf("filter[%s]:
unsupported operator-style key", name)) instead of calling parseStringPtrTyped,
and apply the same operator-rejection logic to the analogous block later (the
similar handling around lines 202-221) so operator-style filters for named
string pointers are rejected consistently.

---

Nitpick comments:
In `@api/v3/filters/parse_test.go`:
- Around line 307-320: Add a subtest inside TestParse_NamedStringType to cover
the empty-value branch: call Parse with url.Values{"filter[tx_type]": {""}}
against a fresh testFilter, require no error, and assert that f.TxType is nil
(similar to the existing "nil when no filter key present" case). This makes the
behavior for Parse, testFilter, TxType and the named type testStringType
explicit when the filter key is present but has an empty value.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 93dd77df-eb1c-4350-90bd-3596f4c04434

📥 Commits

Reviewing files that changed from the base of the PR and between fd00eee and 26c114d.

📒 Files selected for processing (2)
  • api/v3/filters/parse.go
  • api/v3/filters/parse_test.go

Comment thread api/v3/filters/parse.go
@turip turip requested a review from tothandras April 20, 2026 10:41
@GAlexIHU GAlexIHU merged commit 33fd73b into main Apr 27, 2026
25 checks passed
@GAlexIHU GAlexIHU deleted the fix/api-parsing branch April 27, 2026 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/bug-fix Release note: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants