line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.8k stars 912 forks source link

Support thrift 0.19 and 0.20 #5822

Closed jrhee17 closed 2 months ago

jrhee17 commented 2 months ago

Motivation:

Support for thrift 0.19/0.20 has been requested internally.

Notably, there has been two changes upstream.

For the first change, test files have been modified so that HttpClient/Jakarta related classes can be overwritten.

The second change is interesting because generated messages for typdef enums are now correctly set as an EnumMetadata. As a result of this, using TTextProtocol with useNamedEnums=true now yields the correct output (typedef enums serialized as the enum name). This is consistent with the other enum serialization results as well.

One downside of this is older version messages (generated with thrift<0.19) cannot deserialize a typedef enum field by enum name. This is because a FieldValueMetaData is used, and it is not possible to know which enum the field is linked to unless we parse the thrift file manually.

Modifications:

Result:

github-actions[bot] commented 2 months ago

🔍 Build Scan® (commit: 50290a2a7894786e9e7f9b2141a6fe0bba8ad14d)

Job name Status Build Scan®
build-windows-latest-jdk-21 https://ge.armeria.dev/s/sv7jvoujahy4m
build-self-hosted-unsafe-jdk-8 https://ge.armeria.dev/s/auh2qwiinhbdg
build-self-hosted-unsafe-jdk-21-snapshot-blockhound https://ge.armeria.dev/s/yvdqwvg6p2d7w
build-self-hosted-unsafe-jdk-17-min-java-17-coverage https://ge.armeria.dev/s/czzv7fv764dmg
build-self-hosted-unsafe-jdk-17-min-java-11 https://ge.armeria.dev/s/r76f7525pdil6
build-self-hosted-unsafe-jdk-17-leak https://ge.armeria.dev/s/3trm5kcviapwy
build-self-hosted-unsafe-jdk-11 https://ge.armeria.dev/s/5lq3ulcxvgrmy
build-macos-12-jdk-21 https://ge.armeria.dev/s/p4vjolx2n33wc
ikhoon commented 2 months ago

@KarboniteKream would you check the breaking change that may affect your service?

KarboniteKream commented 2 months ago

Thanks, I'll test it today. You just want to make sure that TTEXT still works for our CMS APIs?

jrhee17 commented 2 months ago

Serialized messages using TTextProtocol + typdef Enum + useNamedEnums=true using thrift compiler >=0.19 can't be read using messages compiled with an older version (<0.19).

e.g. If a client with (TTextProtocol + typdef Enum + useNamedEnums=true + >=0.19 generated message) sends a request to a server (<0.19) then the server won't be able to parse the message.

This may or may not be a problem depending on your use-case.

KarboniteKream commented 2 months ago

In this specific case, the client and the server are using the same Thrift version, so this should not be causing issues.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 74.76%. Comparing base (3596b3b) to head (50290a2). Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #5822 +/- ## ============================================ - Coverage 74.78% 74.76% -0.02% + Complexity 21437 21430 -7 ============================================ Files 1877 1877 Lines 79199 79199 Branches 10212 10212 ============================================ - Hits 59228 59217 -11 - Misses 15168 15176 +8 - Partials 4803 4806 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.