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

Fix: Ensure query parameters are added to requests with a body #1634

Conversation

KyrillosNageh
Copy link
Contributor

  • Description:

This pull request fixes a bug where query parameters were not being added to requests that also have a body. It improves the performRequest method to ensure query parameters are correctly appended to the URL, along with enhancing error handling and modularizing the code.

  • Files Modified:

src/main/java/com/shaft/api/RequestBuilder.java

  • Problem:

When making HTTP requests with both query parameters and a request body, the query parameters were ignored, resulting in incorrect request URLs.

  • Solution:

Dynamic URL Construction: Added functionality to append query parameters to the request URL, ensuring they are not lost when a body is present.
Comprehensive Error Management: Improved exception handling to manage errors during the request process effectively.
Modular Helper Methods: Created helper methods to append query parameters, check supported request types, and handle exceptions.

Linked Issue:
Fixes #1615

@KyrillosNageh KyrillosNageh marked this pull request as draft June 8, 2024 23:05
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

Attention: Patch coverage is 35.71429% with 27 lines in your changes missing coverage. Please review.

Project coverage is 54.23%. Comparing base (3c6b582) to head (6b22d7a).

Files Patch % Lines
src/main/java/com/shaft/api/RequestBuilder.java 35.71% 21 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1634      +/-   ##
============================================
- Coverage     54.45%   54.23%   -0.23%     
  Complexity     1363     1363              
============================================
  Files           112      112              
  Lines         10010    10031      +21     
  Branches        973      971       -2     
============================================
- Hits           5451     5440      -11     
- Misses         3949     3979      +30     
- Partials        610      612       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@MohabMohie MohabMohie left a comment

Choose a reason for hiding this comment

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

Excellent fix, and great overall quality enhancements for the class design.

Kindly just comment the test and let's merge.

}
""";
List<List<Object>> parameters = Arrays.asList(Arrays.asList("access_key", "your-access-key"));
@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Kindly comment this line to prevent the test from running in the pipeline.

Copy link
Contributor Author

@KyrillosNageh KyrillosNageh Jun 9, 2024

Choose a reason for hiding this comment

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

I deleted @test line

Copy link
Contributor

Choose a reason for hiding this comment

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

Something is wrong my friend. Can you check the changed files tab? I can only see the test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both exist now (Test file and changes on RequestBuilder class)

@MohabMohie MohabMohie marked this pull request as ready for review June 9, 2024 12:15
@MohabMohie MohabMohie merged commit 1178c36 into ShaftHQ:main Jun 10, 2024
35 of 41 checks passed
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.

[Bug]: Query params are not added to requests that also have a body
2 participants