feat: add AOT path parameter support#2209
Draft
TimothyMakkison wants to merge 37 commits into
Draft
Conversation
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow on from #2182
Hey, here's my most recent take on adding path parameter support, don't know which one you prefer. I reckon I could modify Main to support object parameters and
**paramtypes, and possibly query params if needed, this may have some issues, I can elaborate as needed. I haven't bothered to rebase this PR onto main yet as I'm not sure if it's still wanted or what issues need to be resolved.I'd be curious to see what the performance difference is between this and the current approach, my benchmarks suggest this uses a little less memory but might be slower due to dictionary lookups, plus I don't trust my laptop to do microbenchmarks during a heatwave. The advantage of adding each parameter individually is that we can prevent boxing of value types and potentially use
ISpanFormattablewhenDefaultUrlFormatteris used for allocation free writes, ie an integer parameter with 2 digits will likely save 24+32 bytes with this approach, not a massive saving.ValueStringBuilderHow to handle the use of
ValueStringBuilderEmit
Emit
ValueStringBuilderas a inner class ofGenerated, would also have to emitAddPathParameterandAddPathObjectPropertyas they need access to it. This would make testing function that rely on the emittedValueStringBuildervery tricky.Expose
Make
ValueStringBuilderpublic, this means that people who useRefitwill have access to it. Not a great idea IMOInner class
Make
ValueStringBuilderan inner class ofGenerated, this way all emitted code can still reference it. We would have to makeGeneratedRequestRunneran inner class ofGeneratedso it can seeValueStringBuilder, this way users can't accessValueStringBuilderbut we can still write tests.Reflection
My approach to reflection is continued from #1965 where I've tried to make
ICustomAttributeProvideridentical to the runtime generated version. As I imagine some people internally casted the types back intoParameterInfoIf my understanding of AOT is correct, my current approach should safely access all reflection data, even if generics are used.
I did experiment with a version which didn't accept generic arrays and just used the generic argument count, but wouldn't close the method to correct generic type.
I could use #2174 approach, as you seem to be okay with this. This will have an edge case where it will break in the rare cases where a parameter has a generic attribute, however this can be fixed.
Failing tests
How to handle the failing query tests, most of the tests are failing because they expect query values alongside object path parameters.
Generated code: