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

Decode percent encoded strings in parameters #33

Merged
merged 4 commits into from Feb 18, 2016

Conversation

kylebshr
Copy link
Contributor

This is a simple PR that decodes strings using stringByRemovingPercentEncoding before inserting them into parameters. Almost every web server does this automatically.

I was unsure of what to do with regards to tests, as I didn't see existing tests for the parameters. If I could get feedback on that, I would love to write tests if needed.

@tanner0101
Copy link
Member

Are you working with Vapor in Xcode? If so, you should be able to create a new file in the Tests folder, following the structure of the two tests that already exist in there. (I just added another one a few minutes ago, so you may need to merge master into your fork to see it).

Then select the Tests Scheme (There should only be Vapor and Tests), and hit command + u to run the tests.

Otherwise, this PR looks good. We will see if Travis comes back with the OK.

@kylebshr
Copy link
Contributor Author

Sorry if I wasn't clear — I can run the existing tests, and I know how to add new ones. I just didn't see any existing tests that tests the parameters, so I wasn't really sure how to start some from scratch (since I'm not too familiar with how the project is structured).

If I have time to look into it more though, I'll look into writing same basic tests. I looked at the RouterTests again and I think I could do something similar.

@tanner0101
Copy link
Member

Ah okay. There have been basically no tests written yet, so lots to do... The ResponseTests was there just to make sure the Tests scheme worked really. RouterTests is the first unit test written alongside a new feature.

I would include the test for this PR in the RouterTests file as a separate function–called testUrlParameters() or something like that...

If you want to add additional tests for other aspects of Vapor (which would be greatly appreciated), you could submit that as a separate PR.

@tanner0101
Copy link
Member

Fixes issue #32.

@kylebshr
Copy link
Contributor Author

Working on tests right now, how would I go about updating this PR? My GitHub fu is lacking.

@tanner0101
Copy link
Member

Any commits you make to the branch involved in this PR (kylebshr:master) will automatically appear here.

@kylebshr
Copy link
Contributor Author

Alrighty! Added a test if you wanna take a look, assuming travis is happy of course.


let percentEncodedString = "testing%20parameter%21%23%24%26%27%28%29%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D"
let decodedString = percentEncodedString.stringByRemovingPercentEncoding
Copy link
Member

Choose a reason for hiding this comment

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

To improve the test, you could consider writing the decoded string manually. This way if stringByRemovingPercentEncoding fails (which it could since I think it's included in the Swift linux fixes) we will know.

@kylebshr
Copy link
Contributor Author

Will do! Wasn't sure about that, I'm still new to writing unit tests 👍

@kylebshr
Copy link
Contributor Author

I realized that the test can still pass if the handler doesn't run (if someone modified the test and the handler never got called for some reason). I added in an assertion to make sure the handler was run — is there a better way to do that, or is it a good check?

@tanner0101
Copy link
Member

Everything looks great. Thank you!

tanner0101 added a commit that referenced this pull request Feb 18, 2016
Decode percent encoded strings in parameters
@tanner0101 tanner0101 merged commit 31e2d8b into vapor:master Feb 18, 2016
@tanner0101
Copy link
Member

I've updated tag 0.1.9 to include this commit.

@kylebshr
Copy link
Contributor Author

This is strange — something in 0.1.9 is spitting out gibberish when the server starts. This didn't happen a few hours ago, but I don't think it was anything this PR added:

screen shot 2016-02-18 at 5 11 11 pm

@artkay
Copy link
Member

artkay commented Feb 18, 2016

@kylebshr whoops sorry, I think that was me.
#34 will fix that once merged.

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