Skip to content

Commit

Permalink
Support thrift 0.19 and 0.20 (#5822)
Browse files Browse the repository at this point in the history
Motivation:

Support for thrift 0.19/0.20 has been requested internally.

Notably, there has been two changes upstream.
- Jakarta EE, Apache HttpClient 5 is now used
[ref](apache/thrift#2746)
- The "true" type is now used when generating Metadata
[ref](https://github.com/apache/thrift/pull/2765/files)

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:

- Jakarta EE, Apache HttpClient 5 related dependencies are added
- Extracted `ServletTestUtils` so that
`ThriftOverHttpClientTServletIntegrationTest` can be shared across
modules.
- Extracted `ApacheClientUtils` so that `ThriftDocServiceTest`,
`ThriftOverHttp1Test` can be shared across modules.
  - Modified to use JDK over Apache's `Base64` library
- Generated messages for `typdef` enums are now correctly set, so
`TTextProtocolTest#tTextNamedEnumProtocolReadWriteTest` has been
modified to check if the generated `typedef` value is a enum ordinal or
name.

Result:

- #5243

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
  • Loading branch information
jrhee17 authored Jul 25, 2024
1 parent d793ade commit 4b78ddc
Show file tree
Hide file tree
Showing 31 changed files with 812 additions and 264 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public static Map<String, Version> getAll(ClassLoader classLoader) {
for (Object o : props.keySet()) {
final String k = (String) o;

final int dotIndex = k.indexOf('.');
final int dotIndex = k.lastIndexOf('.');
if (dotIndex <= 0) {
continue;
}
Expand Down
19 changes: 19 additions & 0 deletions dependencies.toml
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ thrift015 = { strictly = "0.15.0" }
thrift016 = { strictly = "0.16.0" }
thrift017 = { strictly = "0.17.0" }
thrift018 = { strictly = "0.18.1" }
thrift019 = { strictly = "0.19.0" }
thrift020 = { strictly = "0.20.0" }
tomcat8 = "8.5.100"
tomcat9 = "9.0.87"
tomcat10 = "10.1.20"
Expand Down Expand Up @@ -1335,6 +1337,23 @@ exclusions = [
"javax.annotation:javax.annotation-api",
"org.apache.httpcomponents:httpcore",
"org.apache.httpcomponents:httpclient"]
[libraries.thrift019]
module = "org.apache.thrift:libthrift"
version.ref = "thrift019"
javadocs = "https://www.javadoc.io/doc/org.apache.thrift/libthrift/0.19.0/"
exclusions = [
"javax.annotation:javax.annotation-api",
"org.apache.httpcomponents:httpcore",
"org.apache.httpcomponents:httpclient"]
[libraries.thrift020]
module = "org.apache.thrift:libthrift"
version.ref = "thrift020"
javadocs = "https://www.javadoc.io/doc/org.apache.thrift/libthrift/0.20.0/"
exclusions = [
"javax.annotation:javax.annotation-api",
"org.apache.httpcomponents:httpcore",
"org.apache.httpcomponents:httpclient"]


[libraries.tomcat8-core]
module = "org.apache.tomcat.embed:tomcat-embed-core"
Expand Down
4 changes: 0 additions & 4 deletions gradle/scripts/.github/CODEOWNERS

This file was deleted.

4 changes: 2 additions & 2 deletions gradle/scripts/.gitrepo
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
[subrepo]
remote = https://github.com/line/gradle-scripts
branch = main
commit = a3211a7ec874b42fc7dc5a84b3960a705d5fc34c
parent = c66b9211afdd86ce388b5b77b99fc7ffad6c0888
commit = 99521b299c71479aff81130ff4cd6881c197bbef
parent = dbae312e51e33b324e80126c174f8a485e4084a3
method = merge
cmdver = 0.4.6
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file added gradle/scripts/lib/thrift/0.19/thrift.osx-x86_64
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file added gradle/scripts/lib/thrift/0.20/thrift.osx-x86_64
Binary file not shown.
Binary file not shown.
4 changes: 4 additions & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,10 @@ includeWithFlags ':thrift0.17', 'java', 'publish', 'rel
project(':thrift0.17').projectDir = file('thrift/thrift0.17')
includeWithFlags ':thrift0.18', 'java11', 'publish', 'relocate', 'no_aggregation', 'native'
project(':thrift0.18').projectDir = file('thrift/thrift0.18')
includeWithFlags ':thrift0.19', 'java11', 'publish', 'relocate', 'no_aggregation', 'native'
project(':thrift0.19').projectDir = file('thrift/thrift0.19')
includeWithFlags ':thrift0.20', 'java11', 'publish', 'relocate', 'no_aggregation', 'native'
project(':thrift0.20').projectDir = file('thrift/thrift0.20')
includeWithFlags ':tomcat8', 'java', 'publish', 'relocate', 'no_aggregation'
includeWithFlags ':tomcat9', 'java', 'publish', 'relocate', 'no_aggregation'
includeWithFlags ':tomcat10', 'java11', 'publish', 'relocate'
Expand Down
2 changes: 1 addition & 1 deletion thrift/thrift0.12/src/test/thrift/TTextProtocolTest.thrift
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ struct TTextProtocolTestMsg {

22: required list<TestUnion> x;

23: Moji y; // Does not support string values.
23: Moji y; // Does not support string values for thrift version < 0.19.

27: required map<Letter, i32> aa;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
/*
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client.thrift;

import static org.assertj.core.api.Assertions.fail;

import java.io.IOException;
import java.util.EnumSet;
import java.util.concurrent.atomic.AtomicBoolean;

import javax.servlet.DispatcherType;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.Servlet;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.apache.thrift.server.TServlet;
import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory;
import org.eclipse.jetty.server.ConnectionFactory;
import org.eclipse.jetty.server.Connector;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.HttpConnectionFactory;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.FilterHolder;
import org.eclipse.jetty.servlet.ServletHandler;
import org.eclipse.jetty.servlet.ServletHolder;

import com.linecorp.armeria.common.thrift.ThriftProtocolFactories;

import testing.thrift.main.HelloService.Processor;

final class ServletTestUtils {

private ServletTestUtils() {}

static final String TSERVLET_PATH = "/thrift";

@SuppressWarnings("unchecked")
private static final Servlet thriftServlet =
new TServlet(new Processor(name -> "Hello, " + name + '!'), ThriftProtocolFactories.binary(0, 0));

private static final Servlet rootServlet = new HttpServlet() {
private static final long serialVersionUID = 6765028749367036441L;

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
resp.setStatus(404);
resp.setContentLength(0);
}
};

static Server startHttp1(AtomicBoolean sendConnectionClose) throws Exception {
final Server server = new Server(0);

final ServletHandler handler = new ServletHandler();
handler.addServletWithMapping(newServletHolder(thriftServlet), TSERVLET_PATH);
handler.addServletWithMapping(newServletHolder(rootServlet), "/");
handler.addFilterWithMapping(new FilterHolder(new ConnectionCloseFilter(sendConnectionClose)), "/*",
EnumSet.of(DispatcherType.REQUEST));

server.setHandler(handler);

for (Connector c : server.getConnectors()) {
for (ConnectionFactory f : c.getConnectionFactories()) {
for (String p : f.getProtocols()) {
if (p.startsWith("h2c")) {
fail("Attempted to create a Jetty server without HTTP/2 support, but failed: " +
f.getProtocols());
}
}
}
}

server.start();
return server;
}

static Server startHttp2() throws Exception {
final Server server = new Server();
// Common HTTP configuration.
final HttpConfiguration config = new HttpConfiguration();

// HTTP/1.1 support.
final HttpConnectionFactory http1 = new HttpConnectionFactory(config);

// HTTP/2 cleartext support.
final HTTP2CServerConnectionFactory http2c = new HTTP2CServerConnectionFactory(config);

// Add the connector.
final ServerConnector connector = new ServerConnector(server, http1, http2c);
connector.setPort(0);
server.addConnector(connector);

// Add the servlet.
final ServletHandler handler = new ServletHandler();
handler.addServletWithMapping(newServletHolder(thriftServlet), TSERVLET_PATH);
server.setHandler(handler);

// Start the server.
server.start();
return server;
}

private static ServletHolder newServletHolder(Servlet rootServlet) {
final ServletHolder holder = new ServletHolder(rootServlet);
holder.setInitParameter("cacheControl", "max-age=0, public");
return holder;
}

private static class ConnectionCloseFilter implements Filter {
private final AtomicBoolean sendConnectionClose;

ConnectionCloseFilter(AtomicBoolean sendConnectionClose) {
this.sendConnectionClose = sendConnectionClose;
}

@Override
public void init(FilterConfig filterConfig) {}

@Override
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
throws IOException, ServletException {

chain.doFilter(request, response);

if (sendConnectionClose.get()) {
((HttpServletResponse) response).setHeader("Connection", "close");
}
}

@Override
public void destroy() {}
}
}
Loading

0 comments on commit 4b78ddc

Please sign in to comment.