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

[java_http] send request body #995

Merged
merged 10 commits into from
Jul 31, 2023

Conversation

alex-james-dev
Copy link
Contributor

This is a PR for the java_http GSoC '23 project 🌞.
Relevant tracking issue: #957.

Purpose

The purpose of this PR is to make the package:java_http client send the request body to the server.
We are now passing another two package:http_client_conformance_tests 🎉:

  1. testIsolate()
  2. testRequestBody()

The testRequestBody() tests were initially failing because we can't send a StreamedRequest object between isolates.

Reviewers

@HosseinYousefi
@natebosch


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 93 to 97
httpUrlConnection.setDoOutput(true);
final outputStream = httpUrlConnection.getOutputStream();
requestBody.forEach(outputStream.write);

outputStream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] Is there any value in minimizing the uses of callMethodWithArgs and avoiding interop calls in a loop?

(untested suggestion)

Suggested change
httpUrlConnection.setDoOutput(true);
final outputStream = httpUrlConnection.getOutputStream();
requestBody.forEach(outputStream.write);
outputStream
httpUrlConnection.setDoOutput(true);
final outputStream = httpUrlConnection.getOutputStream();
final bodyArray = JArray(jbyte.type, requestBody.length)
..setRange(0, requestBody.length, requestBody);
outputStream
..write1(bodyArray)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea to me!

I've pushed a commit with the suggestions and the tests were all passing locally.

Thank you 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been learning about extension methods recently, could we add an extension method to Uint8List?

extension on Uint8List {
  JArray<jbyte> toJArray() =>
      JArray(jbyte.type, length)..setRange(0, length, this);
}

Then, the code would go from:

    final bodyArray = JArray(jbyte.type, requestBody.length)
      ..setRange(0, requestBody.length, requestBody);

    httpUrlConnection.getOutputStream()
      ..write1(bodyArray)
      ..flush()
      ..close();

to:

    httpUrlConnection.getOutputStream()
      ..write1(requestBody.toJArray())
      ..flush()
      ..close();

My intuition is that adding an extension method to Uint8List is unnecessary at the moment. But if it becomes a common occurence to convert Uint8List to JArray<jbyte> in java_http then it would be a nice idea to add an extension method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add an extension method to Uint8List?

This extension method does look like a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Let's go for the extension method toJArray() on Uint8List.

Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as is, or with the discussed extension method toJArray().

@alex-james-dev
Copy link
Contributor Author

Hey @natebosch,

I've pushed a commit which adds the extension method toJArray() on Uint8List.

I'm new to extension methods and I'm not sure where the best place to put the extension method is.
I ended up taking inspiration from package:checks and created an extensions directory.

But I'm open to ideas about where to put the extension method.

Thank you!

@natebosch
Copy link
Member

I'm open to ideas about where to put the extension method.

For something very short like this which is used in 1 place, I usually start with a private extension locally within the library. A "utils" or "extensions" directory or library like you chose here is also great.

It's a pretty low-impact decision in this case which is why I approved without commenting. Since you're looking for input on best practices my recommendation would be a private local extension for this one.

extension on Uint8List {

For example:

https://github.com/dart-lang/async/blob/b65622afa33c5bfc574ae6b34d5a61f18a98f83c/lib/src/cancelable_operation.dart#L560

@alex-james-dev
Copy link
Contributor Author

Hey @natebosch,

Thanks for explaining when to use an unnamed extension and where to store named extensions, I'll definitely remember that for the future 🧠.

I've pushed a commit to make the change to a private extension, I think it looks better. The example you linked was very helpful.

I appreciate your help!

@HosseinYousefi HosseinYousefi merged commit e8e35db into dart-lang:master Jul 31, 2023
15 checks passed
@alex-james-dev alex-james-dev deleted the java-http-request-body branch July 31, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants