Skip to content
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.

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

proposal: add base path to config #356

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

Conversation

Yurovskikh
Copy link

Hey, I tried to improve the mechanism for adding a base path. If this draft is fine for humachi and humago I would add it for another routers

Based on this PR and comment #305 (comment)

@danielgtaylor
Copy link
Owner

@Yurovskikh thanks for the PR! Can you help me understand the need for this change? The existing behavior is described at https://huma.rocks/features/bring-your-own-router/#route-groups-base-urls which seems to cover the various use-cases you might need. Can you help me understand what's confusing about it?

This PR looks okay, but I'd like to:

  1. Avoid adding things to the config that are already possible via other means (e.g. config.Servers = ...)
  2. Prevent ambiguity around whether a base path feature would apply to the router at API creation time and how that might interact with the servers slice.

Mainly trying to understand why we need this.

@Yurovskikh
Copy link
Author

Yes, Huma's documentation describes this use case, but I think explicit is better than implicit.
Setting the base path from the list of servers, as well as adding it to the route group in Huma, seems to me a less convenient way than setting it once in the config via the base path

Also, it now works differently for different routes: for some routes I should use groups, but for the default http router I have to add them to the constructor.
https://github.com/danielgtaylor/huma/blob/main/adapters/humago/humago.go#L151
This is not a problem, but it may be more consistent for any routers

@danielgtaylor
Copy link
Owner

@Yurovskikh is the idea that this would only impact the API routes themselves and not the docs/openapi/schemas paths? I just wonder if this is more confusing than explicitly setting the servers and passing in the router group or prefix.

Also, some of the routers already have explicit functions for this, for example see:

Maybe this could be improved with more such utility functions and some docs instead of another way to set the base path?

@Yurovskikh
Copy link
Author

Yes, this is mainly about API routes, because we declare the API using huma and the router itself just acts as a provider of this scheme.
That’s why I want to reduce work with the router to a minimum
Maybe I'm just misunderstanding the idea. I just saw the same misunderstanding in the issue and tried to improve it

@cardinalby
Copy link

cardinalby commented Jun 27, 2024

Sorry for spamming around, wanted to highlight that it's just another example where more general approach I'm suggesting in #489 (having already ready and tested code) can help to solve this and similar issues with one simple conception instead of multiple local changes trying to cover all the cases

No need to add these multiple features to the core project, we can make it modular allowing users add these modifiers if they need (I already provided most common once in the library)

Just define a group with base path.

v1gr := api.            // all operations registered with v1gr will have:
	AddBasePath("/v1").                            // - "/v1" base path

You can still use api instance that will not add base path to the operations.

Or even

// all operations registered with multiGr will be actually registered twice: with v1 and v2 prefix,
// operationIDs and summaries will be regenerated to correspond the paths to avoid conflicts
multiGr := api.WithMultiBasePaths(nil, "/v1", "/v2")   

Middlewares, transformers, tags, security entries, ... etc can be added for a group, and all the operations registered with this group will have them.

Regarding OpenAPI endpoints, they can be defined manually in the same manner as other operations with possibility to add auth middlewares to them

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.

3 participants