Hi guys, reviwes welcomed since it touches a sensitive area
long story short: the resizing was using size/4 but we can have read way more data so adjusting it more if better also the isClose was skipping buffer reuse it seems Romain ---------- Forwarded message ---------- From: <[email protected]> Date: 2018-02-20 18:42 GMT+01:00 Subject: johnzon git commit: JOHNZON-158 ensure we resize correctly the buffer even when sizes of the internal buffers are not in the expected area + ensure we call close correctly in terms of scope to avoid surprises and leaking buffers To: [email protected] Repository: johnzon Updated Branches: refs/heads/master 57f307cc6 -> d579e1381 JOHNZON-158 ensure we resize correctly the buffer even when sizes of the internal buffers are not in the expected area + ensure we call close correctly in terms of scope to avoid surprises and leaking buffers Project: http://git-wip-us.apache.org/repos/asf/johnzon/repo Commit: http://git-wip-us.apache.org/repos/asf/johnzon/commit/d579e138 Tree: http://git-wip-us.apache.org/repos/asf/johnzon/tree/d579e138 Diff: http://git-wip-us.apache.org/repos/asf/johnzon/diff/d579e138 Branch: refs/heads/master Commit: d579e1381cf1842f08f35f8bf216c349dc123771 Parents: 57f307c Author: Romain Manni-Bucau <[email protected]> Authored: Tue Feb 20 18:42:05 2018 +0100 Committer: Romain Manni-Bucau <[email protected]> Committed: Tue Feb 20 18:42:05 2018 +0100 ---------------------------------------------------------------------- .../johnzon/core/JsonStreamParserImpl.java | 4 +- .../jsonb/DynamicBufferResizingTest.java | 93 ++++++++++++++++++++ .../java/org/apache/johnzon/mapper/Mapper.java | 85 ++++++++---------- .../johnzon/mapper/MappingGeneratorImpl.java | 7 +- .../johnzon/mapper/MappingParserImpl.java | 11 ++- .../apache/johnzon/core/TestBufferProvider.java | 2 +- 6 files changed, 143 insertions(+), 59 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/johnzon/blob/ d579e138/johnzon-core/src/main/java/org/apache/johnzon/ core/JsonStreamParserImpl.java ---------------------------------------------------------------------- diff --git a/johnzon-core/src/main/java/org/apache/johnzon/core/JsonStreamParserImpl.java b/johnzon-core/src/main/java/org/apache/johnzon/core/ JsonStreamParserImpl.java index 44c4ee9..4d0571a 100644 --- a/johnzon-core/src/main/java/org/apache/johnzon/core/ JsonStreamParserImpl.java +++ b/johnzon-core/src/main/java/org/apache/johnzon/core/ JsonStreamParserImpl.java @@ -170,7 +170,7 @@ public class JsonStreamParserImpl extends JohnzonJsonParserImpl implements JsonC throw new ArrayIndexOutOfBoundsException("Buffer too small for such a long string"); } - final char[] newArray = new char[fallBackCopyBuffer.length + getBufferExtends(fallBackCopyBuffer.length)]; + final char[] newArray = new char[fallBackCopyBuffer.length + Math.max(getBufferExtends(fallBackCopyBuffer.length), length)]; // TODO: log to adjust size once? System.arraycopy(fallBackCopyBuffer, 0, newArray, 0, fallBackCopyBufferLength); System.arraycopy(buffer, startOfValueInBuffer, newArray, fallBackCopyBufferLength, length); @@ -192,7 +192,7 @@ public class JsonStreamParserImpl extends JohnzonJsonParserImpl implements JsonC * @return the amount of bytes the current buffer should get extended with */ protected int getBufferExtends(int currentLength) { - return currentLength/4; + return currentLength / 4; } http://git-wip-us.apache.org/repos/asf/johnzon/blob/ d579e138/johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/ DynamicBufferResizingTest.java ---------------------------------------------------------------------- diff --git a/johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/DynamicBufferResizingTest.java b/johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/ DynamicBufferResizingTest.java new file mode 100644 index 0000000..6345a9b --- /dev/null +++ b/johnzon-jsonb/src/test/java/org/apache/johnzon/jsonb/ DynamicBufferResizingTest.java @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 + * + * 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.johnzon.jsonb; + +import static org.junit.Assert.assertEquals; + +import javax.json.bind.Jsonb; +import javax.json.bind.JsonbBuilder; +import javax.json.bind.JsonbConfig; +import javax.json.bind.annotation.JsonbCreator; +import javax.json.bind.annotation.JsonbProperty; +import javax.json.bind.annotation.JsonbPropertyOrder; +import javax.json.bind.config.BinaryDataStrategy; + +import org.junit.Test; + +public class DynamicBufferResizingTest { + @Test + public void main() { + final JsonbConfig config = new JsonbConfig() + .withFormatting(Boolean.TRUE) + .withBinaryDataStrategy(BinaryDataStrategy.BASE_64); + Jsonb jsonb = JsonbBuilder.create(config); + + final Request request = new Request("Screenshot.png", "image/png", new byte[558140]); + String json = jsonb.toJson(request); + + // the first call works + for (int i = 0; i < 10; i++) { // originally the second call was failling + final Request fromJson = jsonb.fromJson(json, Request.class); + assertEquals("Screenshot.png", fromJson.name); + assertEquals("image/png", fromJson.mimeType); + assertEquals(558140, fromJson.body.length); + } + } + + @JsonbPropertyOrder(value = {"name", "mimeType"}) + public static class Request { + private String name; + private String mimeType; + private byte[] body; + + @JsonbCreator + public Request( + final @JsonbProperty("name") String name, + final @JsonbProperty("mimeType") String mimeType, + final @JsonbProperty("body") byte[] body) { + this.name = name; + this.mimeType = mimeType; + this.body = body; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public String getMimeType() { + return mimeType; + } + + public void setMimeType(String mimeType) { + this.mimeType = mimeType; + } + + public byte[] getBody() { + return body; + } + + public void setBody(byte[] body) { + this.body = body; + } + } +} http://git-wip-us.apache.org/repos/asf/johnzon/blob/ d579e138/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/Mapper.java ---------------------------------------------------------------------- diff --git a/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/Mapper.java b/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/Mapper.java index 4bcc775..da28ae7 100644 --- a/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/Mapper.java +++ b/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/Mapper.java @@ -21,7 +21,6 @@ package org.apache.johnzon.mapper; import org.apache.johnzon.mapper.internal.JsonPointerTracker; import org.apache.johnzon.mapper.reflection.JohnzonCollectionType; -import javax.json.JsonException; import javax.json.JsonReader; import javax.json.JsonReaderFactory; import javax.json.JsonValue; @@ -86,9 +85,10 @@ public class Mapper implements Closeable { } public <T> void writeArray(final Collection<T> object, final Writer stream) { - JsonGenerator generator = generatorFactory. createGenerator(stream(stream)); - boolean dedup = Boolean.TRUE.equals(config.isDeduplicateObjects()); - writeObject(object, generator, null, dedup ? new JsonPointerTracker(null, "/") : null); + try (final JsonGenerator generator = generatorFactory. createGenerator(stream(stream))) { + boolean dedup = Boolean.TRUE.equals(config. isDeduplicateObjects()); + writeObject(object, generator, null, dedup ? new JsonPointerTracker(null, "/") : null); + } } public <T> void writeIterable(final Iterable<T> object, final OutputStream stream) { @@ -96,9 +96,10 @@ public class Mapper implements Closeable { } public <T> void writeIterable(final Iterable<T> object, final Writer stream) { - JsonGenerator generator = generatorFactory. createGenerator(stream(stream)); - boolean dedup = Boolean.TRUE.equals(config.isDeduplicateObjects()); - writeObject(object, generator, null, dedup ? new JsonPointerTracker(null, "/") : null); + try (final JsonGenerator generator = generatorFactory. createGenerator(stream(stream))) { + boolean dedup = Boolean.TRUE.equals(config. isDeduplicateObjects()); + writeObject(object, generator, null, dedup ? new JsonPointerTracker(null, "/") : null); + } } public void writeObject(final Object object, final Writer stream) { @@ -128,9 +129,10 @@ public class Mapper implements Closeable { return; } - final JsonGenerator generator = generatorFactory. createGenerator(stream(stream)); - - writeObject(object, generator, null, isDeduplicateObjects(object.getClass()) ? new JsonPointerTracker(null, "/") : null); + try (final JsonGenerator generator = generatorFactory. createGenerator(stream(stream))) { + writeObject(object, generator, null, + isDeduplicateObjects(object.getClass()) ? new JsonPointerTracker(null, "/") : null); + } } private boolean isDeduplicateObjects(Class<?> rootType) { @@ -156,29 +158,7 @@ public class Mapper implements Closeable { private void writeObject(final Object object, final JsonGenerator generator, final Collection<String> ignored, JsonPointerTracker jsonPointer) { MappingGeneratorImpl mappingGenerator = new MappingGeneratorImpl(config, generator, mappings, jsonPointer != null); - - Throwable originalException = null; - try { - mappingGenerator.doWriteObject(object, generator, true, ignored, jsonPointer); - } catch (final Error | RuntimeException e) { - originalException = e; - } finally { - - try { - generator.close(); - } catch (JsonException e) { - - if (originalException != null) { - if (RuntimeException.class.isInstance(originalException)) { - throw RuntimeException.class.cast( originalException); - } - throw Error.class.cast(originalException); // stackoverflow falls here - } else { - throw e; - } - } - } - + mappingGenerator.doWriteObject(object, generator, true, ignored, jsonPointer); } public String writeArrayAsString(final Collection<?> instance) { @@ -205,15 +185,22 @@ public class Mapper implements Closeable { } public <T> T readObject(final Reader stream, final Type clazz) { - return mapObject(clazz, readerFactory.createReader( stream(stream))); + try (final JsonReader reader = readerFactory.createReader(stream(stream))) { + return mapObject(clazz, reader); + } } public <T> T readObject(final InputStream stream, final Type clazz) { - return mapObject(clazz, charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset)); + try (final JsonReader reader = charset == null ? readerFactory.createReader(stream(stream)) : readerFactory.createReader( + stream(stream), charset)) { + return mapObject(clazz, reader); + } } public <T> Collection<T> readCollection(final InputStream stream, final ParameterizedType genericType) { - return mapObject(genericType, charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset)); + try (final JsonReader reader = charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset)) { + return mapObject(genericType, reader); + } } public <T> T readJohnzonCollection(final InputStream stream, final JohnzonCollectionType<T> genericType) { @@ -225,27 +212,33 @@ public class Mapper implements Closeable { } public <T> Collection<T> readCollection(final Reader stream, final ParameterizedType genericType) { - return mapObject(genericType, readerFactory.createReader( stream(stream))); + try (final JsonReader reader = readerFactory.createReader(stream(stream))) { + return mapObject(genericType, reader); + } } public <T> T[] readArray(final Reader stream, final Class<T> clazz) { - final JsonReader reader = readerFactory.createReader( stream(stream)); - return (T[]) mapArray(clazz, reader); + try (final JsonReader reader = readerFactory.createReader(stream(stream))) { + return (T[]) mapArray(clazz, reader); + } } public <T> T readTypedArray(final InputStream stream, final Class<?> elementType, final Class<T> arrayType) { - final JsonReader reader = charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset); - return arrayType.cast(mapArray(elementType, reader)); + try (final JsonReader reader = charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset)) { + return arrayType.cast(mapArray(elementType, reader)); + } } public <T> T readTypedArray(final Reader stream, final Class<?> elementType, final Class<T> arrayType) { - final JsonReader reader = readerFactory.createReader( stream(stream)); - return arrayType.cast(mapArray(elementType, reader)); + try (final JsonReader reader = readerFactory.createReader(stream(stream))) { + return arrayType.cast(mapArray(elementType, reader)); + } } public <T> T[] readArray(final InputStream stream, final Class<T> clazz) { - final JsonReader reader = charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset); - return (T[]) mapArray(clazz, reader); + try (final JsonReader reader = charset == null ? readerFactory.createReader(stream(stream)): readerFactory.createReader(stream(stream), charset)) { + return (T[]) mapArray(clazz, reader); + } } private Object mapArray(final Class<?> clazz, final JsonReader reader) { @@ -282,7 +275,7 @@ public class Mapper implements Closeable { c.close(); } catch (final IOException e) { if (errors == null) { - errors = new ArrayList<Exception>(); + errors = new ArrayList<>(); } errors.add(e); } http://git-wip-us.apache.org/repos/asf/johnzon/blob/ d579e138/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingGeneratorImpl.java ---------------------------------------------------------------------- diff --git a/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingGeneratorImpl.java b/johnzon-mapper/src/main/ java/org/apache/johnzon/mapper/MappingGeneratorImpl.java index 773e615..3fa054a 100644 --- a/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingGeneratorImpl.java +++ b/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingGeneratorImpl.java @@ -18,16 +18,15 @@ */ package org.apache.johnzon.mapper; -import org.apache.johnzon.mapper.internal.AdapterKey; import org.apache.johnzon.mapper.internal.JsonPointerTracker; import javax.json.JsonValue; import javax.json.stream.JsonGenerator; -import javax.xml.bind.DatatypeConverter; import java.lang.reflect.Array; import java.lang.reflect.InvocationTargetException; import java.math.BigDecimal; import java.math.BigInteger; +import java.util.Base64; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -329,12 +328,12 @@ public class MappingGeneratorImpl implements MappingGenerator { } if(config.isTreatByteArrayAsBase64() && (type == byte[].class /*|| type == Byte[].class*/)) { - String base64EncodedByteArray = DatatypeConverter.printBase64Binary((byte[]) value); + String base64EncodedByteArray = Base64.getEncoder().encodeToString((byte[]) value); generator.write(key, base64EncodedByteArray); return; } if(config.isTreatByteArrayAsBase64URL() && (type == byte[].class /*|| type == Byte[].class*/)) { - generator.write(key, String.valueOf(Adapter.class. cast(config.getAdapters().get(new AdapterKey(byte[].class, String.class))).to(value))); + generator.write(key, Base64.getUrlEncoder().encodeToString((byte[]) value)); return; } http://git-wip-us.apache.org/repos/asf/johnzon/blob/ d579e138/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingParserImpl.java ---------------------------------------------------------------------- diff --git a/johnzon-mapper/src/main/java/org/apache/johnzon/mapper/MappingParserImpl.java b/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingParserImpl.java index 18af2ec..8e74e3c 100644 --- a/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingParserImpl.java +++ b/johnzon-mapper/src/main/java/org/apache/johnzon/ mapper/MappingParserImpl.java @@ -33,7 +33,6 @@ import javax.json.JsonReader; import javax.json.JsonString; import javax.json.JsonStructure; import javax.json.JsonValue; -import javax.xml.bind.DatatypeConverter; import java.lang.reflect.Array; import java.lang.reflect.InvocationTargetException; @@ -45,6 +44,7 @@ import java.math.BigDecimal; import java.math.BigInteger; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Base64; import java.util.Collection; import java.util.Collections; import java.util.Deque; @@ -125,10 +125,6 @@ public class MappingParserImpl implements MappingParser { return readObject(jsonReader.readValue(), targetType); } catch (final NoSuchMethodError noSuchMethodError) { // jsonp 1.0 fallback - mainly for tests return readObject(jsonReader.read(), targetType); - } finally { - if (config.isClose()) { - jsonReader.close(); - } } } @@ -482,7 +478,10 @@ public class MappingParserImpl implements MappingParser { } if (config.isTreatByteArrayAsBase64() && jsonValue.getValueType() == JsonValue.ValueType.STRING && (type == byte[].class /*|| type == Byte[].class*/)) { - return DatatypeConverter.parseBase64Binary(((JsonString) jsonValue).getString()); + return Base64.getDecoder().decode(((JsonString) jsonValue).getString()); + } + if (config.isTreatByteArrayAsBase64URL() && jsonValue.getValueType() == JsonValue.ValueType.STRING && (type == byte[].class /*|| type == Byte[].class*/)) { + return Base64.getUrlDecoder().decode(((JsonString) jsonValue).getString()); } if (Object.class == type) { // handling specific types here to keep exception in standard handling http://git-wip-us.apache.org/repos/asf/johnzon/blob/ d579e138/johnzon-mapper/src/test/java/org/apache/johnzon/ core/TestBufferProvider.java ---------------------------------------------------------------------- diff --git a/johnzon-mapper/src/test/java/org/apache/johnzon/core/TestBufferProvider.java b/johnzon-mapper/src/test/java/org/apache/johnzon/core/ TestBufferProvider.java index 532d32c..93733d9 100644 --- a/johnzon-mapper/src/test/java/org/apache/johnzon/core/ TestBufferProvider.java +++ b/johnzon-mapper/src/test/java/org/apache/johnzon/core/ TestBufferProvider.java @@ -69,7 +69,7 @@ public class TestBufferProvider implements BufferStrategy.BufferProvider<char[]> } public int newBufferCalls() { - return releaseCalls.get(); + return newBufferCalls.get(); } public void clear() {
