-
Notifications
You must be signed in to change notification settings - Fork 284
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
Fix for #2202. Introduce support for @wholeBodyParam UDA in vibe.d's web framework. #2203
base: master
Are you sure you want to change the base?
Conversation
What's the advantage over what's implemented in #1723? With 2.082 we will have UDAs for function attributes, so it can be: void postFoo(@bodyParam MyStruct foo) {} |
Yeah I think having |
Hi Sebastian, The thing is @bodyParam("params") didn't work for me for a method from the web interface i.e. in this case the web framework looks for params_member in the form parameters and seems there is no support to match the member name directly (without the prefix). And I though this was intentional based on the comments in #1676 also having in mind that @bodyParam had worked this way for a long time it seemed that changing this behaviour is undesired. Because of the latter I came to the conclusion that the cleanest way to support what I needed was to introduce a new UDA. If it is better to only have the @bodyParam I think I can easily make @bodyParam("params") to work for the web framework the same way as it works for the Rest framework but as I said above this seemed like a breaking change to me. Finally it seems there is some difference in the meaning of @bodyParam and @wholeBodyParam which in my understanding is if the name of the parameter itself should be looked for in the body or not and this will be even more relevant if we can put UDAs on parameters. Best regards, |
Ah sorry. I confused that #1723 implemented this in master only for #1723 (and my own vibe.d fork actually does support this - similar to what I proposed in #1676).
I'm not sure introducing yet another UDA is a good idea - especially given that `@bodyParam("foo") already works for the REST part, so it will probably be a bit unintuive for users.
I don't see how this would be a breaking change, because currently
Yeah I saw a few issues that people opened because of this too. Though I think the missing/scarce documentation problem won't be magically be solved by
+1 (but that's something for a different PR). |
Hi Sebastian, In that case could you please share your implementation so to see if it works for my case and if so I'd like to try to push it through. If indeed it is not a breaking change this is what makes the most sense to me. Best regards, |
Sorry, it's not on GitHub yet, but it looks similar to what #1676 did for the REST part.
The breaking change proposed in #1676 would have been to automatically serialize the body into a parameter without attributes. As long as we still use
FYI: https://dlang.org/changelog/2.082.0.html#uda-function-arguments |
I did find an older version of this on GH though. In https://github.com/teamhackback/hb-web/blob/master/source/hb/web/web.d#L772 |
This is a preliminary (without tests and documentation) pull request just to illustrate a possible implementation of a fix for #2202.
Please comment if there is a better way to implement this and also if it is worth trying to fix this in the first place.