-
Notifications
You must be signed in to change notification settings - Fork 46
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
[#180] Add a JsonCodec #191
Conversation
Signed-off-by: Lukas Eder <[email protected]>
Would love to see this merged and released, as it's blocking us. In the mean time, I'll use the excellent examples here to work-around. Thanks to @lukaseder for this! |
Thanks @lukaseder. If you could give the snapshots a spin @martypitt, let us know if you see any issues. |
@gregturn I pulled down the snapshot and ran into problems. JsonCodec extends AbstractCodec<String>, which means the This causes issues when calling Since the JsonCodec is earlier in the list of DefaultCodecs, it will be the first one picked up here causing a DbException when the JsonCodec can't encode a non-json string. You end up with a stack trace of something like:
|
Ah, nice catch. Did I overlook something, or do the tests not catch this problem? |
Hi @lukaseder , any idea when this might be released on maven central? |
Nope. For I am not in charge here ;-) |
So I tried using |
Previously, no tests would encode a string with all DefaultCodecs loaded, leaving a hole in tests when multiple codecs would try and handle encoding of Strings. Exposes a bug in r2dbc#191.
The test's don't catch the problem. From what I can tell, nothing in DefaultCodecs (or anything else) does a test of string conversion with all the default codecs loaded. Not 100% sure this is the right test for it, but threw up a commit with a test that exposes the problem: |
Let me know, if you don't mind if I create a PR for the fix.
Created a bug issue: #195
|
Hello, this PR would be also good to be merged to |
Issue description
Fixes #180
New Public APIs
None