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

Feature: Save API JSON (briefly) if requested by client #183

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rjp
Copy link

@rjp rjp commented May 23, 2023

I'm using this library for archival purposes (eg. saving my favourites to SQLite) and I find it useful to keep the raw API response around after the API call since it often contains extensions, fields, etc., that aren't necessarily parsed out and returned in library objects.

e.g. from my Akkoma instance, status responses have information about quote-boosting which doesn't exist in the mastodon.Status struct. Also various pleroma and akkoma extensions for the status text and instance information.

Since almost no-one is going to need this, it's gated behind Client.SaveJSON being true (and limited to 100MB of JSON to be safe.) By default, the library will behave exactly the same as before.

(I admit it's not a great solution but I don't think there is one for a situation like this without extending every API method to optionally return the JSON or similar.)

rjp added 3 commits May 23, 2023 08:27
For my archival purposes, I like to keep the raw API JSON around
as it often contains things not picked up by the various objects.

e.g. getting a status from my Akkoma instance contains a whole
`pleroma` extension block that's not reflected in `mastodon.Status`.

Maybe this should be gated by an option when creating the `Client`
since it's probably only relevant to 1% of library users.
This way the library will behave as it used to for anyone who doesn't
explicitly set `client.SaveJSON` to be `true`.  No surprises = good.
@mattn
Copy link
Owner

mattn commented May 23, 2023

I prefer to do with TeeReader.

type Client struct {
    ...
    JsonWriter io.Writer
}
if c.JsonWriter != nil {
    return json.NewDecoder(io.TeeReader(resp.Body, c.JsonWriter)).Decode(&res)
} else {
    return json.NewDecoder(resp.Body).Decode(&res)
}
client := mastodon.NewClient()
var buf bytes.Buffer
client.JsonWriter = &buf
// something to do with a client

@rjp
Copy link
Author

rjp commented May 23, 2023

Neat solution! It does require the caller to remember to issue a buf.Reset() before every API call they want the JSON from though -- a definite footgun (I know because I just encountered it) and a bit untidy. Can't do the Reset() from the library though because it's not available on io.Writer.

A localised WriteResetter interface fixes things and keeps the client code tidy.

type WriteResetter interface {
    io.Writer
    Reset()
}

type Client struct {
    ...
    JSONWriter WriteResetter
}
    if c.JSONWriter != nil {
        c.JSONWriter.Reset()
        return json.NewDecoder(io.TeeReader(resp.Body, c.JSONWriter)).Decode(&res)
    } else {
        return json.NewDecoder(resp.Body).Decode(&res)
    }

Thoughts?

rjp added 2 commits May 25, 2023 08:05
As suggested by 'mattn, use a `TeeReader` with an `io.Writer` to keep
the semantics of `json.NewDecoder().Decode()` whilst also being able
to save the JSON if requested by the client.

However we need to use our own interface (`WriteResetter`) in order to
be able to call `Reset()` on the underlying buffer (which does somewhat
limit the usage to buffers instead of generic `io.Writer` and may not
be the 100% ideal solution.)
After going through several other solutions, there's no clean way
to use an `io.Writer` but also be able to call `Reset()` between
writes.  Filehandles can't `Reset()` and buffers can't `Seek()`.
But then if you supplied `os.Stderr`, you'd be expecting appending
and a `Seek()` would be nonsense.

My own preference would be to make `JSONWriter` a strict `*bytes.Buffer`
which lets the library handle the resetting and the client do any
outputting if required - I can't see much value in ever supplying
a non-`bytes.Buffer` as `JSONWriter`.
@rjp
Copy link
Author

rjp commented May 25, 2023

Reverted to simple io.Writer with client-initiated resetting. Think making JSONWriter a simple *bytes.Buffer and moving the reset to the library makes more sense (when would someone supply a non-bytes.Buffer as somewhere to save the JSON?) but could live with this solution.

@mattn
Copy link
Owner

mattn commented May 25, 2023

It can check before call Write()

if resetter, ok := c.JSONWriter.(WriterResetter); ok {
  resetter.Reset()
}

Supply an `io.Writer` but check it against the `WriterResetter`
interface to see if we can call `Reset()`.  Which is a neat solution!

Add a new test to check that a supplied `bytes.Buffer` gets reset
between API calls.
@rjp
Copy link
Author

rjp commented May 25, 2023

Ah, I think I got almost to that solution twice with the WriteResetter and then checking if an interface{} could call Reset(). Never checked a type assertion on an interface that's bigger than a type before. Thanks.

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

2 participants