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.

Sign up for GitHub

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

Jump to bottom

Fix HttpGet protocol for WebService #1232

Merged
merged 5 commits into from
Feb 12, 2016
Merged

Fix HttpGet protocol for WebService #1232

merged 5 commits into from
Feb 12, 2016

Conversation

MikeFH
Copy link
Contributor

@MikeFH MikeFH commented Feb 10, 2016 •
edited by 304NotModified
Loading

Fixes #701
Reenables the WebserviceTest_restapi_httpget unit test, not sure if other tests are needed.
Not sure either if the spaceAsPlus parameter for the UrlHelper.UrlEncode() should be true or false

This change is Reviewable

@codecov-io
Copy link

Current coverage is 74.42%

Merging #1232 into master will increase coverage by +0.40% as of ec35b52

@@            master   #1232   diff @@
======================================
  Files          266     266       
  Stmts        15325   15346    +21
  Branches      1684    1611    -73
  Methods          0       0       
======================================
+ Hit          11345   11421    +76
+ Partial        420     381    -39
+ Missed        3560    3544    -16

Review entire Coverage Diff as of ec35b52

Uncovered Suggestions

  1. +0.09% via ...gingConfiguration.cs#282...295
  2. +0.09% via ...gingConfiguration.cs#248...261
  3. +0.08% via ...argets/FileTarget.cs#1704...1716
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@304NotModified
Copy link
Member

Thanks! Will try to review it today!

@304NotModified 304NotModified self-assigned this Feb 11, 2016
304NotModified
304NotModified reviewed Feb 11, 2016
View reviewed changes
src/NLog/Targets/WebServiceTarget.cs
var queryParameters = new StringBuilder();
string separator = string.Empty;
int i = 0;
foreach (MethodCallParameter parameter in this.Parameters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I think for is better if we already need an i. Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree. I copied the logic from the PrepareHttpRequest method and did not notice Parameters was an IList<>

@304NotModified
Copy link
Member

Hi, added some small comments.

not sure if other tests are needed.

If we looks at the code cov, we see some yellow and red. It would be nice if we add a new unit test, or expand the current unit test, so everything is covered (green).

Not sure either if the spaceAsPlus parameter for the UrlHelper.UrlEncode() should be true or false

It seems that false is the safe option:
http://stackoverflow.com/a/2678602/201303

304NotModified
304NotModified reviewed Feb 11, 2016
View reviewed changes
src/NLog/Targets/WebServiceTarget.cs
queryParameters.Append(separator);
queryParameters.Append(parameter.Name);
queryParameters.Append("=");
queryParameters.Append(UrlHelper.UrlEncode(Convert.ToString(parameterValues[i], CultureInfo.InvariantCulture), true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaceAsPlus should be false

@MikeFH
Copy link
Contributor Author

MikeFH commented Feb 12, 2016

I tried to fix the different issues. Appveyor build is passing on my repo so i don't really know why it's failing here.

@304NotModified
Copy link
Member

That's an unstable test. Will try a rebuild

304NotModified added a commit that referenced this pull request Feb 12, 2016
@304NotModified
Merge pull request #1232 from MikeFH/fix-httpget
6e35be4 
Fix HttpGet protocol for WebService
@304NotModified 304NotModified merged commit 6e35be4 into NLog:master Feb 12, 2016
@304NotModified
Copy link
Member

Cool! It has been merged. Thanks!

@304NotModified 304NotModified added the bug Bug report / Bug fix label Feb 12, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Feb 12, 2016
@304NotModified
Copy link
Member

Also thanks for nice communication, like the spaceAsPlus thing. :)

@MikeFH
Copy link
Contributor Author

MikeFH commented Feb 12, 2016

My pleasure :)

@MikeFH MikeFH deleted the fix-httpget branch February 14, 2016 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@304NotModified 304NotModified

Labels
bug Bug report / Bug fix webservice-target
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MikeFH @codecov-io @304NotModified