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

Handler errors should be formatted as JSON. #4921

Merged
merged 1 commit into from
Dec 2, 2015
Merged

Handler errors should be formatted as JSON. #4921

merged 1 commit into from
Dec 2, 2015

Conversation

pires
Copy link
Contributor

@pires pires commented Nov 27, 2015

  • CHANGELOG.md updated
  • Rebased/mergable
  • Tests pass
  • Sign CLA

Fixes #3170

@otoolep
Copy link
Contributor

otoolep commented Nov 28, 2015

Thanks @pires

Now that I think about this, this may be considered a breaking change, we need to be careful about this.

@pires -- can you show us an example of the before-and-after for this change?

@pires
Copy link
Contributor Author

pires commented Nov 28, 2015

@otoolep same error code but the only difference is that the error is JSON encoded as per issue request.

Is there any reasoning behind this not being the case before?

@pires
Copy link
Contributor Author

pires commented Nov 28, 2015

Previous behavior:

$ curl -v -X POST 'http://192.168.99.100:8086/write?db=notexist'
*   Trying 192.168.99.100...
* Connected to 192.168.99.100 (192.168.99.100) port 8086 (#0)
> POST /write?db=notexist HTTP/1.1
> Host: 192.168.99.100:8086
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Request-Id: a57fcaeb-95a7-11e5-8003-000000000000
< X-Influxdb-Version: 0.9.3
< Date: Sat, 28 Nov 2015 08:11:37 GMT
< Content-Length: 31
< Content-Type: text/plain; charset=utf-8
<
database not found: "notexist"
* Connection #0 to host 192.168.99.100 left intact

PR behavior:

$ curl -v -X POST 'http://localhost:8086/write?db=notexist'
*   Trying ::1...
* Connected to localhost (::1) port 8086 (#0)
> POST /write?db=notexist HTTP/1.1
> Host: localhost:8086
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Content-Type: application/json
< Request-Id: 156d97ff-95a8-11e5-8001-000000000000
< X-Influxdb-Version: 0.9
< Date: Sat, 28 Nov 2015 08:14:45 GMT
< Content-Length: 45
<
{"error":"database not found: \"notexist\""}
* Connection #0 to host localhost left intact

@otoolep
Copy link
Contributor

otoolep commented Nov 28, 2015

Agreed @pires -- it's the same error, just wrapped in JSON.

@pauldix -- I don't think we ever meant to send back non-JSON formatted errors, so this makes sense to me. But this is technically a breaking change. Need your call.

@pires
Copy link
Contributor Author

pires commented Nov 28, 2015

@otoolep thank you. I think the question about breaking changes begs for more reviewing of the currently open issues, so to prevent anyone else spending time with stuff that in the end may get rejected. Agree?

@otoolep
Copy link
Contributor

otoolep commented Nov 29, 2015

Yeah, makes sense @pires -- I'm on the fence about this one, since it's looks like a bug that it was never JSON-format.

@benbjohnson
Copy link
Contributor

I'm personally ok with this breaking change. It seems like it should have been JSON to begin with.

@pires
Copy link
Contributor Author

pires commented Dec 2, 2015

Any updates on this?

@pauldix
Copy link
Member

pauldix commented Dec 2, 2015

Yep, should have been json to begin with. Thanks @pires!

pauldix added a commit that referenced this pull request Dec 2, 2015
Handler errors should be formatted as JSON.
@pauldix pauldix merged commit 6e1d0f4 into influxdata:master Dec 2, 2015
@pires pires deleted the 3170-db_not_found branch December 2, 2015 16:39
@timhallinflux timhallinflux added this to the 0.10.0 milestone Dec 19, 2016
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.

5 participants