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

json.Unmarshal errors not catch in method client.Write #1662

Closed
wants to merge 2 commits into from

Conversation

akolosov
Copy link

No description provided.

@otoolep
Copy link
Contributor

otoolep commented Feb 21, 2015

Makes sense to me.

On Feb 20, 2015, at 10:00 AM, Alexey Kolosov notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

#1662

Commit Summary

json.Unmarshal errors not catch in method client.Write
File Changes

M client/influxdb.go (3)
Patch Links:

https://github.com/influxdb/influxdb/pull/1662.patch
https://github.com/influxdb/influxdb/pull/1662.diff

Reply to this email directly or view it on GitHub.

@akolosov
Copy link
Author

tests got 'unexpected end of JSON input' error, but you use Unmarshal instead Marshal... just make changes:

    //b := []byte{}
    //err := json.Unmarshal(b, &d)
    b, err := json.Marshal(d)
    if err != nil {
        return nil, err
    }

and tests done

@otoolep
Copy link
Contributor

otoolep commented Feb 28, 2015

@corylanou -- can we merge this? Looks like the code was using the wrong function.

@corylanou
Copy link
Contributor

We should open an issue to write a test around it, but other than that, looks good.

@d2g
Copy link

d2g commented Mar 2, 2015

So #1680 is a test that covers this issue.

However also highlights the issue that the write() actually wraps the write{} in an array (not sure why) which isn't how it's expected at the server end.

@corylanou
Copy link
Contributor

@d2g The write endpoint can take a single write, or multiple writes. This allows the end user to open an http endpoint and stream multiple writes without having to close the connection. This can cut down on the overhead of connecting/disconnecting, as that overhead is typically higher than the actual payload being sent.

@corylanou
Copy link
Contributor

We should add test coverage for multiple writes at some point as well. Looking at the test coverage, that case it not covered. We also don't cover it in the documentation. I'll open up 2 issues prior to closing this issue as a reminder to update both test coverage and documentation.

@corylanou
Copy link
Contributor

Fixed in #1868

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

4 participants