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

Add ArrayStreamJsonDecoder method to http4s-circe #4736

Merged

Conversation

rohan-sircar
Copy link
Contributor

@rohan-sircar rohan-sircar commented Apr 11, 2021

Add streamJsonArrayDecoder counterpart to streamJsonArrayEncoder methods in CirceInstances. It delegates to byteArrayParser method in circe-fs2.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Nice start. Thank you for adding this! A couple comments below.

circe/src/main/scala/org/http4s/circe/CirceInstances.scala Outdated Show resolved Hide resolved
@@ -140,6 +141,26 @@ class CirceSuite extends JawnDecodeSupportSuite[Json] {
writeToString(jsons).assertEquals("""[{"test1":"CirceSupport"},{"test2":"CirceSupport"}]""")
}

test("stream json array decoder should decode JSON") {
Copy link
Member

Choose a reason for hiding this comment

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

This is good.

What are the semantics if we have a stream that's ill-formed JSON? I think we would return a successful stream that fails? Otherwise, we're back to buffering to see if it's going to fail before we start streaming. It would be neat to make this behavior clear with a test, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the semantics if we have a stream that's ill-formed JSON?

I... didn't think about that for some reason. I'll check and add a test case for it. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the IO blows up like this on malformed json:

==> X org.http4s.circe.test.CirceSuite.stream json array decoder should fail on malformed JSON  0.028s org.typelevel.jawn.ParseException: expected json value got 'C...' (line 1, column 36)
// stack trace

Copy link
Member

Choose a reason for hiding this comment

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

By "successful stream that fails," I mean you get a DecodeResult that's a success that's a stream. But compiling that stream will raise the error. That's what's happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you get a DecodeResult that's a success that's a stream. But compiling that stream will raise the error

I pondered upon this for a while. That seems to be the case, yes. I added a few test cases, let me know if I got it right.

@rossabaker rossabaker added enhancement Feature requests and improvements module:circe labels Apr 11, 2021
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

This looks good to me. We'll have to wait for the Bintray Brownout to finish before CI will work again.

@rossabaker rossabaker merged commit cf04ebf into http4s:series/0.21 Apr 14, 2021
@rohan-sircar
Copy link
Contributor Author

@rossabaker thanks for the guidance and the opportunity to contribute! I'll be on the lookout for more things I could meaningfully contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements module:circe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants