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

Lazy initialization of req.json #1677

Merged
merged 1 commit into from Jun 29, 2017

Conversation

Projects
None yet
2 participants
@wilzbach
Contributor

wilzbach commented Feb 8, 2017

I assume that there's probably a reason why the request parameters aren't lazily initialized, but I think the "pay once you use" is quite nice. Hence this PR is an open question whether the request parameters can be initialized on their first use. An additional benefit of using this pattern is that the returned Json reference is const.
Of course, the same pattern could be used to "lazify" other request parameters as well.

Show outdated Hide outdated http/vibe/http/server.d
auto bodyStr = () @trusted { return cast(string) bodyReader.readAll(); } ();
if (!bodyStr.empty) _json = parseJson(bodyStr);
} else {
_json = Json.undefined;

This comment has been minimized.

@wilzbach

wilzbach Feb 8, 2017

Contributor

This is done to emulate the current behavior when no json content type is present.

@wilzbach

wilzbach Feb 8, 2017

Contributor

This is done to emulate the current behavior when no json content type is present.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 9, 2017

Member

I'm actually planning to make the whole request parsing as lazy as possible for the pending rewrite of the HTTP module (in the form of a new vibe-http package), so this fits right in. Since .json should be used in a read-only manner anyway, this change should be no problem. The only thing missing is a @safe annotation.

The merge for this will be deferred until the 0.8.0 release is out to avoid last-minute regressions.

Member

s-ludwig commented Feb 9, 2017

I'm actually planning to make the whole request parsing as lazy as possible for the pending rewrite of the HTTP module (in the form of a new vibe-http package), so this fits right in. Since .json should be used in a read-only manner anyway, this change should be no problem. The only thing missing is a @safe annotation.

The merge for this will be deferred until the 0.8.0 release is out to avoid last-minute regressions.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 29, 2017

Member

This just needs a rebase, and it would be good to have the `parseJsonBody' option documented as "deprecated". It's unfortunate that enum values can't actually be deprecated, the cases for this really start to pile up.

Member

s-ludwig commented Jun 29, 2017

This just needs a rebase, and it would be good to have the `parseJsonBody' option documented as "deprecated". It's unfortunate that enum values can't actually be deprecated, the cases for this really start to pile up.

@wilzbach

This comment has been minimized.

Show comment
Hide comment
@wilzbach

wilzbach Jun 29, 2017

Contributor

That means that #1677 can be merged now, too.
This just needs a rebase, and it would be good to have the `parseJsonBody' option documented as "deprecated".

Cool. Done.

Contributor

wilzbach commented Jun 29, 2017

That means that #1677 can be merged now, too.
This just needs a rebase, and it would be good to have the `parseJsonBody' option documented as "deprecated".

Cool. Done.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 29, 2017

Member

Meh... looks like Travis has the same issues as yesterday. I'll take the risk and just pull as is ;)

Member

s-ludwig commented Jun 29, 2017

Meh... looks like Travis has the same issues as yesterday. I'll take the risk and just pull as is ;)

@s-ludwig s-ludwig merged commit c0154b8 into vibe-d:master Jun 29, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@wilzbach wilzbach deleted the wilzbach:lazy-json branch Jun 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment