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

feat: add query param on http RequestBuilder #150

Closed
wants to merge 9 commits into from

Conversation

sebastian-alfers
Copy link
Contributor

Are tests needed here?

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Looks like we don't have any previous test coverage for the http client except in the JwtEndpointTest which is more of an integration test. Would be good with a unit test of the impl that looks at the created HttpRequest (rather than integration test).

@@ -36,6 +37,10 @@ public interface RequestBuilder<R> {

RequestBuilder<R> withTimeout(Duration timeout);

RequestBuilder<R> withQuery(String key, String value);

RequestBuilder<R> withQuery(Query query);
Copy link
Member

Choose a reason for hiding this comment

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

I guess you added this to be able to add multiple parameters, maybe skip this in the public API for adding the Akka HTTP query type and have a addQueryParameter instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I get what you mean. What do you suggest addQueryParameter to look like? Wouldn't it be enough if we remove this method and rename the withQuery(String, String) into addQueryParameter?

Copy link
Member

Choose a reason for hiding this comment

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

withNnn is expected to replace (and impl does) so subsequent calls drops the previous query. addNnnn is expected to add to existing so would allow adding multiple query parameters in a sequence:

httpClient.POST("/customer/" + id)
  .addQueryParameter("one", "val1")
  .addQueryParameter("two", "val2")
  .invoceAsync()

@sebastian-alfers
Copy link
Contributor Author

Would be good with a unit test of the impl that looks at the created HttpRequest

Let me add it it to this pr.

@patriknw
Copy link
Member

Shall we target java-spi branch instead? It will soon become the new main, and we will release 3.1.0.
Is there demand to have it in a 3.0.x release?

@sebastian-alfers
Copy link
Contributor Author

Shall we target java-spi branch instead? It will soon become the new main, and we will release 3.1.0.
Is there demand to have it in a 3.0.x release?

Please change as needed, I have no idea about branches, versions and releases. There is no external demand, I wanted to use the api in a small project.

@johanandren johanandren changed the base branch from main to java-spi January 15, 2025 13:01
@johanandren
Copy link
Member

I retargeted but that lead to conflict (and we have some stuff to merge from main, should be done separately, I can do that)

@johanandren
Copy link
Member

johanandren commented Jan 15, 2025

Merging main into java-spi should sort the conflict as well (#151)

@sebastian-alfers
Copy link
Contributor Author

Superseded by #157

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

Successfully merging this pull request may close these issues.

6 participants