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

streaming: Skip over unrecognized fields and fields not matching the expected type #544

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

witriew
Copy link
Contributor

@witriew witriew commented Sep 9, 2021

In the Wire-based implementation of reading the raw Thrift protocol, the reader
greedily reads over all of the fields in the structs, populating a Wire
intermediary format. This format is then validated and parsed by the generated
code, populating the appropriate Thrift object with the proper values
associated with the corresponding field ID.

In the stream-based implementation, validation and parsing of the raw Thrift
protocol is done as the bytes are read. Since unrecognized fields were not
skipped over, this caused issues in future reads as there is now data that
doesn't match the expected Thrift type.

Skip over fields that are unrecognized by the streaming reader and skip fields
that don't match the type expected by the Thrift definition.

…h the expected type

In the Wire-based implementation of reading the raw Thrift protocol, the reader
greedily reads over all of the fields in the structs, creating the Wire
intermediary format.  This format is then validated and parsed by the generated
code, populating the appropriate Thrift object with the proper values
associated with the corresponding field ID.

In the stream-based implementation, validation and parsing of the raw Thrift
protocol is done as the bytes are read.  Since unrecognized fields were not
skipped over, this caused issues in future reads as there is now data that
doesn't match the expected Thrift type.

Skip over fields that are unrecognized by the streaming reader and skip fields
that don't match the type expected by the Thrift definition.
@witriew witriew requested review from abhinav and dianale31 September 9, 2021 17:15
@codecov
Copy link

codecov bot commented Sep 9, 2021

Codecov Report

Merging #544 (30b440f) into dev (9c599a2) will decrease coverage by 0.89%.
The diff coverage is 50.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #544      +/-   ##
==========================================
- Coverage   68.22%   67.32%   -0.90%     
==========================================
  Files         135      135              
  Lines       23479    23531      +52     
==========================================
- Hits        16018    15842     -176     
- Misses       4365     4593     +228     
  Partials     3096     3096              
Impacted Files Coverage Δ
...en/internal/tests/non_hyphenated/non_hyphenated.go 56.41% <0.00%> (-4.71%) ⬇️
gen/internal/tests/services/services.go 60.49% <38.95%> (-2.29%) ⬇️
.../internal/tests/hyphenated-file/hyphenated-file.go 60.67% <44.44%> (-2.55%) ⬇️
.../internal/tests/hyphenated_file/hyphenated_file.go 60.67% <44.44%> (-2.55%) ⬇️
gen/internal/tests/containers/containers.go 56.60% <46.87%> (-0.79%) ⬇️
gen/internal/tests/exceptions/exceptions.go 60.35% <47.05%> (-2.56%) ⬇️
gen/internal/tests/enums/enums.go 83.38% <50.00%> (-0.30%) ⬇️
gen/internal/tests/typedefs/typedefs.go 67.49% <50.00%> (-1.02%) ⬇️
gen/internal/tests/uuid_conflict/uuid_conflict.go 63.44% <50.00%> (-1.83%) ⬇️
gen/internal/tests/unions/unions.go 64.18% <52.08%> (-0.90%) ⬇️
... and 9 more

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 9c599a2...30b440f. Read the comment docs.

gen/field.go Outdated Show resolved Hide resolved
gen/struct_test.go Outdated Show resolved Hide resolved
gen/struct_test.go Outdated Show resolved Hide resolved
gen/struct_test.go Show resolved Hide resolved
gen/struct_test.go Outdated Show resolved Hide resolved
gen/struct_test.go Outdated Show resolved Hide resolved
gen/struct_test.go Outdated Show resolved Hide resolved
gen/struct_test.go Outdated Show resolved Hide resolved
gen/struct_test.go Show resolved Hide resolved
witriew and others added 2 commits September 9, 2021 11:07
Co-authored-by: Abhinav Gupta <abg@uber.com>
@witriew witriew requested a review from abhinav September 9, 2021 18:47
@witriew witriew merged commit e322a82 into dev Sep 9, 2021
@witriew witriew deleted the witriew/skip_decode branch September 9, 2021 19:09
witriew added a commit that referenced this pull request Sep 9, 2021
Contains the following fix:
streaming: Handle unrecognized fields and non-matching field types (#544)
witriew added a commit to yarpc/yarpc-go that referenced this pull request Sep 9, 2021
This pulls in v1.29.2 of thriftrw which contains a fix for deserializing fields
with unrecognized entries and fields with non-matching types (thriftrw/thriftrw-go#544)
witriew added a commit to yarpc/yarpc-go that referenced this pull request Sep 9, 2021
This pulls in v1.29.2 of thriftrw which contains a fix for deserializing fields
with unrecognized entries and fields with non-matching types (thriftrw/thriftrw-go#544)
Dogild pushed a commit to yarpc/yarpc-go that referenced this pull request Sep 29, 2021
…#2100)

This pulls in v1.29.2 of thriftrw which contains a fix for deserializing fields
with unrecognized entries and fields with non-matching types (thriftrw/thriftrw-go#544)
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