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

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

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

laeubi
Copy link

@laeubi laeubi commented Jun 21, 2024

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 #1301

@laeubi
Copy link
Author

laeubi commented Jul 8, 2024

@yegor256 anything else needed here?

@yegor256
Copy link
Owner

yegor256 commented Aug 2, 2024

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

@laeubi
Copy link
Author

laeubi commented Aug 2, 2024

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

@yegor256
Copy link
Owner

yegor256 commented Aug 2, 2024

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

@laeubi
Copy link
Author

laeubi commented Aug 2, 2024

Is there a way to apply the desired formatting automatically?

@yegor256
Copy link
Owner

yegor256 commented Aug 2, 2024

@laeubi
Copy link
Author

laeubi commented Aug 2, 2024

@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
Copy link
Owner

yegor256 commented Aug 2, 2024

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

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 and adds a testcase for the given case.
@laeubi
Copy link
Author

laeubi commented Aug 2, 2024

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
Copy link
Owner

yegor256 commented Aug 2, 2024

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

@laeubi
Copy link
Author

laeubi commented Aug 2, 2024

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
Copy link
Author

laeubi commented Aug 2, 2024

Yes I can confirm it hangs in socket read:
grafik

@yegor256 yegor256 merged commit fed1d87 into yegor256:master Aug 2, 2024
2 of 11 checks passed
@yegor256
Copy link
Owner

yegor256 commented Aug 2, 2024

@laeubi thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants