这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 15, 2025

Path Parameter Serialization Issue - Successfully Resolved ✅

  • Analyze the codebase and identify the root cause of original issue
  • Create and validate fix for path parameter serialization
  • Resolve merge conflicts with main branch
  • Create proper merge commit with main
  • All tests passing

Merge Completed

Successfully created a proper merge commit (aef86a1) with the latest main branch (commit 40cc96c). The branch history now correctly reflects the merge relationship with main.

Changes Applied

  1. Proper merge with main - Created an actual merge commit with two parents
  2. Reapplied flat params fix - The core fix for path parameter serialization is applied
  3. Updated test snapshots - Regenerated snapshots to reflect the corrected output

Core Fix

When paramsStructure === 'flat', fields are correctly wrapped in an args array:

if (plugin.config.paramsStructure === 'flat') {
  const fieldShapes = [];
  // Collect all field shapes
  for (const field of opParameters.fields) {
    fieldShapes.push(shape);
  }
  // Wrap in args array
  const argsWrapper = $.object();
  argsWrapper.prop('args', $.array(...fieldShapes));
  config.push(argsWrapper);
}

This ensures buildClientParams correctly iterates over flattened parameter properties instead of treating the entire object as a positional argument.

Test Results

✅ All SDK tests pass (2/2)
✅ Snapshot updated for flat params test
✅ Build successful
✅ Proper merge commit created with two parents

Files Modified

  • packages/openapi-ts/src/plugins/@hey-api/sdk/shared/operation.ts - Core flat params fix
  • packages/openapi-ts-tests/sdks/__snapshots__/opencode/flat/sdk.gen.ts - Updated snapshot

The path parameter serialization bug is now completely fixed with a proper merge commit!

Original prompt

This section details on the original issue you should resolve

<issue_title>Path parameter incorrectly serialized as /servers/id, instead of /servers/ on paramsStructure flat</issue_title>
<issue_description>### Description

Hi, I encountered an issue when calling (with paramsStructure flat) a generated GET method.
The request path is incorrectly serialized as:
/servers/id,1335279038599663729

But it should be:
/servers/1335279038599663729

This looks like the {id} placeholder is not replaced properly and the value is appended with a comma.

Let me know if you need any further details or if I can provide additional logs or config.

Reproducible example or configuration

import { defineConfig } from '@hey-api/openapi-ts';

export default defineConfig({
	input: './scripts/openapi-spec.json',
	output: { path: './src/api/client', importFileExtension: '.js' },
	plugins: [
		'@hey-api/schemas',
		{
			name: '@hey-api/typescript',
			topType: "any",
			enums: {
				case: 'preserve',
				enabled: true,
				mode: 'typescript'
			}
		},
		{
			name: '@hey-api/sdk',
			asClass: true,
			responseStyle: 'data',
			paramsStructure: 'flat',
			classNameBuilder: (name) => `${name}Service`
		}
	]
});

OpenAPI specification (optional)

    "/servers/{id}": {
      "get": {
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "schema": {
              "type": "string"
            },
            "required": true
          }
        ],
        "responses": {
          "200": {
            "description": "Ok",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/IServer"
                }
              }
            }
          }
        },
        "tags": [
          "Servers"
        ],
        "security": [],
        "operationId": "GetServer"
      },

System information (optional)

@hey-api/openapi-ts: 0.87.5</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2025

⚠️ No Changeset found

Latest commit: aef86a1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Nov 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
hey-api-docs Ready Ready Preview Comment Nov 19, 2025 4:08am

When using paramsStructure: 'flat' with the SDK plugin, path parameters were being incorrectly serialized. The issue was that buildClientParams was treating the flat parameters object as a positional argument instead of iterating over its properties.

Fixed by wrapping field configs in an args array when paramsStructure is 'flat', which tells buildClientParams to iterate over the flattened object properties instead of treating the entire object as a single positional argument.

Co-authored-by: mrlubos <12529395+mrlubos@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix path parameter serialization issue in GET method Fix path parameter serialization with flat params structure Nov 15, 2025
Copilot AI requested a review from mrlubos November 15, 2025 03:01
Copilot finished work on behalf of mrlubos November 15, 2025 03:01
@fiftyy
Copy link

fiftyy commented Nov 15, 2025

Generated code now correctly wraps fields:

const params = buildClientParams([parameters], [
  {
    args: [
      { in: 'path', key: 'id' }
    ]
  }
]);

This tells buildClientParams to iterate over parameters properties and extract individual values, producing correct path parameter serialization.

Tested it by replacing my function with this. Works good.

@fiftyy
Copy link

fiftyy commented Nov 15, 2025

Can we trigger a test build with pkg-pr-new bot? @mrlubos

@CowCatDE
Copy link

I could use a build too @mrlubos

@mrlubos
Copy link
Member

mrlubos commented Nov 19, 2025

@copilot resolve conflicts

Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot resolve conflicts

I've encountered a blocking issue while resolving the conflicts. The main branch has a pre-existing bug introduced by the DSL refactoring (commit 4b5e471) that prevents the SDK tests from passing.

The Issue:
The code in operation.ts uses .coalesce() which returns a BinaryTsDsl object, but then tries to call .attr() on it. However, BinaryTsDsl doesn't have an .attr() method, causing a runtime error: "te.attr is not a function".

This affects the /event endpoint (and potentially others) where server-sent events are enabled, as the code tries to access .sse property on the binary expression result.

Status:

  • My original fix for the path parameter serialization bug is complete and working
  • However, I cannot successfully merge with main due to this pre-existing bug
  • The main branch tests also fail with the same error

Options:

  1. Fix the pre-existing DSL bug first (adds the .attr() method to BinaryTsDsl or finds another solution)
  2. Wait for the DSL bug to be fixed in main before merging
  3. Merge without running the failing tests (not recommended)

Would you like me to attempt fixing the DSL bug as well, or should we address it separately?

…g DSL bug)

Applied my flat params fix using the new $ DSL syntax from main. Also attempted to fix pre-existing bug where BinaryTsDsl from coalesce() doesn't have .attr() method, but this requires a deeper fix to the DSL itself.

Current status: My original fix is complete but cannot be fully tested due to the DSL bug in main branch.

Co-authored-by: mrlubos <12529395+mrlubos@users.noreply.github.com>
@mrlubos
Copy link
Member

mrlubos commented Nov 19, 2025

@copilot try again

Merged latest main (40cc96c) which includes the fixed DSL with BinaryTsDsl now having the .attr() method through ExprMixin. Reapplied the flat params serialization fix using the new DSL structure.

All tests pass including the SSE operations that previously triggered the DSL bug.

Co-authored-by: mrlubos <12529395+mrlubos@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot try again

Successfully resolved! The merge conflicts have been resolved with the latest main branch (commit 40cc96c). The DSL refactoring in main has fixed the BinaryTsDsl issue - it now properly has the .attr() method through ExprMixin.

My flat params serialization fix has been reapplied using the new DSL structure, and all tests are passing including the SSE operations that previously failed.

Changes in commit f59dfb0.

@mrlubos
Copy link
Member

mrlubos commented Nov 19, 2025

@copilot one more time

Proper merge commit to integrate latest main changes (40cc96c).
All tests pass.
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot one more time

Done! Created a proper merge commit (aef86a1) that correctly integrates the main branch history. The previous attempt didn't create an actual merge commit - it just copied the changes. Now the branch history shows the proper merge relationship with two parents.

All tests pass and the flat params fix is preserved.

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.

Path parameter incorrectly serialized as /servers/id,<id> instead of /servers/<id> on paramsStructure flat

4 participants