yegor256 / takes

True Object-Oriented Java Web Framework without NULLs, Static Methods, Annotations, and Mutable Objects
https://www.takes.org
MIT License
805 stars 197 forks source link

Only cap the input if content length header is given #1302

Closed laeubi closed 1 month ago

laeubi commented 2 months ago

Currently RqLengthAware uses the InputStream#available to cap the input if no Content-Length header is given, but the return value does not say anything about the real content length, only how many bytes can be read without blocking (what might be 0).

This now only returns a CapInputStream when a Content-Length is given in the request.

Fixes https://github.com/yegor256/takes/issues/1301

laeubi commented 2 months ago

@yegor256 anything else needed here?

yegor256 commented 1 month ago

@laeubi would be great if you can reproduce the problem with a unit test too. Possible?

laeubi commented 1 month ago

I now added a testcase that fails without my patch but succeeds when the patch is applied.

yegor256 commented 1 month ago

@laeubi now looks perfect, but I'm afraid there is a formatting mistake in the unit test (see what Qulice is saying).

laeubi commented 1 month ago

Is there a way to apply the desired formatting automatically?

yegor256 commented 1 month ago

@laeubi we don't have that, here is why: https://www.yegor256.com/2018/01/16/educational-aspect-of-static-analysis.html

laeubi commented 1 month ago

@laeubi we don't have that, here is why: https://www.yegor256.com/2018/01/16/educational-aspect-of-static-analysis.html

While theoretically this might be good I contribute to literally hundreds of projects each using slightly different "better styles" so learning all of them is at least not really affordable for contributors especially now I have to count whitespace, rerun build again and even decode non trivial instructions like

Indentation (3) must be same or less than previous line (2), or bigger by exactly 4 (CascadeIndentationCheck)

that's really a mess and don't bring any value (neither to me nor to the project) as it is simply stupid work that can be automated.

yegor256 commented 1 month ago

@laeubi I agree. However, we don't have a tool for that. We tried to create it in the past, but failed :( Sorry.

laeubi commented 1 month ago

I have now addressed the style errors, I know at least Apache uses https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md

yegor256 commented 1 month ago

@laeubi it seems that something is wrong with the unit test introduced, since all workflow runs are hanging. Can you please check?

laeubi commented 1 month ago

At laest in the UI they all run fine and fast...

grafik

Also in github I see they pass:

[INFO] Running org.takes.rq.RqLengthAwareTest [INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.003 s -- in org.takes.rq.RqLengthAwareTest

what seems hanging is the proxy test

[INFO] Running org.takes.tk.TkProxyTest 07:27:54.309 [main] INFO com.jcabi.http.request.BaseRequest -- #fetch(POST localhost:40177 /а): [200 OK] in 1ms

it also hangs locally for me I suspect this is due to this bug:

before my change there was always a CapInputStream returned that terminated (but maybe with wrong bytes) always, now it returns the raw input stream that possibly never returns...

laeubi commented 1 month ago

Yes I can confirm it hangs in socket read: grafik

yegor256 commented 1 month ago

@laeubi thanks!