Skip to content

Conversation

@ryanwalker
Copy link

@ryanwalker ryanwalker commented Sep 8, 2025

Fixed an issue when extracting tools where if the query param value was an array, it would not get serialized by axios properly.

For example, if I need to specify the fields to return for an api call, I want this:

  • ?fields=value1,value2,value3

But it was doing this:

  • ?fields[]=value1&fields[]=value2&fields[]=value3

This fix checks if the query param value is an array, and if it is, it joins them into a comma separated list.

Summary by CodeRabbit

  • Bug Fixes
    • Arrays passed as query parameters are now sent as comma-separated values, improving compatibility with APIs that expect CSV lists. Non-array parameters unchanged. Users should see more reliable filtering and pagination when supplying multiple values.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 8, 2025

Walkthrough

The executeApiTool function in src/utils/security.ts now serializes array-valued query parameters into comma-separated strings, while non-array values are passed unchanged. A clarifying inline comment was added. No exports or public interfaces were modified.

Changes

Cohort / File(s) Summary
Query parameter serialization update
src/utils/security.ts
Adjusted query param handling: arrays are filtered for non-null/undefined, converted to strings and joined into comma-separated values; scalars unchanged. Added inline documentation comment. No control-flow or public API signature changes.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Caller
    participant Util as executeApiTool
    participant HTTP as HTTP Client

    Caller->>Util: call executeApiTool(params)
    loop Build query
        Util->>Util: for each key/value in params.query
        alt value is Array
            Note right of Util #E6F7FF: New behavior — filter null/undefined,\nstringify elements, join with ","
            Util->>Util: value = value.filter(...).map(String).join(",")
        else value is not Array
            Util->>Util: value unchanged
        end
    end
    Util->>HTTP: send request with serialized query
    HTTP-->>Util: response
    Util-->>Caller: result
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (3 passed)

✅ Passed Checks (3 passed)
Check Name Status Explanation
Title Check ✅ Passed The title succinctly and accurately describes the key change of serializing array query parameters into comma-separated strings, making it clear and directly related to the changeset.
Description Check ✅ Passed The description clearly explains the issue, provides concrete examples of the incorrect and desired serialization behavior, and outlines the implemented fix, giving sufficient context and rationale for reviewers.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Poem

I nibble code like clover leaves, neat—
Arrays now whisper, comma-sweet.
Queries line up, hop in a row,
A tidy warren of params to go.
Thump! The request bounds swift and light. 🐇

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  - Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.
  - Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fed8311 and 54603a9.

📒 Files selected for processing (1)
  • src/utils/security.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/utils/security.ts
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link
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: 0

🧹 Nitpick comments (2)
src/utils/security.ts (2)

415-416: Harden serialization: filter nullish, stringify items.

Prevents "1,,3" and "[object Object]" leaks when arrays contain null/undefined or non-primitives.

-                queryParams[param.name] = Array.isArray(value) ? value.join(',') : value;
+                queryParams[param.name] = Array.isArray(value)
+                  ? value
+                      .filter((v) => v !== undefined && v !== null)
+                      .map((v) => String(v))
+                      .join(',')
+                  : value;

415-416: Optional: honor OpenAPI style/explode when available.

If definition.executionParameters carries style/explode, switch between CSV (explode=false) and repeated params (explode=true), keeping current CSV as default when unspecified.

Would you like me to add a small helper serializer that chooses CSV vs repeated params per parameter (without adding deps)?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f29c277 and fed8311.

📒 Files selected for processing (1)
  • src/utils/security.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/security.ts (2)
examples/pet-store-sse/src/index.ts (1)
  • executeApiTool (730-1048)
examples/pet-store-streamable-http/src/index.ts (1)
  • executeApiTool (730-1048)
🔇 Additional comments (1)
src/utils/security.ts (1)

415-416: CSV serialization for array query params — good fix.

This directly addresses axios’ default array/bracket serialization and matches the PR intent.

@sergionsz
Copy link

The format ?fields[]=value1&fields[]=value2&fields[]=value3 is not wrong. Nor is ?fields=value1,value2,value3. I actually need ?fields=value1&fields=value2&fields=value3. It depends on how the server handles query parameters, really
I think ideally we should have a setting to control the behavior of the serializer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants