Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARUHA-1680 Use strict json parsing for event type publishing #882

Merged
merged 14 commits into from Jun 7, 2018
Merged

Conversation

@antban
Copy link
Member

@antban antban commented May 30, 2018

One-line summary

ARUHA-1680 Use strict json parsing for event type publishing

"(ෛ@Ǯっ䧼䵤[aテൖvEnAdU렖뗈@볓yꈪ,mԴ|꟢캁(而첸죕CX4Y믅": "2⯩㳿ꢚ훀~迯?᪑\\啚;4X\u20c2襏B箹)俣eỻw䇄",
"75༂f詳䅫ꐧ鏿 }3\u20b5'∓䝱虀f菼Iq鈆﨤g퍩)BFa왢d0뮪痮M鋡nw∵謊;ꝧf美箈ḋ*\u001c`퇚퐋䳫$!V#N㹲抗ⱉ珎(V嵟鬒_b㳅\u0019": null,
"e_m@(i㜀3ꦗ䕯䭰Oc+-련0뭦⢹苿蟰ꂏSV䰭勢덥.ྈ爑Vd,ᕥ=퀍)vz뱊ꈊB_6듯\"?{㒲&㵞뵫疝돡믈%Qw限,?\r枮\"? N~癃ruࡗdn&": null,

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

no newline
:)

@codecov-io
Copy link

@codecov-io codecov-io commented May 30, 2018

Codecov Report

Merging #882 into master will increase coverage by 0.43%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #882      +/-   ##
============================================
+ Coverage     53.62%   54.05%   +0.43%     
- Complexity     1705     1750      +45     
============================================
  Files           314      315       +1     
  Lines          9501     9660     +159     
  Branches        857      887      +30     
============================================
+ Hits           5095     5222     +127     
- Misses         4098     4115      +17     
- Partials        308      323      +15
Impacted Files Coverage Δ Complexity Δ
...g/zalando/nakadi/service/FeatureToggleService.java 53.12% <100%> (+1.51%) 0 <0> (ø) ⬇️
...ava/org/zalando/nakadi/service/EventPublisher.java 91.66% <100%> (+0.17%) 20 <1> (ø) ⬇️
...o/nakadi/controller/EventPublishingController.java 86.11% <100%> (ø) 16 <1> (ø) ⬇️
...n/java/org/zalando/nakadi/domain/BatchFactory.java 85.55% <100%> (+0.16%) 53 <1> (+1) ⬆️
...c/main/java/org/zalando/nakadi/util/JsonUtils.java 83.33% <100%> (ø) 1 <1> (ø) ⬇️
...main/java/org/zalando/nakadi/domain/BatchItem.java 82.14% <68.75%> (-3.01%) 25 <1> (+1)
...va/org/zalando/nakadi/domain/StrictJsonParser.java 79.86% <79.86%> (ø) 42 <42> (?)
...n/java/org/zalando/nakadi/service/EventStream.java 75% <0%> (+1.61%) 30% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d116a2...fe354b6. Read the comment docs.

import org.json.JSONException;
import org.json.JSONObject;

public class StrictJsonParser {

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

Can you please describe which part of this file was written by you and which was borrowed? So that I know where to focus the review.

This comment has been minimized.

@antban

antban May 31, 2018
Author Member

I took readStringTillTheEnd method completely. Everything else is more or less rewritten

if (tokenizer.next(3).equals("rue")) {
return Boolean.TRUE;
} else {
throw syntaxError("Expected false value", tokenizer);

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

Maybe "Expected true value"?

This comment has been minimized.

@antban

antban May 31, 2018
Author Member

yep, mistake

StrictJsonParser.parse("{\"name\":NaN}");
}

}

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

no newline

This comment has been minimized.

@antban

antban May 31, 2018
Author Member

ok

}

@Test(expected = JSONException.class)
public void testBadJson5() {

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

I think it would be better if the test method names describe the actual test case they test.

This comment has been minimized.

@antban

antban May 31, 2018
Author Member

@v-stepanov The problem with this approach is that I can not describe what I am testing against. But if it really matters, I will rewrite tests for failing scenarios completely into 1 and check exception by hands.

private void testSingleString(final String value) {

final JSONObject orthodoxJson = new JSONObject(value);
final JSONObject anarchyJson = (JSONObject) StrictJsonParser.parse(value);

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

I thought that strict parser should generate orthodoxJson

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

StrictJsonParser.parseObject returns JSONObject. No casting will be needed.

This comment has been minimized.

@antban

antban May 31, 2018
Author Member

ok

}
}

public static Object parse(final String value) throws JSONException {

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

why does it return Object, not the JSONObject?

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

Ok, I see now that the next method returns JSONObject. Do we need both of them?

This comment has been minimized.

@v-stepanov

v-stepanov Jun 4, 2018
Contributor

@antban what about this point?

This comment has been minimized.

public void testBadJson5() {
StrictJsonParser.parse("{\"name\":NaN}");
}

This comment has been minimized.

@v-stepanov

v-stepanov May 30, 2018
Contributor

One more case I remember is extra "," like

{
  "blah": "foo",
}

Can we add a test for that?

This comment has been minimized.

@antban

antban May 31, 2018
Author Member

yep

StrictJsonParser.parse("{\"name\":NaN}");
@Test
public void testInvalidJsonFormats() {
final String[] testsData = new String[]{

This comment has been minimized.

This comment has been minimized.

@antban

antban Jun 5, 2018
Author Member

@v-stepanov Do you think that it pays to readability or simplifies development?

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

The problem with your approach is that if you test A, B, C, D, E, F in your test and the test fails for B, it will not even run for C, D, E, F. So you will not have the full picture of which cases fail/successful.

This comment has been minimized.

@antban

antban Jun 5, 2018
Author Member

I know about this, but honestly, I think that it is not the case here. It either fails or succeeds. But as you insist, I will change it.

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

No, for me it's fine like this, I think it's not a big issue.

This comment has been minimized.

@antban

antban Jun 5, 2018
Author Member

@v-stepanov too late b3058d7

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 4, 2018

@antban the parser itself looks good for me. Of course I'm not a compiler so I can't say if it will work or not :)
What is your plan regarding releasing of this?

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 5, 2018

👍

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 5, 2018

👍

1 similar comment
@antban
Copy link
Member Author

@antban antban commented Jun 5, 2018

👍

@antban
Copy link
Member Author

@antban antban commented Jun 5, 2018

deploy validation please

if (nextToken == '}') {
finished = true;
} else if (nextToken != ',') {
throw syntaxError("Unexpected symbol " + nextToken + " while parsing object", tokenizer);

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

can you please put that symbol in single quotes?

if (separator == ']') {
finished = true;
} else if (separator != ',') {
throw syntaxError("Unexpected separator " + separator, tokenizer);

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

can you please put the separator in single quotes?

} catch (JSONException ignore) {
return result;
}
throw syntaxError("Unexpected symbol " + unexpectedValue, stringTokenizer);

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

can you please put that symbol in single quotes?

return (JSONObject) parse(value, true);
}

public static Object parse(final String value, final boolean allowMore) throws JSONException {

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

There is still the public parse method which returns plain Object. I believe we don't need it as public method here, right?

}
}

public static JSONObject parseObject(final String value) throws JSONException {

This comment has been minimized.

@v-stepanov

v-stepanov Jun 5, 2018
Contributor

Is it possible to provide more context when showing error to user? Currently it looks like:

{
  "type": "http://httpstatus.es/400",
  "title": "Bad Request",
  "status": 400,
  "detail": "Expected false value at pos 164"
}

I think it would be better if we show something like "Error occurred when parsing event(s). Expected false value at pos 164"

@antban
Copy link
Member Author

@antban antban commented Jun 6, 2018

@v-stepanov Fixed all the comments 3e03aa5

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 6, 2018

deploy validation please

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 6, 2018

👍

1 similar comment
@antban
Copy link
Member Author

@antban antban commented Jun 6, 2018

👍

if (strictParsing) {
try {
final JSONObject shadowEvent = StrictJsonParser.parseObject(rawEvent);
if (!shadowEvent.toString().equals(this.event.toString())) {

This comment has been minimized.

@antban

antban Jun 6, 2018
Author Member

If the order of the fields in object will be the same here?

This comment has been minimized.

@v-stepanov

v-stepanov Jun 6, 2018
Contributor

I used the same approach as you used in unit test. I thought that it is safe :)

This comment has been minimized.

@v-stepanov

v-stepanov Jun 6, 2018
Contributor

I mean JSONObject doesn't really have equals method. So I can't compare them in a simple way.

This comment has been minimized.

@antban

antban Jun 6, 2018
Author Member

Then I agree with Ricardo that we don't need shadow parsing

This comment has been minimized.

@v-stepanov

v-stepanov Jun 6, 2018
Contributor

why?

try {
final JSONObject shadowEvent = StrictJsonParser.parseObject(rawEvent);
if (!shadowEvent.toString().equals(this.event.toString())) {
LOG.debug("[STRICT_JSON_DIFF] Strict parser produced different output for event: {}", rawEvent);

This comment has been minimized.

@adyach

adyach Jun 6, 2018
Member

could you please log both parsed events to compare them later?

This comment has been minimized.

@v-stepanov

v-stepanov Jun 6, 2018
Contributor

ok, will do

This comment has been minimized.

if (strictParsing) {
try {
final JSONObject shadowEvent = StrictJsonParser.parseObject(rawEvent);
if (!shadowEvent.toString().equals(this.event.toString())) {

This comment has been minimized.

@rcillo

rcillo Jun 6, 2018
Member

@v-stepanov This toString operation is very expensive and is the core of the optimization that @antban did that modified the behaviour couple of months ago. I think we cannot go this far. The best we can do is to verify if it throws exception or not.

This comment has been minimized.

@v-stepanov

v-stepanov Jun 6, 2018
Contributor

Then we will not be able to validate the correctness. We can check the performance on staging and cancel if it consumes too much resources.

This comment has been minimized.

@rcillo

rcillo Jun 6, 2018
Member

ok, I get it. Then we need to be mindful and run it for a limited amount of time. 1 day I would say.

@rcillo
Copy link
Member

@rcillo rcillo commented Jun 6, 2018

👍

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 6, 2018

👍

v-stepanov and others added 2 commits Jun 6, 2018
@antban antban force-pushed the ARUHA-1680 branch from 999412b to 3c625e5 Jun 7, 2018
@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 7, 2018

👍

2 similar comments
@antban
Copy link
Member Author

@antban antban commented Jun 7, 2018

👍

@antban
Copy link
Member Author

@antban antban commented Jun 7, 2018

👍

@antban antban closed this Jun 7, 2018
@antban antban reopened this Jun 7, 2018
@antban
Copy link
Member Author

@antban antban commented Jun 7, 2018

👍

2 similar comments
@lmontrieux
Copy link
Member

@lmontrieux lmontrieux commented Jun 7, 2018

👍

@rcillo
Copy link
Member

@rcillo rcillo commented Jun 7, 2018

👍

@antban antban merged commit c8fb6c2 into master Jun 7, 2018
6 of 7 checks passed
6 of 7 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
codecov/patch 80% of diff hit (target 53.62%)
Details
codecov/project 54.05% (+0.43%) compared to 9d116a2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
zappr Approvals: @antban, @lmontrieux, @rcillo.
zappr/pr/specification PR has passed specification checks
@antban antban deleted the ARUHA-1680 branch Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.