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

Fix repeated string serialization for JSON. #6888

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsuereth
Copy link
Contributor

Fixes an issue where repeated strings would serialize as the same JSON field multiple times (the same encoding used in binary proto).

This is because in JSON string is a primitive, but not in binary Protocol buffers.

E.g. instead of stringTable: ["one", "two"],, we'd get stringTable: "one", stringTable: "two".

  • Adds serializeRepeatedString abstract method which can appropriately handled string arrays in JSON.
  • Adds new sizeRepatedString for solidarity with other methods.

For maintainers - This code relies on existing protobuf tests. If you'd like to see the serialization primitives start to have test cases, I'd have to build up a test suite for them but happy to do so.

@jsuereth jsuereth requested a review from a team as a code owner November 18, 2024 15:10
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 90.18%. Comparing base (9a0bfb2) to head (e3eb3e1).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...etry/exporter/internal/marshal/JsonSerializer.java 0.00% 5 Missing ⚠️
...metry/exporter/internal/marshal/MarshalerUtil.java 0.00% 4 Missing ⚠️
...elemetry/exporter/internal/marshal/Serializer.java 0.00% 4 Missing ⚠️
...try/exporter/internal/marshal/ProtoSerializer.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6888      +/-   ##
============================================
- Coverage     90.26%   90.18%   -0.08%     
  Complexity     6586     6586              
============================================
  Files           729      729              
  Lines         19767    19783      +16     
  Branches       1944     1946       +2     
============================================
- Hits          17842    17841       -1     
- Misses         1334     1350      +16     
- Partials        591      592       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trask
Copy link
Member

trask commented Nov 18, 2024

cc @jhalliday

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