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

Test the PHP action API with a bad hostname #102

Merged
merged 5 commits into from
Jan 6, 2015
Merged

Test the PHP action API with a bad hostname #102

merged 5 commits into from
Jan 6, 2015

Conversation

earldouglas
Copy link
Contributor

This revealed a problem in the forwarding a 'localhost' host header
out to external Web services, which makes the PHP action API rather
unhappy.

return filtered;
}

function find(xs, predicate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, Array.find() is part of Harmony, and is provided by es6-shim, with the difference being it returns the value, not the index in the array, so maybe we should use that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out - I have much to learn about the various JS std libs. I'll switch to that and toss out this ad-hoc implementation.

@earldouglas
Copy link
Contributor Author

Rebased and switched to es6-shim's Array.find implementation.

This revealed a problem in the forwarding a 'localhost' host header
out to external Web services, which makes the PHP action API rather
unhappy.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c0bab7c on earldouglas:action-404 into 95e282f on wikimedia:master.

@earldouglas
Copy link
Contributor Author

Reased on master.

@@ -17,7 +17,12 @@ module.exports = {
body.format = 'json';
return restbase[req.method](req)
.then(function(res) {
if (res.status !== 200) {
if (
res.status !== 200
Copy link
Member

Choose a reason for hiding this comment

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

This would likely be simplified if we raised exceptions / rejected the promise like in preq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about non-rejected statuses (e.g. 201, 30x)?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a legitimate case where the body is empty, but the return status is 200?

I'm actually not sure about res.body.query{,.pages} either. For example, this query still sets query and pages despite not matching any page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ curl -s 'http://en.wikipedia.org/w/api.php?format=json&action=query' | python -mjson.tool
{
    "warnings": {
        "query": {
            "*": "Formatting of continuation data will be changing soon. To continue using the current formatting, use the 'rawcontinue' parameter. To begin using the new format, pass an empty string for 'continue' in the initial query."
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Well, that's the reply to an empty query, which we (afaik) never send. Can this actually happen with queries we are sending?

Copy link
Member

Choose a reason for hiding this comment

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

The action API wrapper is really a very embryonic internal thing right now. Once we start thinking about how we could integrate the PHP API into a more general REST API we'd probably want to give it a more systematic treatment.

In the meantime, internal uses that don't actually supply a proper query should probably fail loud & early, rather than just seeming to succeed without results. It did so far (by accessing something that wasn't defined), but we could make it more explicit with a check & an explicit throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll make these cases result in errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does a 400 make sense for these cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe a 500 is better. We don't necessarily know that the request was poorly formatted -- all we know is that the PHP action API didn't give us back what we expect.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Anything that throws is good ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling 0df0d2f on earldouglas:action-404 into 95e282f on wikimedia:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.44%) when pulling 0df0d2f on earldouglas:action-404 into 95e282f on wikimedia:master.

@@ -272,5 +272,24 @@ util.inherits(HTTPError, Error);

rbUtil.HTTPError = HTTPError;

rbUtil.httpErrors = {
offline: new HTTPError({
Copy link
Member

Choose a reason for hiding this comment

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

Minor: It might be more consistent to make both httpErrors functions.

gwicke added a commit that referenced this pull request Jan 6, 2015
Test the PHP action API with a bad hostname
@gwicke gwicke merged commit db3c981 into wikimedia:master Jan 6, 2015
@earldouglas earldouglas deleted the action-404 branch January 15, 2015 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants