athena-framework / athena

An ecosystem of reusable, independent components
https://athenaframework.org
MIT License
211 stars 17 forks source link

Refactor Query/Request Parameter Support #418

Closed Blacksmoke16 closed 2 months ago

Blacksmoke16 commented 5 months ago

Athena currently supports query and request params via the logic within ATH::Params. It is somewhat of a one-off implementation, that adds a fair bit of complexity, and less than ideal DX given the annotations are applied to the controller action vs the specific param.

I'm planning on re-implementing this to tie into the argument resolver logic, where there will be an annotation to map a specific query parameter to a controller parameter, and one to map multiple query parameters into a DTO type. The latter of which can make use of validation constraints whereas the former cannot.

This new implementation will not have the same functionality as before, but I think this is an overall win to keep complexity down until a use case can be explored. But we shall see how it goes.

Blacksmoke16 commented 5 months ago

This is partially blocked by https://github.com/crystal-lang/crystal/issues/14680, which would make the query params => DTO process a lot easier. For now could still handle the refactor and specific query params, and have that come as a follow up.

ysbaddaden commented 5 months ago

@Blacksmoke16 please don't get blocked by stdlib, you can work on an experimental shard to implement this :)

Blacksmoke16 commented 5 months ago

Now that we're getting into where a single value resolver can potentially handle more than 1 annotation, I think I'd like to try and decouple the resolver's configuration annotation from the resolver type itself. Currently this is how the ATHR::Interface::Typed module maps to a specific resolver. One major benefit of doing this is allowing better named annotations. E.g. ATHA::MapRequestPayload vs the current ATHR::RequestBody::Extract.

https://github.com/crystal-lang/crystal/issues/9802#issuecomment-2171032485 would be the most ideal solution, but I have some alternatives that might do the trick until then.

Blacksmoke16 commented 5 months ago

Another point I realized while working on this is unlike some of the other more generic resolvers, if you apply say @[ATHA::MapQueryParameter], it is expected that that parameter be handled by the query parameter resolver; without trying the others.

Currently every resolver is iterated until one returns a value. This is a bit inefficient as by virtue of adding the annotation, it's clear which one should handle it. This is something that'll need to be figured out.