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

vibe.web.web: Allow Json as a auto-injected parameter type #1853

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wilzbach
Copy link
Member

Split-off from #1697

Now that req.json is lazy, this shouldn't be blocked anymore :)

@s-ludwig
Copy link
Member

Instead of giving Json such a special role, I'd like to go the same route as the REST interface. Having a parameter consume the full query/body would then work with an explicit @queryParam/@bodyParam attribute:

@bodyParam("bar", "b-ar") // searches for form field "b-ar"
void getFoo(string bar) {}

@bodyParam("bar") // the whole body will be stored in bar
void getFoo2(string bar) {}

That would also mean that @headerParam becomes available, which may be handy in some situations.

When that works, Json just has to be supported in some way in readFormParamRec.

This is of course a bit more to type and the question is if frequency wins over clarity of intent here. If it should turn out that it does, then we should at least restrict this to parameters named _body/_query or similar.

@wilzbach
Copy link
Member Author

wilzbach commented Jul 19, 2017

I'd like to go the same route as the REST interface. Having a parameter consume the full query/body would then work with an explicit @queryParam/@bodyParam attribute

As you might have guessed, I am not a huge fan of @bodyParam, because I don't have any need to handle old-school forms (it's 2017 after all). All my request PUT/POST requests originate from JS (or other) and thus I receive a rather large payload. Thus I hugely prefer to document the InputType explicitly:

struct MyInputObject {
  string p1, p2, p3;
  @optional Nullable!int i;
}
void foo(MyInputObject input)

over picking out some attributes manually:

@bodyParam("p1")
@bodyParam("p2")
@bodyParam("p3")
@bodyParam("i")
void (string p1, string p2, string p3, Nullable!int i = Nullable!int.init)

In practice there are much more attributes, of course.
Regarding @queryParam, while I agree that it's nice to have them documented as part of the function header, in practice they are almost always optional for me, which usually leads to nice code like this:

void foo() {
if (auto tok = "adminToken" in req.query) {
...

Whereas with @queyParam this would be rather redundant:

@queryParam("adminToken")
void foo(Nullable!string adminToken = Nullable!string.init) {
if (!adminToken.isNull) {
...

That would also mean that @headerParam becomes available, which may be handy in some situations.

Btw currently headers(<key>, <value>) only allows setting the response header, but headers(<key>) could be added.

This is of course a bit more to type and the question is if frequency wins over clarity of intent here.

Well, I maintain a small fork of vibe.web.web (renamed to hb-web to avoid conflicts) which contains #1697
(It's intended as an experiment and I try to port successful/useful additions to upstream).

then we should at least restrict this to parameters named _body/_query or similar.

Would be fine for me.

@wilzbach
Copy link
Member Author

then we should at least restrict this to parameters named _body/_query or similar.

Done (used _body and _json). Any chance json could be part of the whitelist as well?
(We don't use _req or _res as well either).

FWIW there are already a lot of special cases:

else static if (is(PT == InputStream)) params[i] = req.bodyReader;
else static if (is(PT == HTTPServerRequest) || is(PT == HTTPRequest)) params[i] = req;
else static if (is(PT == HTTPServerResponse) || is(PT == HTTPResponse)) params[i] = res;
else static if (is(PT == Json)) {

The point of this PR was that the someone has a Json type in their class method, it's like InputStream or HTTPServerResponse rather obvious that one wants to access req.json.

Btw we could also - similarly to header, status. render, redirect, terminateSession etc. add a json helper function that can be used to access and write Json payloads.

@wilzbach
Copy link
Member Author

Btw we could also - similarly to header, status. render, redirect, terminateSession etc. add a json helper function that can be used to access and write Json payloads.

-> #1945

@wilzbach
Copy link
Member Author

Rebased to trigger the CI systems.

then we should at least restrict this to parameters named _body/_query or similar.

This is the case since some time. @s-ludwig anything else blocking this?
BTW (I repeat the question from above): is there any chance json could be part of the whitelist as well?

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.

2 participants