papsign / Ktor-OpenAPI-Generator

Ktor OpenAPI/Swagger 3 Generator
Apache License 2.0
241 stars 42 forks source link

Allow explicit class references when building routes #63

Closed davidhiendl closed 4 years ago

davidhiendl commented 4 years ago

When building routes with reflection instead of manually registering them it is not possible to use reified DSL because it is not available at runtime using reflection. This can be fixed by only using reified for DSL methods and passing the class instance as parameters. This would allow external code to build routes from for example something like this:

@Controller("/test")
class TestController {
    @Throws(TestException::class, TestExceptionHandler::class) 
    @Route("info", RouteHttpMethod.GET, "example info", "example description")
    fun testRoute(params: String, body: String, ctx: APIRequestContext): String {
        return params
    }
}

There is no impact to the current DSL and there should be no performance impact, since this only affects the route building.

Wicpar commented 4 years ago

People can use Requests, Responses and Parameters with specialized generic classes like List<T>, and your code gets the star projected type of the class wich would erase T. It is necessary to change the code to use KType instead of KClass or this will create a regression.

davidhiendl commented 4 years ago

Ah right, I forgot about that. I've only been using explicit response classes. I will update this PR later today to fix that plus add some more unit tests that will cover such cases.

Wicpar commented 4 years ago

Some parts of this library still provide and use KClass instances, they should be (and eventually will be) changed to use KType as well. Don't hesitate to convert them if convenient.

davidhiendl commented 4 years ago

Alright, already got a working version with a mix of the two. I will replace it entirely then.

davidhiendl commented 4 years ago

I've replaced the KClass usages with KType.

I've also restricted access to several non-DSL methods to internal as they can now be used with non-matching generic types and KType which would result in errors (preHandle/handle). This should not affect the API at all.

I've also removed the inlining of the route building logic, this is only executed once at application startup which means the performance impact of inlining these methods in almost non-existent however it leads to massive increase in generated classes which can actually make a negative performance and memory impact (I've experienced that myself with a application with around ~120 API endpoints).

The tests are passing and I've added a few additional ones, but I'm not convinced that the tests are sufficient to ensure proper functionality. I'll update my current project to use the new library now and check a few actual uses.

Wicpar commented 4 years ago

Alright, it looks good to me on a first glance, it didn't change any behaviour fundamentally as far as i can see. I'll test this later today, i still have an interrogation regarding the usage of jvmErasure, maybe using .classifier as KAnnotatedElement would be more appropriate. Maybe even allowing type annotations for the KType so one could override those on a per-endpoint basis. But Type annotations still seem a bit unstable at the moment.

davidhiendl commented 4 years ago

I tried to preserve the existing DSL as much as possible. When using annotations per Endpoint (eg. per "handler" method) you would have to use the KParameter from that function to detect parameter annotations. It is possible to refactor it this way, but perhaps this should be done with the proposed syntax changes in #10 Alternatively I would be willing to contribute the "Annotation Controller" idea (see example in PR description) I'm implementing for my project on top of this one. My idea is to remove any function invocation for route building and instead do it 100% based on annotations and attached handlers including auto-discovery of controllers and registering them with Ktor using class path scans.

Wicpar commented 4 years ago

You can annotate the types like this get<@Annotation Type, @Annotation Type2> {} and the KType will provide the annotation in the current code.

Be careful with package discovery through reflections, it tends to be really slow. I used to work with spring and it suffered greatly under that, with up to 15 minutes load times on larger-ish projects. Even the package scan i use for the 7-odd classes takes 200ms to load... I would get rid of it if it wasn't necessary to provide default modules automatically and conveniently.

davidhiendl commented 4 years ago

Ah I forgot about that one, I was thinking about annotations for method parameters. Perhaps it is worth supporting it as it would allow classes outside of the project to be used.

I wouldn't use reflection for it, I would use class path scanning: https://github.com/classgraph/classgraph It can avoid actually loading the classes into the classloader. I've been using it for several projects with great results. It could also be optional, allowing manual registration if desired.

Wicpar commented 4 years ago

Alright, currently I use reflections, maybe classgraph is faster. i'll give it a shot. But that is what caused spring to take so long to initialise.

davidhiendl commented 4 years ago

I found it to be a very good alternative. Let me know if this PR needs any more changes. I've updated my current project to use the code from this PR already and so far it looks good and significantly reduced compile time and bundle size for the api module (~9mb jar to ~3mb). Finally a properly documented API :)