axum icon indicating copy to clipboard operation
axum copied to clipboard

Add support for generics to debug_handler macro

Open slessans opened this issue 2 years ago • 5 comments

This pull requests adds support for generics in the debug_handler macro as described here.

Motivation

https://github.com/tokio-rs/axum/issues/1253

Solution

I added a Specializer type that is able to produce all specializations of generic types in a function declaration given a set of specializations for each parameter. This PR basically modified all existing checks (where relevant) to produce a check for each specialized version of the function.

slessans avatar Aug 16 '22 23:08 slessans

@jplatte I believe this is ready for review. Thanks for all the feedback so far!

slessans avatar Sep 28 '22 20:09 slessans

@jplatte I think I have addressed everything and am pretty happy with how things are. A few small things to address:

  1. I did a big merge yesterday before I saw your comment to rebase -- would you still like me to rebase this or is squashing and merging the current changes fine
  2. I think the name Specializer is probably due for an update to like GenericItemFn or something but I don't have a great name right now off-hand and would like to get this merged before more conflicts, so if you are OK with it I can address this in a follow-up PR once this is merged and I have a better name for it

slessans avatar Oct 20 '22 18:10 slessans

would you still like me to rebase this or is squashing and merging the current changes fine

No need to rebase, merge is fine.

Regarding the name "specializer", I don't find it too bad.

jplatte avatar Oct 20 '22 19:10 jplatte

Absolutely awesome work here @slessans. I'll try and take a look this weekend!

davidpdrsn avatar Oct 20 '22 19:10 davidpdrsn

@davidpdrsn gentle ping on this, just want to get it merged soon to minimize conflicts since it turned into such a big PR. 🙏

slessans avatar Oct 28 '22 21:10 slessans

@davidpdrsn @jplatte would it be helpful if i split this into a series of smaller PRs?

slessans avatar Nov 27 '22 19:11 slessans

Sorry for taking so long to look at this. I should get time Friday next week 😊

davidpdrsn avatar Nov 27 '22 20:11 davidpdrsn

ping @davidpdrsn , any updates?

MichaelScofield avatar Jan 09 '23 12:01 MichaelScofield

Okay first of all sorry for taking an eternity to answer. That's entirely my fault.

The reason I've been hesitant to reply is that I'm kinda conflicted about these changes. On the one hand I really appreciate the work you've done, on the other I'm not fully sure this feature is worth the additional maintenance cost.

While I think #[debug_handler] supporting generics is neat, I can't remember a time that I needed that myself. Nor have I heard from more than a few people asking about it on Discord or on GitHub. But it's not like there is zero interest either, we did get some +1s here and on the issue.

We (and by that I mean myself) should probably have more carefully considered this feature when the issue was opened, before you started implementing it.

davidpdrsn avatar Apr 07 '23 20:04 davidpdrsn

Given what I said above I think I'll close this for now. @slessans sorry about wasting your time 😞

davidpdrsn avatar Apr 14 '23 11:04 davidpdrsn

In my current project I need generics for debug_handler and I ended up here. IMHO I think this feature would bring great flexibility and help the axum ecosystem grow. I hope this can be reconsidered in the future.

vaniusrb avatar Jun 03 '23 01:06 vaniusrb

Going to add my voice of support for this feature. Not having support for generics in debug_handler has been a huge pain in my team's side.

cdata avatar Jan 09 '24 21:01 cdata

The macro code does not depend on private things in axum's API. I think personally I would prefer axum itself to provide this too, but it would be possible to publish a separate crate that provides an extended version of the debug_handler attribute macro.

jplatte avatar Jan 09 '24 21:01 jplatte

The macro code does not depend on private things in axum's API. I think personally I would prefer axum itself to provide this too, but it would be possible to publish a separate crate that provides an extended version of the debug_handler attribute macro.

We're trying to update to Axum 0.7+ and in the process, one of the routes is no longer satisfying trait bounds; specifically a stream-up/stream-down route updated to Body from previous StreamBody/BodyStream, where other routes that either stream-up or stream-down are fine, suspecting some !Send across await.

As the routes use generics, something like this PR is very appealing! As the issue only surfaced while upgrading to 0.7, I attempted to use this specialized debug_handler (changing axum/core dependencies to use their 0.7 equivalents), and some changes may be needed in order to work with 0.7:

#[axum_macros::debug_handler(with(T = String))]
pub async fn test_route<T>(string: T) -> Result<(), StatusCode> {
    Ok(())
}
trait takes at most 2 generic arguments but 3 generic arguments were supplied
expected at most 2 generic arguments

mod.rs(79, 11): trait defined here, with at most 2 generic parameters: `S`, `M`
push.rs(50, 1): replace the generic bound with the associated type: `Rejection = `

Thanks all for the work here! Some questions:

  • Any pointers for updating this for Axum 0.7?
  • Would nested generics #[debug_handler(with(T = String; U = Box<T>))] be debuggable with this macro?

jsantell avatar Jan 12 '24 20:01 jsantell

Would nested generics #[debug_handler(with(T = String; U = Box<T>))] be debuggable with this macro?

I think it might work if written the other way around (if replacements are done in order): #[debug_handler(with(U = Box<T>, T = String))]. But would there be a problem with expanding things fully, as in #[debug_handler(with(T = String, U = Box<String>))]?

jplatte avatar Jan 26 '24 22:01 jplatte