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 3 simple bugs #82

Merged
merged 4 commits into from
Nov 25, 2015
Merged

Fix 3 simple bugs #82

merged 4 commits into from
Nov 25, 2015

Conversation

git-jiby-me
Copy link
Collaborator

  • The regex for matching request, was not considering arrays in the
    request JSON
  • For request with a request body, canned was checking content type to
    exactly match application/json, which is not good as browsers may
    sent charset as well with the content type.
  • For matching request and filters with more accuracy, we were
    converting the values of all keys in request to string before
    comparing, but this was being done wrong as it was creating string of
    Objects and arrays as well, which it shouldn’t

- The regex for matching request, was not considering arrays in the
request JSON
- For request with a request body, canned was checking content type to
exactly match `application/json`, which is not good as browsers may
sent charset as well with the content type.
- For matching request and filters with more accuracy, we were
converting the values of all keys in request to string before
comparing, but this was being done wrong as it was creating string of
Objects and arrays as well, which it shouldn’t
@git-jiby-me
Copy link
Collaborator Author

@sideshowcoder I forgot to create a new feature branch for this PR, but i guess that is ok, please review and let me know if you need any changes.

If you can release a revision in npm, with these fixes today, it would be really cool 💯

@git-jiby-me
Copy link
Collaborator Author

@sideshowcoder I am also working in something similar to #56, but my requirement being more of like having a filter like

//! proxyRequest: http:domain.com/path/endpoint?queryKey=queryValue

which will basically proxy matching request to the url and then provide the response from the url back.

Would this be something interesting for you?

@@ -87,6 +87,16 @@ function getContentType(mimetype){
return Response.content_types[mimetype]
}

function traverseAndSanitise(object) {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry just a typo traverseAndSanitize

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sideshowcoder oops, fixing right away

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe call it stringifyValues because that is what it does I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

metaData.request[key] = String(value)
})

traverseAndSanitise(metaData.request);
Copy link
Owner

Choose a reason for hiding this comment

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

Calling it stringifyValues makes this comment reduantant :) so we can remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sideshowcoder okey dokey 👍 , 1 min

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sideshowcoder
Copy link
Owner

Yes such a change feature as proxy would be quite valuable! thank you :)

I will try to get out a new version to npm today :)

@sideshowcoder
Copy link
Owner

I added you as a collaborator on canned as you are already adding a bunch of features and fixes :)

sideshowcoder added a commit that referenced this pull request Nov 25, 2015
@sideshowcoder sideshowcoder merged commit 23e0497 into sideshowcoder:master Nov 25, 2015
@git-jiby-me
Copy link
Collaborator Author

@sideshowcoder cool, thanks, thats nice 👍

@sideshowcoder
Copy link
Owner

out on npm.

@git-jiby-me
Copy link
Collaborator Author

@sideshowcoder thanks for the quick publish, much appreciated 👍

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