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

pass nested error in compatible configuration, fixes #388 #422

Merged
merged 1 commit into from
Dec 21, 2019

Conversation

JensErat
Copy link
Contributor

When invalid types inside a map were marshalled (in general, as soon as
sorted maps have been configured), the error message has not been
propagated out of the map's subStream.

Also fix and re-enable the channel test, which now resembles the
behavior of encoding/json and tests both default and compatible
configurations.

Signed-off-by: Jens Erat email@jenserat.de

When invalid types inside a map were marshalled (in general, as soon as
sorted maps have been configured), the error message has not been
propagated out of the map's `subStream`.

Also fix and re-enable the channel test, which now resembles the
behavior of `encoding/json` and tests both default and compatible
configurations.

Signed-off-by: Jens Erat <email@jenserat.de>
@JensErat
Copy link
Contributor Author

Oh -- and this resolves #388

@JensErat JensErat changed the title pass nested error in compatible configuration pass nested error in compatible configuration, fixes #388 Nov 22, 2019
@codecov
Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #422 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
+ Coverage   86.46%   86.54%   +0.08%     
==========================================
  Files          41       41              
  Lines        5105     5107       +2     
==========================================
+ Hits         4414     4420       +6     
+ Misses        555      551       -4     
  Partials      136      136
Impacted Files Coverage Δ
reflect_map.go 87.8% <100%> (+0.12%) ⬆️
reflect_struct_decoder.go 81.89% <0%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44a7e73...a1c9557. Read the comment docs.

JensErat added a commit to JensErat/loki that referenced this pull request Nov 22, 2019
While developing another fix, I stumbled upon grafana#1309 as a newly written
unit test (with multiple key-value pairs in a map) was flaky.

While JSON does not [strictly define an order on records in a map][RFC8259],
but practical operations with a logging tool pretty much require it
(although of course grep for JSON is jq, not grep...). Also, the key
value output format is already sorted.

Switching to sorted output in jsoniter is pretty easy. As of today, it
still has a [bug] though, for which I already provided a [fix]. I
propose accepting that rare case where invalid types can occur from
msgpack output (can this even happen?) and re-enable the failing test
case as soon as the upstream PR is merged.

[RFC8259]: https://tools.ietf.org/html/rfc8259#section-4
[bug]:     json-iterator/go#388
[fix]:     json-iterator/go#422

Signed-off-by: Jens Erat <email@jenserat.de>
cyriltovena pushed a commit to grafana/loki that referenced this pull request Nov 26, 2019
* fluent-bit: fix variable spelling mistake

Signed-off-by: Jens Erat <email@jenserat.de>

* fluent-bit: sort JSON map

While developing another fix, I stumbled upon #1309 as a newly written
unit test (with multiple key-value pairs in a map) was flaky.

While JSON does not [strictly define an order on records in a map][RFC8259],
but practical operations with a logging tool pretty much require it
(although of course grep for JSON is jq, not grep...). Also, the key
value output format is already sorted.

Switching to sorted output in jsoniter is pretty easy. As of today, it
still has a [bug] though, for which I already provided a [fix]. I
propose accepting that rare case where invalid types can occur from
msgpack output (can this even happen?) and re-enable the failing test
case as soon as the upstream PR is merged.

[RFC8259]: https://tools.ietf.org/html/rfc8259#section-4
[bug]:     json-iterator/go#388
[fix]:     json-iterator/go#422

Signed-off-by: Jens Erat <email@jenserat.de>

* fluent-bit: properly convert []byte to string

Recently, a regression was introduced that no longer ran a deep
conversion of `[]byte` to `string` unless a label map was supplied. This
commit fixes this by running the string conversion recursively, also
removing the need of applying the conversion function again during label
map stage.

This change has two minor side effects:

- Some test cases had to be moved, as string conversion happens much
  earlier now.
- Invalid characters do not result in the whole label being ignored any
  more, but are replaced by the unicode placeholder character now. I'd
  consider this an improvement, making both debugging much easier ("why
  is that value suddenly missing?") and preserving as much information
  as possible.

Signed-off-by: Jens Erat <email@jenserat.de>
@taowen taowen merged commit acfec88 into json-iterator:master Dec 21, 2019
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
pass nested error in compatible configuration, fixes json-iterator#388
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.

2 participants