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

Add support for path custom type conversion #539

Open
SentryMan opened this issue Jan 11, 2025 · 4 comments
Open

Add support for path custom type conversion #539

SentryMan opened this issue Jan 11, 2025 · 4 comments
Labels
enhancement New feature or request

Comments

@SentryMan
Copy link
Collaborator

SentryMan commented Jan 11, 2025

From the discord:

image

I'm thinking we could make it so that:

@Get("/something")
public void getMetadata(@QueryParam("id") Foo foo) {
  //
}

could generate something like:

    var id = asType(ctx.pathParam("id"), Foo::new);
    var result = controller.getMetadata(id);

Where asType would be a new method added that takes a reference to a constructor/valueOf method

@SentryMan SentryMan added the enhancement New feature or request label Jan 11, 2025
@rbygrave
Copy link
Contributor

Can we get more concrete examples of the types for this use case rather than Foo? I presume these are DDD types but let's get some good understanding of the motivation.

There is some interaction with the logic that determines what parameter is used for the body content. This is probably the main concern here - that we don't suffer in terms of handling the body parameter. This is somewhat because as avaje-http uses annotation processing we actually know the method parameter names and as such we then don't need explicit annotations for path parameters and query parameters ... and this then impacts the determination of which parameter is for the body.

Rightly or wrongly, we have the @BodyString annotation for the rare use case where the body is just a String. Calling that @BodyString instead of @Body almost feels like a mistake right at this moment.

@SentryMan
Copy link
Collaborator Author

This is probably the main concern here - that we don't suffer in terms of handling the body parameter.

this is simple to handle, this should only apply if it's a GET/DELETE HTTP verb, or if the parameter is explicitly annotated with @QueryParam/@Header/Other

@SentryMan
Copy link
Collaborator Author

Calling that @BodyString instead of @Body almost feels like a mistake right at this moment.

I'm indifferent, but since we're jumping major versions to 3.0 you have the opportunity to change it.

@rbygrave
Copy link
Contributor

rbygrave commented Jan 27, 2025

Can we get more concrete examples of the types for this use case rather than Foo? I presume these are DDD types but let's get some good understanding of the motivation.

We still need this. The description uses Foo which does not actually express the motivation.

In addition the discord example is not valid as it uses {foo} as a path parameter.

Ideally we get the description of this issue updated so that we clearly understand the motivation and use case.

Edit: My guess is that this is desired to support both query parameters and path parameters, it would be good to clarify exactly what this feature request is for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants