(#42) HtHead can be broken. #69

ilyakharlamov · 2019-01-15T00:46:25Z

( #42 ) HtHead can be broken

Adds unit test for buffer tearing
Replaces the double loop with a scanner

0crat · 2019-01-15T00:47:02Z

Job #69 is now in scope, role is REV

0crat · 2019-01-15T00:47:49Z

This pull request #69 is assigned to @victornoel /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

codecov-io · 2019-01-15T00:53:52Z

Codecov Report

Merging #69 into master will decrease coverage by 0.39%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master      #69     +/-   ##
===========================================
- Coverage      96.9%   96.51%   -0.4%     
+ Complexity       67       61      -6     
===========================================
  Files            17       17             
  Lines           194      172     -22     
  Branches         15        9      -6     
===========================================
- Hits            188      166     -22     
  Misses            4        4             
  Partials          2        2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/http/HtHead.java	`100% <100%> (ø)`	`3 <2> (-6)`	⬇️

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 9f10348...89c7a77. Read the comment docs.

victornoel

@ilyakharlamov see my comments :) it's a nice solution!

victornoel · 2019-01-17T08:17:41Z

src/main/java/org/cactoos/http/HtHead.java

     */
-    private static final int LENGTH = 16384;
+    private static final String DELIMITER = "\r\n";


@ilyakharlamov it would be more elegant if the constructor took this information instead of making it fixed like this. There should be a secondary constructor that set it to \r\n then.

This is more of an architectural choice so @llorllale should confirm first I think :)

@ilyakharlamov @llorllale let's forget about this, the delimiter is strongly related to the behaviour of this class, no need to introduce an extra parameter

src/main/java/org/cactoos/http/HtHead.java

victornoel · 2019-01-17T08:21:32Z

src/test/java/org/cactoos/http/HtHeadTest.java

+            Matchers.allOf(
+                Matchers.startsWith("HTTP"),
+                Matchers.containsString("OK\r\nReferer"),
+                Matchers.endsWith("text/plain")


@ilyakharlamov please use the OO version of the matchers (instantiate them with new XXX intead of the static method), and if possible use those available from cactoos-matchers

What would be the replacement for Matchers.allOf ?
I guess that supposed to be
new AllOf<>( new IterableOf<>( new StringStartsWith("HTTP"), new StringContains("OK\r\nReferer"), new StringEndsWith("text/plain") ) )
However that throws a compilation warning "unchecked generic array creation for varargs".
Please advise what to replace static Matchers.allOf with.

@ilyakharlamov you are right, it is too much with new AllOf, let' keep the static only for allOf then and see if @llorllale is good with it.

@ilyakharlamov also use cactoos-matchers StartsWith, TextHasString and EndsWith

@victornoel As far as I understand StartsWith, TextHasString and EndsWith are coming in cactoos-matchers 0.13 which has not been released yet, so it's not available in maven, so can't use it yet.

@ilyakharlamov it's happening: llorllale/cactoos-matchers#18 (comment) :)

victornoel · 2019-01-17T08:22:10Z

src/test/java/org/cactoos/http/HtHeadTest.java

+        );
+        MatcherAssert.assertThat(
+            block.length(),
+            new IsEqual<>(


@ilyakharlamov please use cactoos-matchers' TextIs

Here I am constructing a block and making sure it has the exact length of the block.
How TextIs can help to verify that the string is of a certain size?

@ilyakharlamov you are right yes. I think this test is too complex to be easily understandable.

Why are you checking the block length? You are not testing HtHead here, so I don't think you should be asserting anything. I would advise to remove this assertion.

@victornoel In order to reproduce the issue, I need to have a header of the exact size of the block (16384 bytes)
So it's way too easy to miss one character, like one extra space and the block will be 16383 or 16385 and the test will yield a false-positive result. So here I am checking the length to make sure that the constructed header is exactly 16384 bytes before running the actual test.

@ilyakharlamov ok, it's good for me then, thank you for the explanation

victornoel · 2019-01-17T08:23:29Z

src/main/java/org/cactoos/http/HtHead.java

+                this.scanner.next()
+            );
+            if (this.hasMoreElements()) {
+                builder.append(HtHead.DELIMITER);


@ilyakharlamov can you double check that we want to include the delimiter in the resulting InputStream? In the original code I see this line if (found) { tail = tail - end.length + 1; } which let me think it should not be included

I've checked against the previous commit 487af7 and
Matchers.containsString("OK\r\nReferer") passes .
Which makes sense. Since the goal is to separate the HTTP header from the body so the separator between the headers needs to be kept.

@ilyakharlamov ok yes, i better understand, thank you for confirming :)

src/test/java/org/cactoos/http/HtHeadTest.java

victornoel · 2019-01-18T16:37:58Z

src/test/java/org/cactoos/http/HtHeadTest.java

-                            "",
-                            "text here"
+                            new TextOf(""),
+                            new TextOf("body here")


@ilyakharlamov in this case I wouldn't have bothered to use TextOf, it is a bit too much I think :)

@victornoel done in c7a5980

victornoel · 2019-01-18T16:39:18Z

src/test/java/org/cactoos/http/HtHeadTest.java

+                    new StringStartsWith("HTTP"),
+                    new StringContains("OK\r\nReferer"),
+                    new StringEndsWith("text/plain")
+                )


@ilyakharlamov when cactoos-matches is released, please use directly allOf with Text based matchers (instead of using TextIs which makes all of this akward :)

victornoel · 2019-01-20T17:09:09Z

src/test/java/org/cactoos/http/HtHeadTest.java

@@ -102,8 +110,54 @@ public void largeText() throws IOException {
                    )
                )
            ).asString(),
-            Matchers.endsWith("text/plain")
+            new StringEndsWith("text/plain")


@ilyakharlamov please replace this with cactoos-matchers EndsWith (and don't call asString() on the Text)

@victornoel Not sure I understand. I have a Text, and a EndsWith. EndsWith is basically a Matcher<String>. How do I assert them between each other without using asString()?
new Assertion<>("description", Text, Matcher<String>) is obviously not compilable.

@ilyakharlamov https://github.com/llorllale/cactoos-matchers/blob/master/src/main/java/org/llorllale/cactoos/matchers/EndsWith.java is a Matcher<Text> :)

@victornoel thank you, I was very confused, there are so many matchers in the classpath with similar names that do similar things but incompatible. fixed in 8915ec4

src/test/java/org/cactoos/http/HtHeadTest.java

victornoel · 2019-01-20T17:11:41Z

@ilyakharlamov I've added a few comments, we are almost there :)

victornoel · 2019-01-21T15:17:32Z

@ilyakharlamov thank you for your patience, I think it's all good now :)
@llorllale this is good to merge!

llorllale · 2019-01-22T01:18:17Z

@ilyakharlamov I cringed when I saw Enumeration. Is there any way we can get creative and replace enumeration with some composition of org.cactoos.io.Joined and others? It could potentially allow us not to define a class just for this.

ilyakharlamov · 2019-01-22T02:52:31Z

@llorllale That can be possible after cactoos#1035.

victornoel · 2019-01-28T12:35:24Z

@ilyakharlamov apparenty yegor256/cactoos#1038 that solves yegor256/cactoos#1035 has been merged a few days ago, I suppose you can continue the work here?

ilyakharlamov · 2019-01-28T14:56:58Z

@victornoel Even though the yegor256/cactoos#1038 is merged, it is not released (will be cactoos 0.39). I am not responsible for cactoos-releases, it may be a few month before we have 0.39.

victornoel · 2019-01-28T15:12:59Z

@ilyakharlamov you should ask ARC in the issue you are interested in to make a new release

victornoel · 2019-02-02T10:12:08Z

@ilyakharlamov cactoos 0.39 has been released (see yegor256/cactoos#1035)

ilyakharlamov · 2019-02-02T13:54:51Z

@0crat wait #72

victornoel · 2019-02-02T13:55:57Z

@ilyakharlamov it has already been released... and also it is useless to tell 0crat to wait in a PR

victornoel · 2019-02-02T13:56:18Z

@ilyakharlamov just update the dependency and fix the PR

0crat · 2019-02-02T14:00:57Z

@0crat wait #72 (here)

@ilyakharlamov It's a code review job #69, can't put it on hold

ilyakharlamov · 2019-02-02T14:18:31Z

@victornoel Cactoos 0.39 introduces a few signature changes.
For some reason it decided to use checked exceptions everywhere (I thought checked exceptions are dead), so it's not a minor change, it will affect dozens of files just to upgrade, so will wait for #72.

victornoel · 2019-02-02T14:23:21Z

@ilyakharlamov yes, you are right, thanks for the explanation

victornoel · 2019-02-20T08:01:24Z

@ilyakharlamov cactoos-http now uses cactoos 0.39, I believe we can continue here now :)

victornoel · 2019-02-22T17:36:54Z

@ilyakharlamov thanks for your patience, it looks real nice now :)
@llorllale good to merge

llorllale · 2019-02-23T22:58:31Z

@victornoel @ilyakharlamov I just want to mention that @dgroup's hamcrest PR to add AllOf vararg ctor got merged a while back, so we should remove Matchers.allOf after their next release.

llorllale · 2019-02-23T22:58:36Z

@rultor merge

rultor · 2019-02-23T22:59:37Z

@rultor merge

@llorllale OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2019-02-23T23:11:52Z

@rultor merge

@llorllale Done! FYI, the full log is here (took me 12min)

0crat · 2019-02-23T23:30:24Z

Code review was too long (39 days), architects (@llorllale) were penalized, see §55

0crat · 2019-02-23T23:33:01Z

The job #69 is now out of scope

0crat · 2019-02-23T23:34:40Z

Order was finished: +15 point(s) just awarded to @victornoel /z

0crat · 2019-02-23T23:34:51Z

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale /z

0crat added the scope label Jan 15, 2019

victornoel suggested changes Jan 17, 2019

View reviewed changes

victornoel reviewed Jan 17, 2019

View reviewed changes

src/test/java/org/cactoos/http/HtHeadTest.java Outdated Show resolved Hide resolved

victornoel reviewed Jan 17, 2019

View reviewed changes

src/test/java/org/cactoos/http/HtHeadTest.java Outdated Show resolved Hide resolved

victornoel reviewed Jan 18, 2019

View reviewed changes

victornoel reviewed Jan 20, 2019

View reviewed changes

src/test/java/org/cactoos/http/HtHeadTest.java Outdated Show resolved Hide resolved

victornoel reviewed Jan 20, 2019

View reviewed changes

src/test/java/org/cactoos/http/HtHeadTest.java Outdated Show resolved Hide resolved

victornoel reviewed Jan 20, 2019

View reviewed changes

src/test/java/org/cactoos/http/HtHeadTest.java Outdated Show resolved Hide resolved

victornoel mentioned this pull request Jan 31, 2019

org.cactoos.io.Joined: add a Ctor of Iterable<Input> yegor256/cactoos#1035

Closed

(yegor256#42) HtHead can be broken.

89c7a77

ilyakharlamov force-pushed the block-tearing branch from 7858809 to 89c7a77 Compare February 21, 2019 04:20

rultor merged commit 89c7a77 into yegor256:master Feb 23, 2019

0crat removed the scope label Feb 23, 2019

ilyakharlamov deleted the block-tearing branch February 24, 2019 05:58

llorllale mentioned this pull request Jun 7, 2019

HtHead can be broken. #42

Closed

(#42) HtHead can be broken. #69

(#42) HtHead can be broken. #69

Conversation

ilyakharlamov commented Jan 15, 2019

0crat commented Jan 15, 2019

0crat commented Jan 15, 2019

codecov-io commented Jan 15, 2019 • edited Loading

Codecov Report

victornoel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakharlamov Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakharlamov Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyakharlamov Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

victornoel commented Jan 20, 2019

victornoel commented Jan 21, 2019

llorllale commented Jan 22, 2019 • edited Loading

ilyakharlamov commented Jan 22, 2019

victornoel commented Jan 28, 2019 • edited Loading

ilyakharlamov commented Jan 28, 2019

victornoel commented Jan 28, 2019

victornoel commented Feb 2, 2019

ilyakharlamov commented Feb 2, 2019

victornoel commented Feb 2, 2019

victornoel commented Feb 2, 2019

0crat commented Feb 2, 2019

ilyakharlamov commented Feb 2, 2019 • edited Loading

victornoel commented Feb 2, 2019

victornoel commented Feb 20, 2019

victornoel commented Feb 22, 2019

llorllale commented Feb 23, 2019

llorllale commented Feb 23, 2019

rultor commented Feb 23, 2019

rultor commented Feb 23, 2019

0crat commented Feb 23, 2019

0crat commented Feb 23, 2019

0crat commented Feb 23, 2019

0crat commented Feb 23, 2019

codecov-io commented Jan 15, 2019 •

edited

Loading

ilyakharlamov Jan 17, 2019 •

edited

Loading

ilyakharlamov Jan 20, 2019 •

edited

Loading

ilyakharlamov Jan 21, 2019 •

edited

Loading

llorllale commented Jan 22, 2019 •

edited

Loading

victornoel commented Jan 28, 2019 •

edited

Loading

ilyakharlamov commented Feb 2, 2019 •

edited

Loading