Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dbankov-vmware
Copy link

@dbankov-vmware dbankov-vmware commented Aug 15, 2018

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.

@wilzbach
Copy link
Member

What's the advantage over what's implemented in #1723?
E.g. @bodyParam("foo")

With 2.082 we will have UDAs for function attributes, so it can be:

void postFoo(@bodyParam MyStruct foo) {}

@Geod24
Copy link
Contributor

Geod24 commented Aug 15, 2018

Yeah I think having @bodyParam only is better.
But it seems to come up a lot that people don't want the second argument and get confused by it.
One thing we should do is better document it. Another could be to deprecate the second argument, given its probably not widely used (and easy to replace with just a struct containing a struct).

@dbankov-vmware
Copy link
Author

dbankov-vmware commented Aug 16, 2018

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,
Dentcho Bankov

@wilzbach
Copy link
Member

The thing is @bodyParam("params") didn't work for me for a method from the web interface

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).

Because of the latter I came to the conclusion that the cleanest way to support what I needed was to introduce a new UDA.

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.

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.

I don't see how this would be a breaking change, because currently @bodyParam isn't supported at all for the web interfaces, so it will only affect new code, no?

But it seems to come up a lot that people don't want the second argument and get confused by it.
One thing we should do is better document it.

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 wholeBodyParam.

Another could be to deprecate the second argument, given its probably not widely used (and easy to replace with just a struct containing a struct).

+1 (but that's something for a different PR).

@dbankov-vmware
Copy link
Author

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,
Dentcho Bankov

@wilzbach
Copy link
Member

In that case could you please share your implementation so to see if it works for my case

Sorry, it's not on GitHub yet, but it looks similar to what #1676 did for the REST part.

If indeed it is not a breaking change this is what makes the most sense to me.

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 @bodyParam I don't see any problem.

With 2.082 we will have UDAs for function attributes, so it can be:
void postFoo(@bodyParam MyStruct foo) {}

FYI:

https://dlang.org/changelog/2.082.0.html#uda-function-arguments

@wilzbach
Copy link
Member

wilzbach commented Aug 20, 2018

In that case could you please share your implementation so to see if it works for my case

Sorry, it's not on GitHub yet, but it looks similar to what #1676 did for the REST part.

I did find an older version of this on GH though. In hb-web, Path parameters with start with _ automatically get injected and everything which has no attribution for PUT/POST requests gets serialized as whole body if it's the first parameter. It's a collection of a few hacks:

https://github.com/teamhackback/hb-web/blob/master/source/hb/web/web.d#L772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants