Skip to content
This repository has been archived by the owner on Jun 7, 2024. It is now read-only.

Commit

Permalink
Merge pull request #888 from zalando/ARUHA-1680-final-switch-to-strict
Browse files Browse the repository at this point in the history
ARUHA-1680: switched to strict validation; removed feature toggle;
  • Loading branch information
v-stepanov authored Jun 13, 2018
2 parents 0d77122 + 16684d6 commit 857c84c
Show file tree
Hide file tree
Showing 8 changed files with 28 additions and 45 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

## [2.7.4] - 2018-06-13

### Changed
- Switched to strict json parsing

## [2.7.3] - 2018-06-11

### Changed
Expand Down
12 changes: 3 additions & 9 deletions src/main/java/org/zalando/nakadi/domain/BatchFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ private static int navigateToObjectStart(final int from, final int end, final St
return found ? curPos : -1;
}

private static int navigateToObjectEnd(
final boolean strict, final int from, final int end, final String data,
final Consumer<BatchItem> batchItemConsumer) {
private static int navigateToObjectEnd(final int from, final int end, final String data,
final Consumer<BatchItem> batchItemConsumer) {
int curPos = from;
int nestingLevel = 0;
boolean escaped = false;
Expand Down Expand Up @@ -80,7 +79,6 @@ private static int navigateToObjectEnd(
batchItemConsumer.accept(
new BatchItem(
data.substring(from, curPos + 1),
strict,
BatchItem.EmptyInjectionConfiguration.build(1, hasFields),
injections,
skipPositions));
Expand Down Expand Up @@ -114,16 +112,12 @@ private static BatchItem.InjectionConfiguration extractInjection(
}

public static List<BatchItem> from(final String events) {
return from(events, true);
}

public static List<BatchItem> from(final String events, final boolean strict) {
final List<BatchItem> batch = new ArrayList<>();
int objectStart = locateOpenSquareBracket(events) + 1;
final int arrayEnd = locateClosingSquareBracket(objectStart, events);

while (-1 != (objectStart = navigateToObjectStart(objectStart, arrayEnd, events))) {
final int objectEnd = navigateToObjectEnd(strict, objectStart, arrayEnd, events, batch::add);
final int objectEnd = navigateToObjectEnd(objectStart, arrayEnd, events, batch::add);
if (objectEnd == -1) {
throw new JSONException("Unclosed object staring at " + objectStart + " found.");
}
Expand Down
25 changes: 1 addition & 24 deletions src/main/java/org/zalando/nakadi/domain/BatchItem.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package org.zalando.nakadi.domain;

import org.json.JSONObject;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.Nullable;
import java.nio.charset.StandardCharsets;
Expand All @@ -13,8 +11,6 @@

public class BatchItem {

private static final Logger LOG = LoggerFactory.getLogger(BatchItem.class);

public enum Injection {
METADATA("metadata");
public final String name;
Expand Down Expand Up @@ -69,31 +65,12 @@ public static EmptyInjectionConfiguration build(final int position, final boolea

public BatchItem(
final String rawEvent,
final boolean strictParsing,
final EmptyInjectionConfiguration emptyInjectionConfiguration,
final InjectionConfiguration[] injections,
final List<Integer> skipCharacters) {
this.rawEvent = rawEvent;
this.skipCharacters = skipCharacters;
this.event = new JSONObject(rawEvent);

// We will first release in a shadow mode when the strict parser does second parsing
// and we compare if the result is the same as with the old parser; we also log invalid events;
if (strictParsing) {
try {
final JSONObject shadowEvent = StrictJsonParser.parseObject(rawEvent);
final String shadowOutput = shadowEvent.toString();
final String usualOutput = this.event.toString();
if (!shadowOutput.equals(usualOutput)) {
LOG.debug("[STRICT_JSON_DIFF] Strict parser produced different output for event: {} " +
"Strict parser output: {} Old parser output: {}", rawEvent, shadowOutput, usualOutput);
}
} catch (final Exception e) {
LOG.debug("[STRICT_JSON_DIFF] Failed to parse event with strict parser: {} Error message: {}",
rawEvent, e.getMessage());
}
}

this.event = StrictJsonParser.parseObject(rawEvent);
this.eventSize = rawEvent.getBytes(StandardCharsets.UTF_8).length;
this.emptyInjectionConfiguration = emptyInjectionConfiguration;
this.injections = injections;
Expand Down
13 changes: 12 additions & 1 deletion src/main/java/org/zalando/nakadi/domain/StrictJsonParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@
import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class StrictJsonParser {

private static final Logger LOG = LoggerFactory.getLogger(StrictJsonParser.class);

private static final String POSSIBLE_NUMBER_DIGITS = "0123456789-+.Ee";

private static class StringTokenizer {
Expand Down Expand Up @@ -67,7 +71,14 @@ public static JSONObject parseObject(final String value) throws JSONException {
}

public static JSONObject parse(final String value, final boolean allowMore) throws JSONException {
return (JSONObject) parse(value, 0, value.length(), allowMore);
try {
return (JSONObject) parse(value, 0, value.length(), allowMore);
} catch (final JSONException e) {
// temporary logging
LOG.debug("[STRICT_JSON_FAIL] Failed to parse json with strict parser: {} Error message: {}",
value, e.getMessage());
throw e;
}
}

private static Object parse(final String value, final int startIdx, final int endIdx, final boolean allowMore)
Expand Down
5 changes: 1 addition & 4 deletions src/main/java/org/zalando/nakadi/service/EventPublisher.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@
import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;

import static org.zalando.nakadi.service.FeatureToggleService.Feature.STRICT_JSON_PARSING;

@Component
public class EventPublisher {

Expand Down Expand Up @@ -90,8 +88,7 @@ EventPublishResult publishInternal(final String events,
AccessDeniedException, ServiceTemporarilyUnavailableException {

Closeable publishingCloser = null;
final List<BatchItem> batch = BatchFactory.from(
events, featureToggleService.isFeatureEnabled(STRICT_JSON_PARSING));
final List<BatchItem> batch = BatchFactory.from(events);
try {
publishingCloser = timelineSync.workWithEventType(eventTypeName, nakadiSettings.getTimelineWaitTimeoutMs());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ enum Feature {
SEND_BATCH_VIA_OUTPUT_STREAM("send_batch_via_output_stream"),
REMOTE_TOKENINFO("remote_tokeninfo"),
KPI_COLLECTION("kpi_collection"),
STRICT_JSON_PARSING("strict_json"),
DISABLE_DB_WRITE_OPERATIONS("disable_db_write_operations");

private final String id;
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/zalando/nakadi/domain/BatchItemTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ private static JSONObject restoreJsonObject(final BatchItem bi) {

@Test
public void testBatchItemSizeWithMultiByteChar() {
final BatchItem item = new BatchItem("{ \"name\": \"香港\"} ", true,
final BatchItem item = new BatchItem("{ \"name\": \"香港\"} ",
BatchItem.EmptyInjectionConfiguration.build(1, false),
new BatchItem.InjectionConfiguration[BatchItem.Injection.values().length],
Collections.emptyList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public void canLoadPartitionEndStatistics() {
@Test
public void whenPostEventTimesOutThenUpdateItemStatus() {
final BatchItem item = new BatchItem(
"{}", true,
"{}",
BatchItem.EmptyInjectionConfiguration.build(1, true),
new BatchItem.InjectionConfiguration[BatchItem.Injection.values().length],
Collections.emptyList());
Expand All @@ -248,7 +248,7 @@ public void whenPostEventTimesOutThenUpdateItemStatus() {

@Test
public void whenPostEventOverflowsBufferThenUpdateItemStatus() {
final BatchItem item = new BatchItem("{}", true,
final BatchItem item = new BatchItem("{}",
BatchItem.EmptyInjectionConfiguration.build(1, true),
new BatchItem.InjectionConfiguration[BatchItem.Injection.values().length],
Collections.emptyList());
Expand Down Expand Up @@ -276,11 +276,11 @@ public void whenPostEventOverflowsBufferThenUpdateItemStatus() {
@Test
public void whenKafkaPublishCallbackWithExceptionThenEventPublishingException() {

final BatchItem firstItem = new BatchItem("{}", true, BatchItem.EmptyInjectionConfiguration.build(1, true),
final BatchItem firstItem = new BatchItem("{}", BatchItem.EmptyInjectionConfiguration.build(1, true),
new BatchItem.InjectionConfiguration[BatchItem.Injection.values().length],
Collections.emptyList());
firstItem.setPartition("1");
final BatchItem secondItem = new BatchItem("{}", true, BatchItem.EmptyInjectionConfiguration.build(1, true),
final BatchItem secondItem = new BatchItem("{}", BatchItem.EmptyInjectionConfiguration.build(1, true),
new BatchItem.InjectionConfiguration[BatchItem.Injection.values().length],
Collections.emptyList());
secondItem.setPartition("2");
Expand Down Expand Up @@ -329,7 +329,7 @@ public void whenKafkaPublishTimeoutThenCircuitIsOpened() {
final List<BatchItem> batches = new LinkedList<>();
for (int i = 0; i < 1000; i++) {
try {
final BatchItem batchItem = new BatchItem("{}", true,
final BatchItem batchItem = new BatchItem("{}",
BatchItem.EmptyInjectionConfiguration.build(1, true),
new BatchItem.InjectionConfiguration[BatchItem.Injection.values().length],
Collections.emptyList());
Expand Down

0 comments on commit 857c84c

Please sign in to comment.