fix: avoid faulty duplicate detection

[Mcklmo]

The bug is caused by registering operations with inline struct definitions with varying field names, combined with an empty OperationID.

I noticed this causing a runtime panic on app start. It is no actual security vulnerability, because it is not possible to trigger during app uptime - it only triggers during the initialisation phase. But it is an irritating error to me as a user, as the panic message does not really make sense to me.

I added a test for reproducability. To see the problem in action, just run the tests on your current version, without my fix to the code.

I fixed the error by allowing multiple inline struct definitions with varying field names. What would have caused a duplicate panic before now increments the would be duplicate by one. E.g., the first type will be called "Request", the second "Request1", and so on. That way the behavior is still deterministic and humanly readable in a generated spec. This change does not affect existing code bases because the condition the change has effect on was previously guarded by the runtime panic. I.e., no configuration can have existed that was able to initialise the app with this condition before. Hence, the change will be entirely backwards compatible.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.05%. Comparing base (99e60fd) to head (31f157a).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
huma.go	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #893      +/-   ##
==========================================
- Coverage   93.08%   93.05%   -0.03%     
==========================================
  Files          23       23              
  Lines        4769     4779      +10     
==========================================
+ Hits         4439     4447       +8     
- Misses        270      271       +1     
- Partials       60       61       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wolveix]

Hey @Mcklmo! Sorry this has sat for so long.

I agree that the underlying issue could be improved; however, I'm not sure this is the best approach. We generally avoid adding anything to our external interface definitions (see #925). Would you be open to reworking this to try to avoid this?

[Mcklmo]

Hi, I see your point. How about we pivot on no fix, but a more developer friendly error message. I could take a shot at that. Let me know what you think.

[wolveix]

Honestly while an improved error message might help, I think fixing the underlying issue would still be better if we can find an elegant way to implement it :D

[Mcklmo]

The underlying issue stems from the fact that empty OperationIDs are legal in the first place and the hint creation logic creates exact copies of the hints. The bug is quite rare, also because it only occurs if the operations are registered with input or output types defined as inline structs.

IMO, OperationIDs should be required and unique: that would fix the issue entirely. However, that would of course change the external API substantially, which I agree is a bad idea.

What I propose in the PR is a deterministic hint creation logic that only has effect for apps that currently don't exist, because they would panic on start. So I actually think that this a fix with an absolute minimal impact on the external API.

Do you have another idea how this could be approached?

[wolveix]

It is worth mentioning that since #937, operation IDs must be unique. Perhaps I'm slightly misunderstanding (just getting back from being away from a few weeks); would you mind providing a code sample that triggers this issue, please? Here's a base example I use for testing:

package main

import (
	"context"
	"fmt"
	"net/http"

	"github.com/danielgtaylor/huma/v2"
	"github.com/danielgtaylor/huma/v2/adapters/humachi"
	"github.com/danielgtaylor/huma/v2/humacli"
	"github.com/go-chi/chi/v5"

	_ "github.com/danielgtaylor/huma/v2/formats/cbor"
)

// Options for the CLI.
type Options struct {
	Port int `help:"Port to listen on" short:"p" default:"8888"`
}

func main() {
	// Create a CLI app which takes a port option.
	cli := humacli.New(func(hooks humacli.Hooks, options *Options) {
		// Create a new router & API
		router := chi.NewMux()
		cfg := huma.DefaultConfig("My API", "1.0.0")
		cfg.DocsRenderer = huma.DocsRendererScalar
		api := humachi.New(router, cfg)

		// Register GET /greeting/{name}.
		huma.Register(api, huma.Operation{
			OperationID: "get-greeting",
			Summary:     "Get a greeting",
			Method:      http.MethodGet,
			Path:        "/greeting/{name}",
		}, func(ctx context.Context, input *struct {
			Name string `path:"name" minLength:"1" maxLength:"255" pattern:"^(?:[^%]|%[0-9A-Fa-f]{2})+$" example:"lol" doc:"The path of the directory that is URL encoded"`
		}) (*struct{}, error) {
			return nil, nil
		})

		// Register GET /greeting2/{name}.
		huma.Register(api, huma.Operation{
			OperationID: "get-greeting-2",
			Summary:     "Get a greeting",
			Method:      http.MethodPost,
			Path:        "/greeting2/{name}",
		}, func(ctx context.Context, input *struct {
			Name string `path:"name" minLength:"1" maxLength:"255" pattern:"^(?:[^%]|%[0-9A-Fa-f]{2})+$" example:"lol" doc:"The path of the directory that is URL encoded"`
		}) (*struct{}, error) {
			return nil, nil
		})

		// Tell the CLI how to start your router.
		hooks.OnStart(func() {
			http.ListenAndServe(fmt.Sprintf(":%d", options.Port), router)
		})
	})

	// Run the CLI. When passed no commands, it starts the server.
	cli.Run()
}

[Mcklmo]

That actually sounds promising, I didn't know that. You can try running the unit tests I added to the PR. Back when I created the PR, they triggered the bug, before my fix.