Ed Morley [:emorley] Reporter
	Description • 10 years ago

Broken out from bug 1065771, which morphed along the way.

	Ed Morley [:emorley] Reporter
	Comment 1 • 10 years ago

Example:

https://hg.mozilla.org/mozilla-central/json-pushes?version=2&startID=999999999

Expected:
HTTP 404
"unknown push ID '999999999'"

Or (but I suspect this may not be simple):
HTTP 404
{
 "lastpushid": 28005, 
 "pushes": {}
}

Actual:
HTTP 200
{
 "lastpushid": 28005, 
 "pushes": {}
}

	Gregory Szorc [:gps]
	Comment 3 • 10 years ago

Comment on attachment 8540485 [details] [diff] [review]
pushlog: return HTTP 404 if the startID push doesn't exist

Review of attachment 8540485 [details] [diff] [review]:
-----------------------------------------------------------------

Changing this is backwards incompatible and scares me a little. You speak for for TreeHerder. But I also need to hear back from everyone else consuming pushlog.

It would be much safer if we could change the behavior only for version=2 requests (which I plan to make the default once all consumers not passing version=* disappear).

Furthermore, I'm not a huge fan of a single string as the response. I know this is valid JSON. But we like objects. Let's add an "error" property to the version 2 object and remove or make empty the "pushes" entry.

Is this proposal acceptable?

	Ed Morley [:emorley] Reporter
	Comment 4 • 10 years ago

(In reply to Gregory Szorc [:gps] from comment #3)
> It would be much safer if we could change the behavior only for version=2
> requests (which I plan to make the default once all consumers not passing
> version=* disappear).

wfm :-)
 
> Furthermore, I'm not a huge fan of a single string as the response. I know
> this is valid JSON. But we like objects. Let's add an "error" property to
> the version 2 object and remove or make empty the "pushes" entry.
> 
> Is this proposal acceptable?

Yeah agree this is preferable (I almost added an "error" property to my "Or...:" example in comment 0), I was just less sure how to get a 404 without using ErrorResponse(), so went with the inferior option due to not wanting to spend too much more time on this. Would you mind driving the fix using this preferable approach? :-)

	Gregory Szorc [:gps]
	Comment 5 • 10 years ago

Yes, I can implement this.

	Gregory Szorc [:gps]
	Comment 7 • 10 years ago

/r/1715 - pushlog: return an error if startID is too large (bug 1114843)

Pull down this commit:

hg pull review -r 4fb4e6f2f3580f433eac1f012900894804041985

	Ed Morley [:emorley] Reporter
	Comment 8 • 10 years ago

https://reviewboard.mozilla.org/r/1713/#review1103

Ah of course, I was overthinking it, the answer is to still use ErrorResponse(). Thanks for taking a look at this :-)

r=me with s/200/404/

::: hgext/pushlog-legacy/pushlog-feed.py
(Diff revision 1)
> +                    raiseHTTPJSONError(200, 'PUSH_ID_GREATER_THAN_AVAILABLE',

s/200/404/ right? :-)

::: hgext/pushlog-legacy/tests/test-hgweb.t
(Diff revision 1)
> +  200

And here

	Gregory Szorc [:gps]
	Comment 9 • 10 years ago

https://reviewboard.mozilla.org/r/1713/#review1105

> s/200/404/ right? :-)

I used to be a HTTP/REST zealot. I once would have strongly argued for 404 here: "the thing I'm requesting doesn't exist, so 404 Not Found is appropriate." However, I no longer hold that view.

`json-pushes` is an RPC-style interface. The base URL/resource here is `json-pushes`. Everything after the `?` are arguments to the remote procedure call. In my mind, 404 means *the resource isn't available.* The resource is `json-pushes` (it's an RPC API). And that resource **is** available. However, the content requested by that resource (a set of pushes) is not available.

HTTP status codes communicate state of the resource. I don't think it is appropriate to return 404 here because the `json-pushes` resource/API is still available - it just doesn't have the data you want. I think internal status codes within the HTTP response body are the proper mechanism here.

FWIW, once you accept that not everything (notably RPC-style APIs) fits inside the REST framework, API designs becomes much easier :)

	Ed Morley [:emorley] Reporter
	Comment 10 • 10 years ago

In that case, can we stop returning the 404 if a non existent &fromchange=REV is specified? I'm happy to agree with your reasoning in comment 9, I'd just like consistency either way if possible :-)

	Ed Morley [:emorley] Reporter
	Comment 13 • 9 years ago

Was this ready to land? :-)

	Gregory Szorc [:gps]
	Comment 14 • 9 years ago

It was. However I'm hesitant to land it right now since we're cutting a number of releases. Leaving needinfo open to track doing this in a few days.

	Gregory Szorc [:gps]
	Comment 15 • 9 years ago

url:        https://hg.mozilla.org/hgcustom/version-control-tools/rev/3912cb415898ee50999630f2b8f2231573abb40b
changeset:  3912cb415898ee50999630f2b8f2231573abb40b
user:       Gregory Szorc <gps@mozilla.com>
date:       Mon Aug 10 14:00:36 2015 -0700
description:
pushlog: return an error if startID is too large (bug 1114843); r=edmorley

Clients need a mechanism to identify when a push ID value is out of
bounds. This patch gives them that.

This patch also introduces an error reporting mechanism for version 2
payloads. The new error mechanism may not be used everywhere yet. But it
should be. Failure to utilize the new error mechanism should be
considered a bug.

	Gregory Szorc [:gps]
	Comment 16 • 9 years ago

This is deploying currently.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

Seems like this broke taskcluster's gecko-decision (bug 1193098). Any chance we can get this undeployed?

	Gregory Szorc [:gps]
	Comment 18 • 9 years ago

url:        https://hg.mozilla.org/hgcustom/version-control-tools/rev/07bfdf492c6e62b9eeefc3b88e0e139cd7e75d8e
changeset:  07bfdf492c6e62b9eeefc3b88e0e139cd7e75d8e
user:       Gregory Szorc <gps@mozilla.com>
date:       Mon Aug 10 18:46:41 2015 -0700
description:
pushlog: backout 3912cb415898 (bug 1114843) for breaking TaskCluster

I suspect this is due to type coercion between ints and strings.

	Gregory Szorc [:gps]
	Comment 19 • 9 years ago

Comment on attachment 8618967 [details]
MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

pushlog: return an error if startID is too large (bug 1114843); r?smacleod

Clients need a mechanism to identify when a push ID value is out of
bounds. This commit gives them that.

This commit also introduces an error reporting mechanism for version 2
payloads. The new error mechanism may not be used everywhere yet. But it
should be. Failure to utilize the new error mechanism should be
considered a bug.

	Steven MacLeod [:smacleod]
	Comment 20 • 9 years ago

Comment on attachment 8618967 [details]
MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

https://reviewboard.mozilla.org/r/1715/#review14655

::: hgext/pushlog-legacy/pushlog-feed.py:304
(Diff revision 2)
> -        query.querystart_value = req.form.get('startID', ['0'])[0]
> +        query.querystart_value = int(req.form.get('startID', ['0'])[0])

Will this cast to int cause a 500 if it fails due to something non int-able being passed in?

::: hgext/pushlog-legacy/tests/test-hgweb.t:127
(Diff revision 2)
> +Querying against a startID that is too high results in an error

Can you also toss in tests for non numeric startID and endID?

	Gregory Szorc [:gps]
	Comment 21 • 9 years ago

Comment on attachment 8618967 [details]
MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

pushlog: return an error if startID is too large (bug 1114843); r?smacleod

Clients need a mechanism to identify when a push ID value is out of
bounds. This commit gives them that.

This commit also introduces an error reporting mechanism for version 2
payloads. The new error mechanism may not be used everywhere yet. But it
should be. Failure to utilize the new error mechanism should be
considered a bug.

	Gregory Szorc [:gps]
	Comment 22 • 6 years ago

3 years later and I'm definitely not actively working on this.