Feature/concurrency tests #6
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,50 +1,72 @@ | ||
| package io.datanerds.avropatch; | ||
|
|
||
| import io.datanerds.avropatch.operation.DefaultSchema; | ||
| import io.datanerds.avropatch.operation.Operation; | ||
| import io.datanerds.avropatch.value.conversion.*; | ||
| import org.apache.avro.Schema; | ||
| import org.apache.avro.io.*; | ||
| import org.apache.avro.reflect.AvroSchema; | ||
|
|
||
| import javax.annotation.concurrent.ThreadSafe; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.IOException; | ||
| import java.util.*; | ||
|
|
||
| /** | ||
| * This class represents a JSON PATCH operation holding a sequence of operations to apply to a given object. | ||
| * | ||
| * @see <a href="https://tools.ietf.org/html/rfc6902">https://tools.ietf.org/html/rfc6902</a> | ||
| */ | ||
| @ThreadSafe | ||
|
There was a problem hiding this comment. I think helper methods for |
||
| public class Patch { | ||
|
|
||
| private static final PatchSerializer SERIALIZER = new PatchSerializer(); | ||
|
There was a problem hiding this comment. Why is domain and (de)-serialization combined? |
||
|
|
||
| @AvroSchema(DefaultSchema.HEADERS) | ||
| private final Map<String, ?> headers; | ||
|
|
||
| private final List<Operation> operations; | ||
|
|
||
| @SuppressWarnings("unused") // no arg constructor needed by Avro | ||
|
There was a problem hiding this comment. rename to: no-arg constructor, otherwise the meaning is that an arg constructor is not needed. Change everywhere else, too. |
||
| private Patch() { | ||
| this(new ArrayList<>(), new HashMap<>()); | ||
| } | ||
|
|
||
| public Patch(List<Operation> operations) { | ||
|
There was a problem hiding this comment. Always a good idea to declare method arguments as final (especially in case of collections) which makes it explicit to the calling function that the list would not be modified in the called method. There was a problem hiding this comment. Disagree. The Agree though on parameter declarations being implicitly |
||
| this(operations, Collections.EMPTY_MAP); | ||
| } | ||
|
There was a problem hiding this comment. Why not call the two-argument constructor here? This would follow the same principle as the no-argument constructor... |
||
|
|
||
| public Patch(List<Operation> operations, Map<String, ?> headers) { | ||
|
There was a problem hiding this comment. Should we do a sanity check that the value instances are of a supported type? There was a problem hiding this comment. I created issue #10 for this feature |
||
| this.operations = new ArrayList<>(operations); | ||
|
There was a problem hiding this comment. Following the pattern above, |
||
| this.headers = new HashMap<>(headers); | ||
| } | ||
|
|
||
| public List<Operation> getOperations() { | ||
| return Collections.unmodifiableList(operations); | ||
| } | ||
|
|
||
| public Map<String, ?> getHeaders() { | ||
|
There was a problem hiding this comment. Not a concurrency issue with the map as long as the map is not open to modification. It might not be the case right now but leaves the code open to errors in case there is a write interface exposed to the map erroneously in the future. Would suggest initializing the map as immutable during initialization in the constructor. There was a problem hiding this comment. the map should not be open to modifications There was a problem hiding this comment. It's not: There was a problem hiding this comment. Agreed that it is not open to modifications to external modifiers but from a code extensibility perspective, if there is a method in this class which tries to modify this map, extra care would need to be taken to ensure that the map is made immutable then. By making it immutable now, we can future proof it so that we don't run the risk of somebody not making it immutable/thread safe while adding code modifying it in future. |
||
| return Collections.unmodifiableMap(headers); | ||
| } | ||
|
|
||
| public byte[] toBytes() throws IOException { | ||
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); | ||
| Encoder binaryEncoder = EncoderFactory.get().directBinaryEncoder(outputStream, null); | ||
|
There was a problem hiding this comment. this local variables binaryEncoder and outputStream can probably shortened for readability, as they are used only once and right after declaration. (see heuristic N5 in Clean Code). |
||
| SERIALIZER.writer.write(this, binaryEncoder); | ||
| return outputStream.toByteArray(); | ||
| } | ||
|
|
||
| public static Patch of(byte[] bytes) throws IOException { | ||
| return SERIALIZER.reader.read(null, DecoderFactory.get().binaryDecoder(bytes, null)); | ||
|
There was a problem hiding this comment. I assume we are not reusing the read "Patch" object here because it is being returned to an external method as it is, correct? There was a problem hiding this comment. Yes and we'd like to have thread-safety |
||
| } | ||
|
|
||
| private static class PatchSerializer { | ||
| final DatumWriter<Patch> writer; | ||
| final DatumReader<Patch> reader; | ||
|
|
||
| private PatchSerializer() { | ||
| Schema schema = AvroData.get().getSchema(Patch.class); | ||
| writer = AvroData.get().createDatumWriter(schema); | ||
| reader = AvroData.get().createDatumReader(schema); | ||
|
There was a problem hiding this comment. maybe |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| package io.datanerds.avropatch.operation; | ||
|
|
||
| /** | ||
| * This interface holds the schema for all valid value types. | ||
| */ | ||
| public interface DefaultSchema { | ||
| String VALUE = | ||
|
There was a problem hiding this comment. Eventually move to file and load with There was a problem hiding this comment. Would probably make it more readable? There was a problem hiding this comment. This would not compile since the value is used in an annotation (only constants allowed there). Would need an indirection for compilation. I am adding a card to the backlog There was a problem hiding this comment. This would not compile per se since the value is used in an annotation (only constants allowed there). Would need an indirection for compilation. I am adding a card to the backlog There was a problem hiding this comment. In that case generating that file from a json is probably the easiest? |
||
| "[{" | ||
| + " \"type\": \"record\"," | ||
| + " \"name\": \"decimal\"," | ||
| + " \"fields\": [{" | ||
| + " \"name\": \"unscaledValue\"," | ||
| + " \"type\": {" | ||
| + " \"type\": \"bytes\"," | ||
| + " \"logicalType\": \"big-integer\"" | ||
| + " }" | ||
| + " }, {" | ||
| + " \"name\": \"scale\"," | ||
| + " \"type\": \"int\"" | ||
| + " }]," | ||
| + " \"logicalType\": \"big-decimal\"" | ||
| + "}, {" | ||
| + " \"type\": \"bytes\"," | ||
| + " \"logicalType\": \"big-integer\"" | ||
| + "}, \"boolean\", {" | ||
| + " \"type\": \"fixed\"," | ||
| + " \"name\": \"timestamp\"," | ||
| + " \"size\": 8," | ||
| + " \"logicalType\": \"timestamp\"" | ||
| + "}, \"double\", \"float\", \"int\", \"long\", \"null\", \"string\", {" | ||
| + " \"type\": \"fixed\"," | ||
| + " \"name\": \"uuid\"," | ||
| + " \"size\": 16," | ||
| + " \"logicalType\": \"uuid\"" | ||
| + "}, {" | ||
| + " \"type\": \"array\"," | ||
| + " \"items\": [\"decimal\", {" | ||
| + " \"type\": \"bytes\"," | ||
| + " \"logicalType\": \"big-integer\"" | ||
| + " }, \"boolean\", \"timestamp\", \"double\", \"float\", \"int\", \"long\", \"null\", \"string\", \"uuid\"]" | ||
| + "}, {\n" | ||
| + " \"type\": \"map\"," | ||
| + " \"values\": [\"decimal\", {" | ||
| + " \"type\": \"bytes\"," | ||
| + " \"logicalType\": \"big-integer\"" | ||
| + " }, \"boolean\", \"timestamp\", \"double\", \"float\", \"int\", \"long\", \"null\", \"string\", \"uuid\"]" | ||
| + "}]"; | ||
|
|
||
| String HEADERS = | ||
| "{\n" | ||
| + " \"type\": \"map\",\n" | ||
| + " \"values\": [\"boolean\", \"double\", \"float\", \"int\", \"long\", \"string\", {\n" | ||
| + " \"type\": \"record\",\n" | ||
| + " \"name\": \"decimal\",\n" | ||
| + " \"doc\": \"BigDecimal value represented via it's scale and unscaled value.\",\n" | ||
| + " \"fields\": [{\n" | ||
| + " \"name\": \"unscaledValue\",\n" | ||
| + " \"type\": {\n" | ||
| + " \"type\": \"bytes\",\n" | ||
| + " \"logicalType\": \"big-integer\"\n" | ||
| + " }\n" | ||
| + " }, {\n" | ||
| + " \"name\": \"scale\",\n" | ||
| + " \"type\": \"int\"\n" | ||
| + " }],\n" | ||
| + " \"logicalType\": \"big-decimal\"\n" | ||
| + " }, {\n" | ||
| + " \"type\": \"bytes\",\n" | ||
| + " \"logicalType\": \"big-integer\"\n" | ||
| + " }, {\n" | ||
| + " \"type\": \"fixed\",\n" | ||
| + " \"name\": \"timestamp\",\n" | ||
| + " \"doc\": \"Timestamp representing the number of milliseconds since January 1, 1970, 00:00:00 GMT\",\n" | ||
| + " \"size\": 8,\n" | ||
| + " \"logicalType\": \"timestamp\"\n" | ||
| + " }, {\n" | ||
| + " \"type\": \"fixed\",\n" | ||
| + " \"name\": \"uuid\",\n" | ||
| + " \"doc\": \"UUID serialized via two long values: It's most significant and least significant 64 bits.\",\n" | ||
| + " \"size\": 16,\n" | ||
| + " \"logicalType\": \"uuid\"\n" | ||
| + " }]\n" | ||
| + "}"; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slf4j version should become a variable since it is also used in testCompile