Repository: aurora Updated Branches: refs/heads/master ccf23820d -> 140b6e7c9
Make `ServerSet.JSON_CODEC` robust to errors. Previously bad or partial input (deserialize), and errors writing output (serialize) could throw exceptions not matching the `Codec` contract. Factor the codec into its own top-level class to allow testing the serialization failure case and expand coverage to include bad inputs. Bugs closed: AURORA-1468 Reviewed at https://reviews.apache.org/r/45992/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/140b6e7c Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/140b6e7c Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/140b6e7c Branch: refs/heads/master Commit: 140b6e7c993b93a67959c950a0d169f630f8af22 Parents: ccf2382 Author: John Sirois <[email protected]> Authored: Mon Apr 11 22:31:49 2016 -0600 Committer: John Sirois <[email protected]> Committed: Mon Apr 11 22:31:49 2016 -0600 ---------------------------------------------------------------------- build.gradle | 2 + .../aurora/common/zookeeper/JsonCodec.java | 139 +++++++++++++++++ .../aurora/common/zookeeper/ServerSet.java | 78 +--------- .../aurora/common/zookeeper/JsonCodecTest.java | 151 +++++++++++++++++++ .../aurora/common/zookeeper/ServerSetTest.java | 78 ---------- 5 files changed, 293 insertions(+), 155 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/140b6e7c/build.gradle ---------------------------------------------------------------------- diff --git a/build.gradle b/build.gradle index d981ab7..e4e42fd 100644 --- a/build.gradle +++ b/build.gradle @@ -178,6 +178,8 @@ project(':commons') { compile "junit:junit:${junitRev}" testCompile "junit:junit:${junitRev}" + testCompile "org.powermock:powermock-module-junit4:1.6.4" + testCompile "org.powermock:powermock-api-easymock:1.6.4" } } http://git-wip-us.apache.org/repos/asf/aurora/blob/140b6e7c/commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java ---------------------------------------------------------------------- diff --git a/commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java b/commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java new file mode 100644 index 0000000..9d31608 --- /dev/null +++ b/commons/src/main/java/org/apache/aurora/common/zookeeper/JsonCodec.java @@ -0,0 +1,139 @@ +/** + * Licensed 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 + * + * http://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 org.apache.aurora.common.zookeeper; + +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.Charset; +import java.util.Map; + +import javax.annotation.Nullable; + +import com.google.common.base.Charsets; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; +import com.google.gson.Gson; +import com.google.gson.JsonIOException; +import com.google.gson.JsonParseException; + +import org.apache.aurora.common.io.Codec; +import org.apache.aurora.common.thrift.Endpoint; +import org.apache.aurora.common.thrift.ServiceInstance; +import org.apache.aurora.common.thrift.Status; + +import static java.util.Objects.requireNonNull; + +class JsonCodec implements Codec<ServiceInstance> { + + private static void assertRequiredField(String fieldName, Object fieldValue) { + if (fieldValue == null) { + throw new JsonParseException(String.format("Field %s is required", fieldName)); + } + } + + private static class EndpointSchema { + private final String host; + private final Integer port; + + EndpointSchema(Endpoint endpoint) { + host = endpoint.getHost(); + port = endpoint.getPort(); + } + + Endpoint asEndpoint() { + assertRequiredField("host", host); + assertRequiredField("port", port); + + return new Endpoint(host, port); + } + } + + private static class ServiceInstanceSchema { + private final EndpointSchema serviceEndpoint; + private final Map<String, EndpointSchema> additionalEndpoints; + private final Status status; + private final @Nullable Integer shard; + + ServiceInstanceSchema(ServiceInstance instance) { + serviceEndpoint = new EndpointSchema(instance.getServiceEndpoint()); + if (instance.isSetAdditionalEndpoints()) { + additionalEndpoints = + Maps.transformValues(instance.getAdditionalEndpoints(), EndpointSchema::new); + } else { + additionalEndpoints = ImmutableMap.of(); + } + status = instance.getStatus(); + shard = instance.isSetShard() ? instance.getShard() : null; + } + + ServiceInstance asServiceInstance() { + assertRequiredField("serviceEndpoint", serviceEndpoint); + assertRequiredField("status", status); + + Map<String, EndpointSchema> extraEndpoints = + additionalEndpoints == null ? ImmutableMap.of() : additionalEndpoints; + + ServiceInstance instance = + new ServiceInstance( + serviceEndpoint.asEndpoint(), + Maps.transformValues(extraEndpoints, EndpointSchema::asEndpoint), + status); + if (shard != null) { + instance.setShard(shard); + } + return instance; + } + } + + private static final Charset ENCODING = Charsets.UTF_8; + + private final Gson gson; + + JsonCodec() { + this(new Gson()); + } + + JsonCodec(Gson gson) { + this.gson = requireNonNull(gson); + } + + @Override + public void serialize(ServiceInstance instance, OutputStream sink) throws IOException { + Writer writer = new OutputStreamWriter(sink, ENCODING); + try { + gson.toJson(new ServiceInstanceSchema(instance), writer); + } catch (JsonIOException e) { + throw new IOException(String.format("Problem serializing %s to JSON", instance), e); + } + writer.flush(); + } + + @Override + public ServiceInstance deserialize(InputStream source) throws IOException { + InputStreamReader reader = new InputStreamReader(source, ENCODING); + try { + @Nullable ServiceInstanceSchema schema = gson.fromJson(reader, ServiceInstanceSchema.class); + if (schema == null) { + throw new IOException("JSON did not include a ServiceInstance object"); + } + return schema.asServiceInstance(); + } catch (JsonParseException e) { + throw new IOException("Problem parsing JSON ServiceInstance.", e); + } + } +} http://git-wip-us.apache.org/repos/asf/aurora/blob/140b6e7c/commons/src/main/java/org/apache/aurora/common/zookeeper/ServerSet.java ---------------------------------------------------------------------- diff --git a/commons/src/main/java/org/apache/aurora/common/zookeeper/ServerSet.java b/commons/src/main/java/org/apache/aurora/common/zookeeper/ServerSet.java index 2d978c1..aeea02d 100644 --- a/commons/src/main/java/org/apache/aurora/common/zookeeper/ServerSet.java +++ b/commons/src/main/java/org/apache/aurora/common/zookeeper/ServerSet.java @@ -13,26 +13,11 @@ */ package org.apache.aurora.common.zookeeper; -import java.io.IOException; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.OutputStream; -import java.io.OutputStreamWriter; -import java.io.Writer; import java.net.InetSocketAddress; -import java.nio.charset.Charset; import java.util.Map; -import javax.annotation.Nullable; - -import com.google.common.base.Charsets; -import com.google.common.collect.Maps; -import com.google.gson.Gson; - import org.apache.aurora.common.io.Codec; -import org.apache.aurora.common.thrift.Endpoint; import org.apache.aurora.common.thrift.ServiceInstance; -import org.apache.aurora.common.thrift.Status; import org.apache.aurora.common.zookeeper.Group.JoinException; /** @@ -49,68 +34,7 @@ public interface ServerSet { * * This is the default encoding for service instance data in ZooKeeper. */ - Codec<ServiceInstance> JSON_CODEC = new Codec<ServiceInstance>() { - class EndpointSchema { - private final String host; - private final int port; - - EndpointSchema(Endpoint endpoint) { - host = endpoint.getHost(); - port = endpoint.getPort(); - } - - Endpoint asEndpoint() { - return new Endpoint(host, port); - } - } - - class ServiceInstanceSchema { - private final EndpointSchema serviceEndpoint; - private final Map<String, EndpointSchema> additionalEndpoints; - private final Status status; - private final @Nullable Integer shard; - - ServiceInstanceSchema(ServiceInstance instance) { - serviceEndpoint = new EndpointSchema(instance.getServiceEndpoint()); - if (instance.getAdditionalEndpoints() != null) { - additionalEndpoints = - Maps.transformValues(instance.getAdditionalEndpoints(), EndpointSchema::new); - } else { - additionalEndpoints = Maps.newHashMap(); - } - status = instance.getStatus(); - shard = instance.isSetShard() ? instance.getShard() : null; - } - - ServiceInstance asServiceInstance() { - ServiceInstance instance = - new ServiceInstance( - serviceEndpoint.asEndpoint(), - Maps.transformValues(additionalEndpoints, EndpointSchema::asEndpoint), - status); - if (shard != null) { - instance.setShard(shard); - } - return instance; - } - } - - private final Charset encoding = Charsets.UTF_8; - private final Gson gson = new Gson(); - - @Override - public void serialize(ServiceInstance instance, OutputStream sink) throws IOException { - Writer writer = new OutputStreamWriter(sink, encoding); - gson.toJson(new ServiceInstanceSchema(instance), writer); - writer.flush(); - } - - @Override - public ServiceInstance deserialize(InputStream source) throws IOException { - InputStreamReader reader = new InputStreamReader(source, encoding); - return gson.fromJson(reader, ServiceInstanceSchema.class).asServiceInstance(); - } - }; + Codec<ServiceInstance> JSON_CODEC = new JsonCodec(); /** * Attempts to join a server set for this logical service group. http://git-wip-us.apache.org/repos/asf/aurora/blob/140b6e7c/commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java ---------------------------------------------------------------------- diff --git a/commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java b/commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java new file mode 100644 index 0000000..2166123 --- /dev/null +++ b/commons/src/test/java/org/apache/aurora/common/zookeeper/JsonCodecTest.java @@ -0,0 +1,151 @@ +/** + * Licensed 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 + * + * http://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 org.apache.aurora.common.zookeeper; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; + +import com.google.common.base.Charsets; +import com.google.common.collect.ImmutableMap; +import com.google.gson.Gson; +import com.google.gson.JsonIOException; + +import org.apache.aurora.common.io.Codec; +import org.apache.aurora.common.thrift.Endpoint; +import org.apache.aurora.common.thrift.ServiceInstance; +import org.apache.aurora.common.thrift.Status; +import org.easymock.EasyMock; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.powermock.api.easymock.PowerMock; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +@RunWith(PowerMockRunner.class) +@PrepareForTest(Gson.class) +public class JsonCodecTest { + + private static final Codec<ServiceInstance> STANDARD_JSON_CODEC = new JsonCodec(); + + @Test + public void testJsonCodecRoundtrip() throws Exception { + Codec<ServiceInstance> codec = STANDARD_JSON_CODEC; + ServiceInstance instance1 = new ServiceInstance( + new Endpoint("foo", 1000), + ImmutableMap.of("http", new Endpoint("foo", 8080)), + Status.ALIVE) + .setShard(0); + byte[] data = ServerSets.serializeServiceInstance(instance1, codec); + assertTrue(ServerSets.deserializeServiceInstance(data, codec).getServiceEndpoint().isSetPort()); + assertTrue(ServerSets.deserializeServiceInstance(data, codec).isSetShard()); + + ServiceInstance instance2 = new ServiceInstance( + new Endpoint("foo", 1000), + ImmutableMap.of("http-admin1", new Endpoint("foo", 8080)), + Status.ALIVE); + data = ServerSets.serializeServiceInstance(instance2, codec); + assertTrue(ServerSets.deserializeServiceInstance(data, codec).getServiceEndpoint().isSetPort()); + assertFalse(ServerSets.deserializeServiceInstance(data, codec).isSetShard()); + + ServiceInstance instance3 = new ServiceInstance( + new Endpoint("foo", 1000), + ImmutableMap.<String, Endpoint>of(), + Status.ALIVE); + data = ServerSets.serializeServiceInstance(instance3, codec); + assertTrue(ServerSets.deserializeServiceInstance(data, codec).getServiceEndpoint().isSetPort()); + assertFalse(ServerSets.deserializeServiceInstance(data, codec).isSetShard()); + } + + @Test + public void testJsonCompatibility() throws IOException { + ServiceInstance instance = new ServiceInstance( + new Endpoint("foo", 1000), + ImmutableMap.of("http", new Endpoint("foo", 8080)), + Status.ALIVE).setShard(42); + + ByteArrayOutputStream results = new ByteArrayOutputStream(); + STANDARD_JSON_CODEC.serialize(instance, results); + assertEquals( + "{\"serviceEndpoint\":{\"host\":\"foo\",\"port\":1000}," + + "\"additionalEndpoints\":{\"http\":{\"host\":\"foo\",\"port\":8080}}," + + "\"status\":\"ALIVE\"," + + "\"shard\":42}", + results.toString()); + } + + @Test + public void testInvalidSerialize() { + // Gson is final so we need to call on PowerMock here. + Gson gson = PowerMock.createMock(Gson.class); + gson.toJson(EasyMock.isA(Object.class), EasyMock.isA(Appendable.class)); + EasyMock.expectLastCall().andThrow(new JsonIOException("error")); + PowerMock.replay(gson); + + ServiceInstance instance = + new ServiceInstance(new Endpoint("foo", 1000), ImmutableMap.of(), Status.ALIVE); + + try { + new JsonCodec(gson).serialize(instance, new ByteArrayOutputStream()); + fail(); + } catch (IOException e) { + // Expected. + } + + PowerMock.verify(gson); + } + + @Test + public void testDeserializeMinimal() throws IOException { + String minimal = "{\"serviceEndpoint\":{\"host\":\"foo\",\"port\":1000},\"status\":\"ALIVE\"}"; + ByteArrayInputStream source = new ByteArrayInputStream(minimal.getBytes(Charsets.UTF_8)); + ServiceInstance actual = STANDARD_JSON_CODEC.deserialize(source); + ServiceInstance expected = + new ServiceInstance(new Endpoint("foo", 1000), ImmutableMap.of(), Status.ALIVE); + assertEquals(expected, actual); + } + + @Test + public void testInvalidDeserialize() { + // Not JSON. + assertInvalidDeserialize(new byte[] {0xC, 0xA, 0xF, 0xE}); + + // No JSON object. + assertInvalidDeserialize(""); + assertInvalidDeserialize("[]"); + + // Missing required fields. + assertInvalidDeserialize("{}"); + assertInvalidDeserialize("{\"serviceEndpoint\":{\"host\":\"foo\",\"port\":1000}}"); + assertInvalidDeserialize("{\"status\":\"ALIVE\"}"); + } + + private void assertInvalidDeserialize(String data) { + assertInvalidDeserialize(data.getBytes(Charsets.UTF_8)); + } + + private void assertInvalidDeserialize(byte[] data) { + try { + STANDARD_JSON_CODEC.deserialize(new ByteArrayInputStream(data)); + fail(); + } catch (IOException e) { + // Expected. + } + } +} http://git-wip-us.apache.org/repos/asf/aurora/blob/140b6e7c/commons/src/test/java/org/apache/aurora/common/zookeeper/ServerSetTest.java ---------------------------------------------------------------------- diff --git a/commons/src/test/java/org/apache/aurora/common/zookeeper/ServerSetTest.java b/commons/src/test/java/org/apache/aurora/common/zookeeper/ServerSetTest.java deleted file mode 100644 index b48c1f1..0000000 --- a/commons/src/test/java/org/apache/aurora/common/zookeeper/ServerSetTest.java +++ /dev/null @@ -1,78 +0,0 @@ -/** - * Licensed 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 - * - * http://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 org.apache.aurora.common.zookeeper; - -import java.io.ByteArrayOutputStream; -import java.io.IOException; - -import com.google.common.collect.ImmutableMap; - -import org.apache.aurora.common.io.Codec; -import org.apache.aurora.common.thrift.Endpoint; -import org.apache.aurora.common.thrift.ServiceInstance; -import org.apache.aurora.common.thrift.Status; -import org.junit.Test; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -public class ServerSetTest { - - @Test - public void testJsonCodecRoundtrip() throws Exception { - Codec<ServiceInstance> codec = ServerSet.JSON_CODEC; - ServiceInstance instance1 = new ServiceInstance( - new Endpoint("foo", 1000), - ImmutableMap.of("http", new Endpoint("foo", 8080)), - Status.ALIVE) - .setShard(0); - byte[] data = ServerSets.serializeServiceInstance(instance1, codec); - assertTrue(ServerSets.deserializeServiceInstance(data, codec).getServiceEndpoint().isSetPort()); - assertTrue(ServerSets.deserializeServiceInstance(data, codec).isSetShard()); - - ServiceInstance instance2 = new ServiceInstance( - new Endpoint("foo", 1000), - ImmutableMap.of("http-admin1", new Endpoint("foo", 8080)), - Status.ALIVE); - data = ServerSets.serializeServiceInstance(instance2, codec); - assertTrue(ServerSets.deserializeServiceInstance(data, codec).getServiceEndpoint().isSetPort()); - assertFalse(ServerSets.deserializeServiceInstance(data, codec).isSetShard()); - - ServiceInstance instance3 = new ServiceInstance( - new Endpoint("foo", 1000), - ImmutableMap.<String, Endpoint>of(), - Status.ALIVE); - data = ServerSets.serializeServiceInstance(instance3, codec); - assertTrue(ServerSets.deserializeServiceInstance(data, codec).getServiceEndpoint().isSetPort()); - assertFalse(ServerSets.deserializeServiceInstance(data, codec).isSetShard()); - } - - @Test - public void testJsonCompatibility() throws IOException { - ServiceInstance instance = new ServiceInstance( - new Endpoint("foo", 1000), - ImmutableMap.of("http", new Endpoint("foo", 8080)), - Status.ALIVE).setShard(42); - - ByteArrayOutputStream results = new ByteArrayOutputStream(); - ServerSet.JSON_CODEC.serialize(instance, results); - assertEquals( - "{\"serviceEndpoint\":{\"host\":\"foo\",\"port\":1000}," - + "\"additionalEndpoints\":{\"http\":{\"host\":\"foo\",\"port\":8080}}," - + "\"status\":\"ALIVE\"," - + "\"shard\":42}", - results.toString()); - } -}
