rmannibucau commented on code in PR #145:
URL: https://github.com/apache/johnzon/pull/145#discussion_r3856346948
##########
johnzon-mapper/src/main/java/org/apache/johnzon/mapper/jsonp/RewindableJsonParser.java:
##########
@@ -87,41 +109,73 @@ public void close() {
@Override
public JsonObject getObject() {
- return delegate.getObject();
+ return trackResult(delegate::getObject);
}
@Override
public JsonValue getValue() {
- return delegate.getValue();
+ return trackResult(delegate::getValue);
}
@Override
public JsonArray getArray() {
- return delegate.getArray();
+ return trackResult(delegate::getArray);
}
@Override
public Stream<JsonValue> getArrayStream() {
- return delegate.getArrayStream();
+ return trackStream(delegate.getArrayStream());
}
@Override
public Stream<Map.Entry<String, JsonValue>> getObjectStream() {
- return delegate.getObjectStream();
+ return trackStream(delegate.getObjectStream());
}
@Override
public Stream<JsonValue> getValueStream() {
- return delegate.getValueStream();
+ return trackStream(delegate.getValueStream());
}
@Override
public void skipArray() {
- delegate.skipArray();
+ trackAdvance(delegate::skipArray);
}
@Override
public void skipObject() {
- delegate.skipObject();
+ trackAdvance(delegate::skipObject);
+ }
+
+ private <T> T trackResult(final Supplier<T> operation) {
+ pendingRewind = false;
+ final T result = operation.get();
+ last = delegate.currentEvent();
+ return result;
+ }
+
+ private void trackAdvance(final Runnable operation) {
+ pendingRewind = false;
+ operation.run();
+ last = delegate.currentEvent();
+ }
+
+ private <T> Stream<T> trackStream(final Stream<T> stream) {
+ pendingRewind = false;
+ last = delegate.currentEvent();
+ final boolean parallel = stream.isParallel();
+ final Spliterator<T> spliterator = stream.spliterator();
+ final Spliterator<T> tracking = new
Spliterators.AbstractSpliterator<T>(
+ spliterator.estimateSize(), spliterator.characteristics()) {
+ @Override
+ public boolean tryAdvance(final Consumer<? super T> action) {
+ try {
+ return spliterator.tryAdvance(action);
+ } finally {
+ last = delegate.currentEvent();
+ }
+ }
+ };
+ return StreamSupport.stream(tracking, parallel).onClose(stream::close);
Review Comment:
why closing there, the caller must close it
##########
johnzon-mapper/src/main/java/org/apache/johnzon/mapper/jsonp/RewindableJsonParser.java:
##########
@@ -87,41 +109,73 @@ public void close() {
@Override
public JsonObject getObject() {
- return delegate.getObject();
+ return trackResult(delegate::getObject);
}
@Override
public JsonValue getValue() {
- return delegate.getValue();
+ return trackResult(delegate::getValue);
}
@Override
public JsonArray getArray() {
- return delegate.getArray();
+ return trackResult(delegate::getArray);
}
@Override
public Stream<JsonValue> getArrayStream() {
- return delegate.getArrayStream();
+ return trackStream(delegate.getArrayStream());
}
@Override
public Stream<Map.Entry<String, JsonValue>> getObjectStream() {
- return delegate.getObjectStream();
+ return trackStream(delegate.getObjectStream());
}
@Override
public Stream<JsonValue> getValueStream() {
- return delegate.getValueStream();
+ return trackStream(delegate.getValueStream());
}
@Override
public void skipArray() {
- delegate.skipArray();
+ trackAdvance(delegate::skipArray);
}
@Override
public void skipObject() {
- delegate.skipObject();
+ trackAdvance(delegate::skipObject);
+ }
+
+ private <T> T trackResult(final Supplier<T> operation) {
+ pendingRewind = false;
+ final T result = operation.get();
+ last = delegate.currentEvent();
+ return result;
+ }
+
+ private void trackAdvance(final Runnable operation) {
+ pendingRewind = false;
+ operation.run();
+ last = delegate.currentEvent();
+ }
+
+ private <T> Stream<T> trackStream(final Stream<T> stream) {
+ pendingRewind = false;
+ last = delegate.currentEvent();
+ final boolean parallel = stream.isParallel();
Review Comment:
if parallel nothing works I think
##########
johnzon-mapper/src/main/java/org/apache/johnzon/mapper/jsonp/RewindableJsonParser.java:
##########
@@ -42,14 +48,30 @@ public Event getLast() {
@Override
public boolean hasNext() {
- return delegate.hasNext();
+ return pendingRewind || delegate.hasNext();
}
@Override
public Event next() {
+ if (pendingRewind) {
+ pendingRewind = false;
+ return last;
+ }
return last = delegate.next();
}
+ @Override
+ public Event currentEvent() {
+ if (last == null) {
+ if (!delegate.hasNext()) { // value adapters without event stream
+ return delegate.currentEvent();
+ }
+ last = delegate.next();
+ pendingRewind = true;
Review Comment:
looks wrong to impl it there and violates the contract, if I call it on a
new parser it should return null, the method is literally getLast() only (our
getLast was pre JSON-P 2.1 as a minimum target)
##########
johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/JsonValueParserAdapter.java:
##########
@@ -1,145 +1,159 @@
-/*
- * 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 java.math.BigDecimal;
-import java.util.function.Supplier;
-
-import jakarta.json.JsonNumber;
-import jakarta.json.JsonString;
-import jakarta.json.JsonValue;
-import jakarta.json.stream.JsonLocation;
-import jakarta.json.stream.JsonParser;
-import jakarta.json.stream.JsonParserFactory;
-
-import org.apache.johnzon.mapper.jsonp.RewindableJsonParser;
-
-class JsonValueParserAdapter<T extends JsonValue> implements JsonParser {
-
- private static class JsonStringParserAdapter extends
JsonValueParserAdapter<JsonString> {
-
- public JsonStringParserAdapter(JsonString jsonValue) {
- super(jsonValue);
- }
-
- @Override
- public String getString() {
- return getValue().getString();
- }
- }
-
- private static class JsonNumberParserAdapter extends
JsonValueParserAdapter<JsonNumber> {
-
- public JsonNumberParserAdapter(JsonNumber jsonValue) {
- super(jsonValue);
- }
-
- @Override
- public boolean isIntegralNumber() {
- return getValue().isIntegral();
- }
-
- @Override
- public int getInt() {
- return getValue().intValueExact();
- }
-
- @Override
- public long getLong() {
- return getValue().longValueExact();
- }
-
- @Override
- public BigDecimal getBigDecimal() {
- return getValue().bigDecimalValue();
- }
- }
-
- public static JsonParser createFor(final JsonValue jsonValue,
- final Supplier<JsonParserFactory>
parserFactoryProvider) {
- return new RewindableJsonParser(doCreate(jsonValue,
parserFactoryProvider));
- }
-
- private static JsonParser doCreate(final JsonValue jsonValue,
- final Supplier<JsonParserFactory>
parserFactoryProvider) {
- switch (jsonValue.getValueType()) {
- case OBJECT: return
parserFactoryProvider.get().createParser(jsonValue.asJsonObject());
- case ARRAY: return
parserFactoryProvider.get().createParser(jsonValue.asJsonArray());
- case STRING: return new JsonStringParserAdapter((JsonString)
jsonValue);
- case NUMBER: return new JsonNumberParserAdapter((JsonNumber)
jsonValue);
- default: return new JsonValueParserAdapter<>(jsonValue);
- }
- }
-
- private final T jsonValue;
-
- JsonValueParserAdapter(T jsonValue) {
- this.jsonValue = jsonValue;
- }
-
- @Override
- public boolean hasNext() {
- return false;
- }
-
- @Override
- public Event next() {
- throw new UnsupportedOperationException("next() no supported for " +
jsonValue.getValueType());
- }
-
- @Override
- public String getString() {
- throw new UnsupportedOperationException("next() no supported for " +
jsonValue.getValueType());
- }
-
- @Override
- public boolean isIntegralNumber() {
- throw new UnsupportedOperationException("isIntegralNumber() not
supported for " + jsonValue.getValueType());
- }
-
- @Override
- public int getInt() {
- throw new UnsupportedOperationException("getInt() not supported for "
+ jsonValue.getValueType());
- }
-
- @Override
- public long getLong() {
- throw new UnsupportedOperationException("getLong() not supported for "
+ jsonValue.getValueType());
- }
-
- @Override
- public BigDecimal getBigDecimal() {
- throw new UnsupportedOperationException("getBigDecimal() not supported
for " + jsonValue.getValueType());
- }
-
- @Override
- public JsonLocation getLocation() {
- throw new UnsupportedOperationException("getLocation() not supported
for " + jsonValue.getValueType());
- }
-
- @Override
- public void close() {
- // no-op
- }
-
- @Override
- public T getValue() {
- return jsonValue;
- }
-}
+/*
Review Comment:
maybe rework so the diff is accurate? (content is ok once done)
##########
johnzon-mapper/src/main/java/org/apache/johnzon/mapper/jsonp/RewindableJsonParser.java:
##########
@@ -42,14 +48,30 @@ public Event getLast() {
@Override
public boolean hasNext() {
- return delegate.hasNext();
+ return pendingRewind || delegate.hasNext();
}
@Override
public Event next() {
Review Comment:
this potentially changes the behavior so should be refined I think if
desired (looks ok upfront but having a broken test for it can be sane on the
long run)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
