Support redirects in `SERVICE` requests #2435

hannahbast · 2025-10-15T00:20:57Z

There is now a runtime parameter service-max-redirects that limits the number of redirects that will be followed for a single SERVICE request. The default value is 1, that is, at most one redirect will be followed. To disable following redirects altogether, set the value to 0. In particular, this fixes the problem discussed in #2434

There is now a runtime parameter `service-max-redirects` (default: 1) that limits the number of redirects that will be followed for a single `SERVICE` request. To disable following redirects, set the parameter to 0.

See ad-freiburg/qlever#2435

Copilot

Pull Request Overview

This PR adds support for following HTTP redirects in SERVICE requests by introducing a configurable maximum redirect limit. The implementation handles redirect status codes (301, 302, 307, 308) and follows the Location header up to a specified limit.

Key changes:

Added service-max-redirects runtime parameter (default: 1) to control redirect behavior
Extended HTTP client response structure to include Location header
Implemented redirect following logic in Service.cpp with proper error handling

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/global/RuntimeParameters.h	Added serviceMaxRedirects_ parameter declaration
src/global/RuntimeParameters.cpp	Registered the new runtime parameter
src/util/http/HttpClient.h	Added location_ field to HttpOrHttpsResponse struct
src/util/http/HttpClient.cpp	Extract Location header from HTTP response
src/engine/Service.cpp	Implemented redirect following logic with configurable limits
test/util/HttpClientTestHelpers.h	Updated test helper to support location parameter
test/ServiceTest.cpp	Removed trailing periods from error messages

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-15T00:22:25Z

src/engine/Service.cpp

+  const size_t maxRedirects =
+      getRuntimeParameter<&RuntimeParameters::serviceMaxRedirects_>();
+  size_t redirectCount = 0;
+  while (redirectCount <= maxRedirects) {


The loop condition allows one more redirect than intended. When maxRedirects is 1, this allows 2 redirects (when redirectCount is 0 and 1). Change to redirectCount < maxRedirects to respect the exact limit.

Suggested change

while (redirectCount <= maxRedirects) {

while (redirectCount < maxRedirects) {

The first request is not a redirect. The second request is the first redirect. And so on ...

Copilot · 2025-10-15T00:22:25Z

src/engine/Service.cpp

+
+  // If the loop exited because we exceeded the maximum number of redirects,
+  // throw a corresponding error.
+  if (redirectCount > maxRedirects) {


This condition will never be true due to the loop condition above. The loop exits when redirectCount <= maxRedirects is false, meaning redirectCount equals maxRedirects + 1. Change to redirectCount == maxRedirects + 1 or adjust the loop condition.

Suggested change

if (redirectCount > maxRedirects) {

if (redirectCount == maxRedirects + 1) {

Not true, this condition is only true when there are redirects in every iteration, until the while condition is violated

codecov · 2025-10-15T01:25:54Z

Codecov Report

❌ Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.45%. Comparing base (91e7da9) to head (4e4cb79).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/util/http/HttpClient.cpp	89.74%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2435   +/-   ##
=======================================
  Coverage   91.45%   91.45%           
=======================================
  Files         462      462           
  Lines       46907    46925   +18     
  Branches     5240     5247    +7     
=======================================
+ Hits        42899    42916   +17     
  Misses       2511     2511           
- Partials     1497     1498    +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

RobinTF

I don't think Service.cpp is the right place to add this functionality. This should be moved to HttpClient.cpp where HttpClientImpl<T>::sendRequest gets another parameter that allows it to follow redirects. This way different parts of the code could benefit from this behaviour.

hannahbast · 2025-10-15T14:52:34Z

I don't think Service.cpp is the right place to add this functionality. This should be moved to HttpClient.cpp where HttpClientImpl<T>::sendRequest gets another parameter that allows it to follow redirects. This way different parts of the code could benefit from this behaviour.

Good point, I will try this

…hey belong

RobinTF · 2025-10-15T16:51:43Z

src/util/http/HttpClient.cpp

    std::string_view target, ad_utility::SharedCancellationHandle handle,
    std::string_view requestBody, std::string_view contentTypeHeader,
-    std::string_view acceptHeader) {
+    std::string_view acceptHeader, [[maybe_unused]] size_t maxRedirects) {


If this is unused this can be removed.

src/util/http/HttpClient.cpp

RobinTF · 2025-10-15T17:05:03Z

src/util/http/HttpClient.cpp

+
+    // If it is a redirect, but there is no `Location` header, we cannot
+    // proceed and throw an error.
+    if (response.location_.empty()) {


We should comment that we explicitly don't want to change the request method here, even if browser would do it for 301 and 302.

@RobinTF I don't understand this comment, please explain

Please have a look at the notes on https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/301 and https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/302 .
Changing the request type to GET for 301 and 302 is technically allowed and is the standard behaviour for browsers, but this is of course not what we want here. So we should make it explicit that we just follow location headers and don't change behaviour based on status codes (technically the permanent redirects should be stored somewhere to avoid the extra step, but that's completely overkill for what we need here).

@RobinTF By "make it explicit" you mean: Write it in the comment and the description? Or check it in the code and throw (or ignore) if it happens?

Just a comment should suffice

test/util/HttpClientTestHelpers.h

RobinTF · 2025-10-15T17:09:09Z

test/HttpTest.cpp

+    std::string finalUrl =
+        absl::StrCat("http://localhost:", okServer.getPort());
+
+    // Create `redirectServer` that points to `okServer`.


Instead of creating multiple servers that all run in their own threads. Couldn't you just create a single server, that returns a status code based on GET parameters? Like http://localhost/301?redirect=/200 conceptually? This way you can easily chain them by extending the parameters without having to write much more code.

@RobinTF Is this important? It's just a test

It's not important from a functional perspective, but it avoids the overhead of starting and stopping servers which AFAIK takes rather long for a unit test.
Also I believe it would reduce the amount of code in the test.

hannahbast

@RobinTF Thank your for your comments. I have addressed some of them, I am not sure about the others, see my comments there. Can I merge this now, once everything is green?

PS: Before my last changes, the partial test coverage of 90% in HttpTest was due to a branch that was already untested before.

sparql-conformance · 2025-10-16T06:11:23Z

Overview

Number of Tests	Passed ✅	Failed ❌	Intended ⚠️	Not tested
522	439	16	67	0

Conformance check passed ✅

No test result changes.

Details: https://qlever.dev/sparql-conformance-ui?cur=4e4cb79f8c8520e082a68562d9d20f431627e06f&prev=cba5c74fc0f3c411929d1d02063885cf77f8c0af

sonarqubecloud · 2025-10-16T07:17:55Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

RobinTF

I'm fine with your proposals, but I just realized that we should properly handle relative URLs, otherwise we end up having to deal with this issue again in the future.

RobinTF · 2025-10-16T08:25:49Z

src/util/http/HttpClient.cpp

+    }
+
+    // Follow the redirect.
+    currentUrl = Url{response.location_};


Something to consider still:
The Location header can be a relative URL. In that case it needs to be resolved relative to the previous URL, which is not the case here, so this currently might break on some redirects that don't post the whole URL.

@RobinTF Yeah, I had it in the code, than removed it again, but should include it again

Support redirects in SERVICE requests

06bae43

There is now a runtime parameter `service-max-redirects` (default: 1) that limits the number of redirects that will be followed for a single `SERVICE` request. To disable following redirects, set the parameter to 0.

hannahbast mentioned this pull request Oct 15, 2025

Add service-max-redirects to qlever settings qlever-dev/qlever-control#204

Merged

hannahbast added a commit to qlever-dev/qlever-control that referenced this pull request Oct 15, 2025

Add service.max_redirects to qlever settings (#204)

8ac724e

See ad-freiburg/qlever#2435

hannahbast requested review from RobinTF and Copilot October 15, 2025 00:21

Copilot AI reviewed Oct 15, 2025

View reviewed changes

Hannah Bast added 2 commits October 15, 2025 05:37

Fix test coverage for new redirect code

25c0554

Try to make Clang 21 happy as well

2958b70

RobinTF requested changes Oct 15, 2025

View reviewed changes

Hannah Bast added 4 commits October 15, 2025 18:20

Address Robin's request to handle redirects in HttpClientImpl

37717b4

Minimize change in Service.cpp

e85ee2d

Move redirect tests from ServiceTest.cpp to HttpTest.cpp, where t…

d5a7185

…hey belong

Fix typo

eee09a1

RobinTF requested changes Oct 15, 2025

View reviewed changes

Hannah Bast added 2 commits October 15, 2025 19:15

Revert unnecessary change of throwErrorWithContext

05bd20b

Adress remaining comments

4e4cb79

hannahbast commented Oct 16, 2025

View reviewed changes

RobinTF requested changes Oct 16, 2025

View reviewed changes

	while (redirectCount <= maxRedirects) {
	while (redirectCount < maxRedirects) {

	if (redirectCount > maxRedirects) {
	if (redirectCount == maxRedirects + 1) {

Support redirects in SERVICE requests #2435

Are you sure you want to change the base?

Support redirects in SERVICE requests #2435

Uh oh!

Conversation

hannahbast commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

hannahbast commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RobinTF Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannahbast Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannahbast left a comment

Choose a reason for hiding this comment

Uh oh!

sparql-conformance bot commented Oct 16, 2025

Overview

Conformance check passed ✅

Uh oh!

sonarqubecloud bot commented Oct 16, 2025

Quality Gate passed

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Support redirects in `SERVICE` requests #2435

Support redirects in `SERVICE` requests #2435

hannahbast commented Oct 15, 2025 •

edited

Loading

codecov bot commented Oct 15, 2025 •

edited

Loading

RobinTF Oct 15, 2025 •

edited

Loading

hannahbast Oct 16, 2025 •

edited

Loading