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

Always parse json in strict mode #507

Merged
merged 6 commits into from
Apr 13, 2020
Merged

Always parse json in strict mode #507

merged 6 commits into from
Apr 13, 2020

Conversation

rymndhng
Copy link
Collaborator

@rymndhng rymndhng commented Aug 5, 2019

Fixes #489.

This solution contains the following changes:

  1. Reduce the number of options for JSON coercions
  2. Remove lazy parsing, replacing it with documentation on how to incrementally parse JSON

Reduces the number of options for JSON coercions

An assumption I'm going to make is that most folks who work with clj-http don't really intend to get a lazy sequence back. They want the JSOn value back (with no additional side-effects). By making all the :json options strict, we reduce the number of options from 4 to 2. :json-strict becomes an undocumented alias for :json for backwards compatibility, and similarly :json-strict-string-keys is aliased to :json-string-keys.

This problem started surfacing when introducing #475 and users were caught off guard by confusing error messages due to lazy IO (see #489).

Document how to incrementally parse JSON to avoid unintended lazy IO

Mixing lazy sequences and IO is hrad to grok. For clj-http to incrementally parse JSON and close the underlying reader reliably, it would need to add additional options (similar to clojure.java.jdbc) such as row-fn and resultset-fn in order to process each row. Adding more options increases complexity for users to figure out when/why these options should matter. It'd also add complexity for documentation.

I personally think Incremental JSON parsing is an advanced feature which most users do not need to be exposed to. As an alternative, we should provide some documentation for how to incrementally parse JSON correct. This PR provides this documentation for advanced users who want this functionality.

@rymndhng rymndhng mentioned this pull request Aug 5, 2019
resp-array (client/get (localhost "/json-array") {:as :json})
resp-array-strict (client/get (localhost "/json-array") {:as :json-strict})
resp-large-array (client/get (localhost "/json-large-array") {:as :json})
resp-large-array-strict (client/get (localhost "/json-large-array") {:as :json-strict})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added these additional test cases to replicate the bug.

@dakrone
Copy link
Owner

dakrone commented Aug 6, 2019

@rymndhng it looks like lein test :all causes this to throw an exception when testing deserializing empty responses:

ERROR in (t-empty-response-coercion) (GZIPInputStream.java:269)
Uncaught exception, not in assertion.
expected: nil
  actual: java.io.EOFException: null
 at java.util.zip.GZIPInputStream.readUByte (GZIPInputStream.java:269)
    java.util.zip.GZIPInputStream.readUShort (GZIPInputStream.java:259)
    java.util.zip.GZIPInputStream.readHeader (GZIPInputStream.java:165)
    java.util.zip.GZIPInputStream.<init> (GZIPInputStream.java:80)
    java.util.zip.GZIPInputStream.<init> (GZIPInputStream.java:92)
    org.apache.http.client.entity.GZIPInputStreamFactory.create (GZIPInputStreamFactory.java:61)
    org.apache.http.client.entity.LazyDecompressingInputStream.initWrapper (LazyDecompressingInputStream.java:51)
    org.apache.http.client.entity.LazyDecompressingInputStream.read (LazyDecompressingInputStream.java:69)
    java.io.FilterInputStream.read (FilterInputStream.java:133)
    clj_http.core.proxy$java.io.FilterInputStream$ff19274a.read (:-1)
    sun.nio.cs.StreamDecoder.readBytes (StreamDecoder.java:284)
    sun.nio.cs.StreamDecoder.implRead (StreamDecoder.java:326)
    sun.nio.cs.StreamDecoder.read (StreamDecoder.java:178)
    java.io.InputStreamReader.read (InputStreamReader.java:185)
    java.io.BufferedReader.fill (BufferedReader.java:161)
    java.io.BufferedReader.read1 (BufferedReader.java:212)
    java.io.BufferedReader.read (BufferedReader.java:287)
    com.fasterxml.jackson.core.json.ReaderBasedJsonParser._loadMore (ReaderBasedJsonParser.java:243)
    com.fasterxml.jackson.core.json.ReaderBasedJsonParser._skipWSOrEnd (ReaderBasedJsonParser.java:2339)
    com.fasterxml.jackson.core.json.ReaderBasedJsonParser.nextToken (ReaderBasedJsonParser.java:656)
    cheshire.parse$parse_strict.invokeStatic (parse.clj:82)
    cheshire.parse$parse_strict.invoke (parse.clj:80)
    cheshire.core$parse_stream_strict.invokeStatic (core.clj:271)
    cheshire.core$parse_stream_strict.invoke (core.clj:257)
    cheshire.core$parse_stream_strict.invokeStatic (core.clj:268)
    cheshire.core$parse_stream_strict.invoke (core.clj:257)
    clojure.lang.AFn.applyToHelper (AFn.java:156)
    clojure.lang.AFn.applyTo (AFn.java:144)
    clojure.lang.Var.applyTo (Var.java:705)
    clojure.core$apply.invokeStatic (core.clj:665)
    clojure.core$apply.invoke (core.clj:660)
    clj_http.client$json_decode.invokeStatic (client.clj:131)
    clj_http.client$json_decode.doInvoke (client.clj:127)
    clojure.lang.RestFn.invoke (RestFn.java:421)
    clj_http.client$decode_json_body.invokeStatic (client.clj:451)
    clj_http.client$decode_json_body.invoke (client.clj:449)
    clj_http.client$coerce_json_body.invokeStatic (client.clj:458)
    clj_http.client$coerce_json_body.doInvoke (client.clj:453)

@rymndhng
Copy link
Collaborator Author

rymndhng commented Aug 7, 2019

Thanks for the poke! I've rebased against master and I've put back the logic for detecting if the stream has content.

@rymndhng
Copy link
Collaborator Author

rymndhng commented Dec 9, 2019

I have rebased the PR against the latest 3.x

@dakrone dakrone merged commit 5154fcc into 3.x Apr 13, 2020
@dakrone dakrone deleted the remove-lazy-json-option branch April 13, 2020 23:01
@rymndhng
Copy link
Collaborator Author

Thanks for following up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recently upgraded to clj-http 3.10.0 and :as :json not working
2 participants