Migrate http server to use drogon library - #351
Conversation
WalkthroughThe HTTP server migrated from SimpleWeb to Drogon, adding asynchronous calculation handlers, JSON responses, route registration, and Drogon-based startup. Build, CI, and installation configuration now includes Drogon and jsoncpp while removing Boost.Regex configuration. Parameter factory methods now accept read-only vectors. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0e581f7 to
a39f965
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp`:
- Around line 313-321: Update the /exit handler registered by registerGet to
invoke drogon::app().quit() after sending the response, restoring shutdown
behavior while preserving the existing empty JSON response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4dc73521-df6d-48b2-ba96-9bdb058a5701
📒 Files selected for processing (10)
DockerfileREADME.mdconfigure.acconnection_scan_algorithm/src/parameters/accessibility_parameters.cppconnection_scan_algorithm/src/parameters/common_parameters.cppconnection_scan_algorithm/src/parameters/route_parameters.cppconnection_scan_algorithm/src/transit_routing_http_server.cppinclude/parameters.hppinclude/server_http.hppinclude/server_https.hpp
💤 Files with no reviewable changes (2)
- include/server_https.hpp
- include/server_http.hpp
📜 Review details
⚠️ CI failures not shown inline (10)
GitHub Actions: CodeQL / 0_Analyze (cpp).txt: Migrate http server to use drogon library
Conclusion: failure
##[group]Run ./configure
�[36;1m./configure�[0m
shell: /usr/bin/bash -e {0}
env:
CODEQL_ACTION_FEATURE_MULTI_LANGUAGE: false
CODEQL_ACTION_FEATURE_SANDWICH: false
CODEQL_ACTION_FEATURE_SARIF_COMBINE: true
CODEQL_ACTION_FEATURE_WILL_UPLOAD: true
CODEQL_ACTION_VERSION: 4.37.0
JOB_RUN_UUID: 0d588eb7-ee6a-4781-a0f6-76405c68b1f9
CODEQL_ACTION_INIT_HAS_RUN: true
CODEQL_ACTION_ANALYSIS_KEY: .github/workflows/codeql-analysis.yml:analyze
CODEQL_WORKFLOW_STARTED_AT:
CODEQL_ACTION_CLI_VERSION_INFO: {"cmd":"/opt/hostedtoolcache/CodeQL/2.26.0/x64/codeql/codeql","version":{"productName":"CodeQL","vendor":"GitHub","version":"2.26.0","sha":"d91323f65aac3dfb68abb241929aea94bffc5b3e","branches":["codeql-cli-2.26.0"],"copyright":"Copyright (C) 2019-2026 GitHub, Inc.","unpackedLocation":"/opt/hostedtoolcache/CodeQL/2.26.0/x64/codeql","configFileLocation":"/home/runner/.config/codeql/config","configFileFound":false,"overlayVersion":4,"features":{"analysisSummaryV2Default":true,"buildModeOption":true,"bundleSupportsIncludeDiagnostics":true,"bundleSupportsIncludeLogs":true,"bundleSupportsOverlay":true,"databaseInterpretResultsSupportsSarifRunProperty":true,"featuresInVersionResult":true,"indirectTracingSupportsStaticBinaries":false,"informsAboutUnsupportedPathFilters":true,"supportsPython312":true,"mrvaPackCreate":true,"threatModelOption":true,"traceCommandUseBuildMode":true,"v2ramSizing":true,"mrvaPackCreateMultipleQueries":true,"setsCodeqlRunnerEnvVar":true,"sarifMergeRunsFromEqualCategory":true,"forceOverwrite":true,"generateSummarySymbolMap":true,"pythonDefaultIsToNotExtractStdlib":true,"queryServerRunQueries":true,"queryServerTrimCacheWithMode":true,"builtinExtractorsSpecifyDefaultQueries":true,"bqrsDiffResultSets":true,"bundleSupportsIncludeOption":true,"suppressesMissingFileBaselineWarning":true}}}
CODEQL_EXTRACTOR_CPP_TRAP_CACHING: true
CODEQL_RAM: 14574
CODEQL_THREADS: 4
CODEQL_SCRATCH_DIR: /home/runner/work/_temp/codeql_databases/w...
GitHub Actions: CodeQL / Analyze (cpp): Migrate http server to use drogon library
Conclusion: failure
##[group]Run ./configure
�[36;1m./configure�[0m
shell: /usr/bin/bash -e {0}
env:
CODEQL_ACTION_FEATURE_MULTI_LANGUAGE: false
CODEQL_ACTION_FEATURE_SANDWICH: false
CODEQL_ACTION_FEATURE_SARIF_COMBINE: true
CODEQL_ACTION_FEATURE_WILL_UPLOAD: true
CODEQL_ACTION_VERSION: 4.37.0
JOB_RUN_UUID: 0d588eb7-ee6a-4781-a0f6-76405c68b1f9
CODEQL_ACTION_INIT_HAS_RUN: true
CODEQL_ACTION_ANALYSIS_KEY: .github/workflows/codeql-analysis.yml:analyze
CODEQL_WORKFLOW_STARTED_AT:
CODEQL_ACTION_CLI_VERSION_INFO: {"cmd":"/opt/hostedtoolcache/CodeQL/2.26.0/x64/codeql/codeql","version":{"productName":"CodeQL","vendor":"GitHub","version":"2.26.0","sha":"d91323f65aac3dfb68abb241929aea94bffc5b3e","branches":["codeql-cli-2.26.0"],"copyright":"Copyright (C) 2019-2026 GitHub, Inc.","unpackedLocation":"/opt/hostedtoolcache/CodeQL/2.26.0/x64/codeql","configFileLocation":"/home/runner/.config/codeql/config","configFileFound":false,"overlayVersion":4,"features":{"analysisSummaryV2Default":true,"buildModeOption":true,"bundleSupportsIncludeDiagnostics":true,"bundleSupportsIncludeLogs":true,"bundleSupportsOverlay":true,"databaseInterpretResultsSupportsSarifRunProperty":true,"featuresInVersionResult":true,"indirectTracingSupportsStaticBinaries":false,"informsAboutUnsupportedPathFilters":true,"supportsPython312":true,"mrvaPackCreate":true,"threatModelOption":true,"traceCommandUseBuildMode":true,"v2ramSizing":true,"mrvaPackCreateMultipleQueries":true,"setsCodeqlRunnerEnvVar":true,"sarifMergeRunsFromEqualCategory":true,"forceOverwrite":true,"generateSummarySymbolMap":true,"pythonDefaultIsToNotExtractStdlib":true,"queryServerRunQueries":true,"queryServerTrimCacheWithMode":true,"builtinExtractorsSpecifyDefaultQueries":true,"bqrsDiffResultSets":true,"bundleSupportsIncludeOption":true,"suppressesMissingFileBaselineWarning":true}}}
CODEQL_EXTRACTOR_CPP_TRAP_CACHING: true
CODEQL_RAM: 14574
CODEQL_THREADS: 4
CODEQL_SCRATCH_DIR: /home/runner/work/_temp/codeql_databases/w...
GitHub Actions: C/C++ CI / Test build on macOS-latest (memcached false): Migrate http server to use drogon library
Conclusion: failure
##[group]Run brew update
�[36;1mbrew update�[0m
�[36;1m# Temporary Workaround for conflict with multiple python in brew�[0m
�[36;1mbrew install --overwrite [email protected] [email protected] [email protected]�[0m
�[36;1mbrew install autoconf automake libtool pkg-config�[0m
�[36;1mbrew install boost�[0m
�[36;1mbrew install capnp�[0m
�[36;1mbrew install spdlog�[0m
�[36;1mbrew install nlohmann-json�[0m
shell: /bin/bash -e {0}
##[endgroup]
�[34m==>�[0m �[1mUpdating Homebrew...�[0m
To restore the stashed changes to /opt/homebrew/Library/Taps/homebrew/homebrew-core, run:
cd /opt/homebrew/Library/Taps/homebrew/homebrew-core && git stash pop
�[34m==>�[0m �[1mHomebrew's analytics have entirely moved to our InfluxDB instance in the EU.�[0m
We gather less data than before and have destroyed all Google Analytics data:
�[4mhttps://docs.brew.sh/Analytics�[24m�[0m
Please reconsider re-enabling analytics to help our volunteer maintainers with:
brew analytics on
�[34m==>�[0m �[1mHomebrew is run entirely by unpaid volunteers. Please consider donating:�[0m
�[4mhttps://github.com/Homebrew/brew#-donations�[24m
Updated 4 taps (hashicorp/tap, azure/bicep, homebrew/core and homebrew/cask).
�[34m==>�[0m �[1mNew Formulae�[0m
adrs: Architectural Decision Record tool in Rust
ai-cli: Generate images, video, audio, and text from the terminal
asar: SNES assembler for applying patches to ROM images or building ROMs
bbrew: TUI for managing Homebrew, Flatpak, and Mac App Store packages
bismark: Bisulfite read mapper and methylation caller
bomctl: Format-agnostic SBOM tooling for the stages between SBOM generation and analysis
bracken: Bayesian estimation of species abundance from Kraken output
chopper: Filter and trim long-read sequencing data by quality and length
cve-bin-tool: Scans binaries and SBOMs for known vulnerabilities and prepares reports
cyclonedx-npm: Creates CycloneDX Software Bill of Materials (SBOM) from npm projects
diskwatch: Cross-platform disk diagnosti...
GitHub Actions: C/C++ CI / Test build on macOS-latest (memcached true): Migrate http server to use drogon library
Conclusion: failure
##[group]Run brew update
�[36;1mbrew update�[0m
�[36;1m# Temporary Workaround for conflict with multiple python in brew�[0m
�[36;1mbrew install --overwrite [email protected] [email protected] [email protected]�[0m
�[36;1mbrew install autoconf automake libtool pkg-config�[0m
�[36;1mbrew install boost�[0m
�[36;1mbrew install capnp�[0m
�[36;1mbrew install spdlog�[0m
�[36;1mbrew install nlohmann-json�[0m
shell: /bin/bash -e {0}
##[endgroup]
�[34m==>�[0m �[1mUpdating Homebrew...�[0m
To restore the stashed changes to /opt/homebrew/Library/Taps/homebrew/homebrew-core, run:
cd /opt/homebrew/Library/Taps/homebrew/homebrew-core && git stash pop
�[34m==>�[0m �[1mHomebrew's analytics have entirely moved to our InfluxDB instance in the EU.�[0m
We gather less data than before and have destroyed all Google Analytics data:
�[4mhttps://docs.brew.sh/Analytics�[24m�[0m
Please reconsider re-enabling analytics to help our volunteer maintainers with:
brew analytics on
�[34m==>�[0m �[1mHomebrew is run entirely by unpaid volunteers. Please consider donating:�[0m
�[4mhttps://github.com/Homebrew/brew#-donations�[24m
Updated 4 taps (hashicorp/tap, azure/bicep, homebrew/core and homebrew/cask).
�[34m==>�[0m �[1mNew Formulae�[0m
adrs: Architectural Decision Record tool in Rust
ai-cli: Generate images, video, audio, and text from the terminal
asar: SNES assembler for applying patches to ROM images or building ROMs
bbrew: TUI for managing Homebrew, Flatpak, and Mac App Store packages
bismark: Bisulfite read mapper and methylation caller
bomctl: Format-agnostic SBOM tooling for the stages between SBOM generation and analysis
bracken: Bayesian estimation of species abundance from Kraken output
chopper: Filter and trim long-read sequencing data by quality and length
cve-bin-tool: Scans binaries and SBOMs for known vulnerabilities and prepares reports
cyclonedx-npm: Creates CycloneDX Software Bill of Materials (SBOM) from npm projects
diskwatch: Cross-platform disk diagnosti...
GitHub Actions: C/C++ CI / 1_Test build on macOS-latest (memcached false).txt: Migrate http server to use drogon library
Conclusion: failure
##[group]Run brew update
�[36;1mbrew update�[0m
�[36;1m# Temporary Workaround for conflict with multiple python in brew�[0m
�[36;1mbrew install --overwrite [email protected] [email protected] [email protected]�[0m
�[36;1mbrew install autoconf automake libtool pkg-config�[0m
�[36;1mbrew install boost�[0m
�[36;1mbrew install capnp�[0m
�[36;1mbrew install spdlog�[0m
�[36;1mbrew install nlohmann-json�[0m
shell: /bin/bash -e {0}
##[endgroup]
�[34m==>�[0m �[1mUpdating Homebrew...�[0m
To restore the stashed changes to /opt/homebrew/Library/Taps/homebrew/homebrew-core, run:
cd /opt/homebrew/Library/Taps/homebrew/homebrew-core && git stash pop
�[34m==>�[0m �[1mHomebrew's analytics have entirely moved to our InfluxDB instance in the EU.�[0m
We gather less data than before and have destroyed all Google Analytics data:
�[4mhttps://docs.brew.sh/Analytics�[24m�[0m
Please reconsider re-enabling analytics to help our volunteer maintainers with:
brew analytics on
�[34m==>�[0m �[1mHomebrew is run entirely by unpaid volunteers. Please consider donating:�[0m
�[4mhttps://github.com/Homebrew/brew#-donations�[24m
Updated 4 taps (hashicorp/tap, azure/bicep, homebrew/core and homebrew/cask).
�[34m==>�[0m �[1mNew Formulae�[0m
adrs: Architectural Decision Record tool in Rust
ai-cli: Generate images, video, audio, and text from the terminal
asar: SNES assembler for applying patches to ROM images or building ROMs
bbrew: TUI for managing Homebrew, Flatpak, and Mac App Store packages
bismark: Bisulfite read mapper and methylation caller
bomctl: Format-agnostic SBOM tooling for the stages between SBOM generation and analysis
bracken: Bayesian estimation of species abundance from Kraken output
chopper: Filter and trim long-read sequencing data by quality and length
cve-bin-tool: Scans binaries and SBOMs for known vulnerabilities and prepares reports
cyclonedx-npm: Creates CycloneDX Software Bill of Materials (SBOM) from npm projects
diskwatch: Cross-platform disk diagnosti...
GitHub Actions: C/C++ CI / Test build on ubuntu-latest (memcached false): Migrate http server to use drogon library
Conclusion: failure
##[group]Run sudo apt-get update
�[36;1msudo apt-get update�[0m
�[36;1msudo apt-get install libboost-all-dev libexpat1-dev libjsoncpp-dev libspdlog-dev libcapnp-dev capnproto nlohmann-json3-dev�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Get:1 file:/etc/apt/apt-mirrors.txt Mirrorlist [144 B]
Hit:6 https://packages.microsoft.com/repos/azure-cli noble InRelease
Get:7 https://packages.microsoft.com/ubuntu/24.04/prod noble InRelease [3600 B]
Hit:2 http://azure.archive.ubuntu.com/ubuntu noble InRelease
Get:3 http://azure.archive.ubuntu.com/ubuntu noble-updates InRelease [126 kB]
Get:4 http://azure.archive.ubuntu.com/ubuntu noble-backports InRelease [126 kB]
Get:5 http://azure.archive.ubuntu.com/ubuntu noble-security InRelease [126 kB]
Get:8 https://dl.google.com/linux/chrome-stable/deb stable InRelease [1825 B]
Get:9 https://packages.microsoft.com/ubuntu/24.04/prod noble/main armhf Packages [11.7 kB]
Get:10 https://packages.microsoft.com/ubuntu/24.04/prod noble/main arm64 Packages [193 kB]
Get:11 https://packages.microsoft.com/ubuntu/24.04/prod noble/main amd64 Packages [226 kB]
Get:12 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 Packages [1096 kB]
Get:13 http://azure.archive.ubuntu.com/ubuntu noble-updates/main Translation-en [270 kB]
Get:14 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 Components [180 kB]
Get:15 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 Packages [1661 kB]
Get:16 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe Translation-en [328 kB]
Get:17 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 Components [388 kB]
Get:18 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse amd64 Components [940 B]
Get:19 http://azure.archive.ubuntu.com/ubuntu noble-backports/main amd64 Components [5776 B]
Get:20 http://azure.archive.ubuntu.com/ubuntu noble-backports/universe amd64 Components [10.5 kB]
Get:21 http://azure.archive.ubuntu.com/ub...
GitHub Actions: C/C++ CI / Test build on ubuntu-latest (memcached true): Migrate http server to use drogon library
Conclusion: failure
##[group]Run ./configure
�[36;1m./configure�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a race-free mkdir -p... /usr/bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether the compiler supports GNU C... yes
checking whether gcc accepts -g... yes
checking for gcc option to enable C11 features... none needed
checking whether gcc understands -c and -o together... yes
checking whether make supports the include directive... yes (GNU style)
checking dependency style of gcc... gcc3
checking for g++... g++
checking whether the compiler supports GNU C++... yes
checking whether g++ accepts -g... yes
checking for g++ option to enable C++11 features... none needed
checking dependency style of g++... gcc3
checking for ar... ar
checking the archiver (ar) interface... ar
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
checking how to print strings... printf
checking for a sed that does not truncate output... /usr/bin/sed
checking for grep that handles long lines and -e... /usr/bin/grep
checking for egrep... /usr/bin/grep -E
checking for fgrep... /usr/bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 3145728
checking how to convert x86_64-pc-l...
GitHub Actions: C/C++ CI / 3_Test build on macOS-latest (memcached true).txt: Migrate http server to use drogon library
Conclusion: failure
##[group]Run brew update
�[36;1mbrew update�[0m
�[36;1m# Temporary Workaround for conflict with multiple python in brew�[0m
�[36;1mbrew install --overwrite [email protected] [email protected] [email protected]�[0m
�[36;1mbrew install autoconf automake libtool pkg-config�[0m
�[36;1mbrew install boost�[0m
�[36;1mbrew install capnp�[0m
�[36;1mbrew install spdlog�[0m
�[36;1mbrew install nlohmann-json�[0m
shell: /bin/bash -e {0}
##[endgroup]
�[34m==>�[0m �[1mUpdating Homebrew...�[0m
To restore the stashed changes to /opt/homebrew/Library/Taps/homebrew/homebrew-core, run:
cd /opt/homebrew/Library/Taps/homebrew/homebrew-core && git stash pop
�[34m==>�[0m �[1mHomebrew's analytics have entirely moved to our InfluxDB instance in the EU.�[0m
We gather less data than before and have destroyed all Google Analytics data:
�[4mhttps://docs.brew.sh/Analytics�[24m�[0m
Please reconsider re-enabling analytics to help our volunteer maintainers with:
brew analytics on
�[34m==>�[0m �[1mHomebrew is run entirely by unpaid volunteers. Please consider donating:�[0m
�[4mhttps://github.com/Homebrew/brew#-donations�[24m
Updated 4 taps (hashicorp/tap, azure/bicep, homebrew/core and homebrew/cask).
�[34m==>�[0m �[1mNew Formulae�[0m
adrs: Architectural Decision Record tool in Rust
ai-cli: Generate images, video, audio, and text from the terminal
asar: SNES assembler for applying patches to ROM images or building ROMs
bbrew: TUI for managing Homebrew, Flatpak, and Mac App Store packages
bismark: Bisulfite read mapper and methylation caller
bomctl: Format-agnostic SBOM tooling for the stages between SBOM generation and analysis
bracken: Bayesian estimation of species abundance from Kraken output
chopper: Filter and trim long-read sequencing data by quality and length
cve-bin-tool: Scans binaries and SBOMs for known vulnerabilities and prepares reports
cyclonedx-npm: Creates CycloneDX Software Bill of Materials (SBOM) from npm projects
diskwatch: Cross-platform disk diagnosti...
GitHub Actions: C/C++ CI / 0_Test build on ubuntu-latest (memcached false).txt: Migrate http server to use drogon library
Conclusion: failure
##[group]Run sudo apt-get update
�[36;1msudo apt-get update�[0m
�[36;1msudo apt-get install libboost-all-dev libexpat1-dev libjsoncpp-dev libspdlog-dev libcapnp-dev capnproto nlohmann-json3-dev�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Get:1 file:/etc/apt/apt-mirrors.txt Mirrorlist [144 B]
Hit:6 https://packages.microsoft.com/repos/azure-cli noble InRelease
Get:7 https://packages.microsoft.com/ubuntu/24.04/prod noble InRelease [3600 B]
Hit:2 http://azure.archive.ubuntu.com/ubuntu noble InRelease
Get:3 http://azure.archive.ubuntu.com/ubuntu noble-updates InRelease [126 kB]
Get:4 http://azure.archive.ubuntu.com/ubuntu noble-backports InRelease [126 kB]
Get:5 http://azure.archive.ubuntu.com/ubuntu noble-security InRelease [126 kB]
Get:8 https://dl.google.com/linux/chrome-stable/deb stable InRelease [1825 B]
Get:9 https://packages.microsoft.com/ubuntu/24.04/prod noble/main armhf Packages [11.7 kB]
Get:10 https://packages.microsoft.com/ubuntu/24.04/prod noble/main arm64 Packages [193 kB]
Get:11 https://packages.microsoft.com/ubuntu/24.04/prod noble/main amd64 Packages [226 kB]
Get:12 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 Packages [1096 kB]
Get:13 http://azure.archive.ubuntu.com/ubuntu noble-updates/main Translation-en [270 kB]
Get:14 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 Components [180 kB]
Get:15 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 Packages [1661 kB]
Get:16 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe Translation-en [328 kB]
Get:17 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 Components [388 kB]
Get:18 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse amd64 Components [940 B]
Get:19 http://azure.archive.ubuntu.com/ubuntu noble-backports/main amd64 Components [5776 B]
Get:20 http://azure.archive.ubuntu.com/ubuntu noble-backports/universe amd64 Components [10.5 kB]
Get:21 http://azure.archive.ubuntu.com/ub...
GitHub Actions: C/C++ CI / 2_Test build on ubuntu-latest (memcached true).txt: Migrate http server to use drogon library
Conclusion: failure
##[group]Run ./configure
�[36;1m./configure�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a race-free mkdir -p... /usr/bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether the compiler supports GNU C... yes
checking whether gcc accepts -g... yes
checking for gcc option to enable C11 features... none needed
checking whether gcc understands -c and -o together... yes
checking whether make supports the include directive... yes (GNU style)
checking dependency style of gcc... gcc3
checking for g++... g++
checking whether the compiler supports GNU C++... yes
checking whether g++ accepts -g... yes
checking for g++ option to enable C++11 features... none needed
checking dependency style of g++... gcc3
checking for ar... ar
checking the archiver (ar) interface... ar
checking build system type... x86_64-pc-linux-gnu
checking host system type... x86_64-pc-linux-gnu
checking how to print strings... printf
checking for a sed that does not truncate output... /usr/bin/sed
checking for grep that handles long lines and -e... /usr/bin/grep
checking for egrep... /usr/bin/grep -E
checking for fgrep... /usr/bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 3145728
checking how to convert x86_64-pc-l...
🧰 Additional context used
🪛 Clang (14.0.6)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[warning] 68-68: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 68-68: method 'createCommonParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 68-68: 2 adjacent parameters of 'createCommonParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 68-68: the first parameter in the range is 'parameters'
(clang)
[note] 68-68: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[warning] 42-42: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 42-42: method 'createAccessibilityParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 42-42: 2 adjacent parameters of 'createAccessibilityParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 42-42: the first parameter in the range is 'parameters'
(clang)
[note] 42-42: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[warning] 58-58: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 58-58: method 'createRouteODParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 58-58: 2 adjacent parameters of 'createRouteODParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 58-58: the first parameter in the range is 'parameters'
(clang)
[note] 58-58: the last parameter in the range is 'scenarios'
(clang)
include/parameters.hpp
[warning] 149-149: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 216-216: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 252-252: use a trailing return type for this function
(modernize-use-trailing-return-type)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[warning] 35-35: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 36-36: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 105-105: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 105-105: 2 adjacent parameters of 'makeJsonResponse' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 105-105: the first parameter in the range is 'body'
(clang)
[note] 106-106: the last parameter in the range is 'code'
(clang)
[note] 105-105: after resolving type aliases, 'const std::string &' and 'int' are the same
(clang)
[note] 106-106: 'const std::string &' and 'int' parameters accept and bind the same kind of values
(clang)
[warning] 117-117: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 119-119: variable 'parametersWithValues' is not initialized
(cppcoreguidelines-init-variables)
[warning] 223-223: variable 'cacheNames' is not initialized
(cppcoreguidelines-init-variables)
[warning] 224-224: variable 'cacheNamesVector' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[style] 68-68: The function 'createCommonParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[style] 42-42: The function 'createAccessibilityParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[style] 58-58: The function 'createRouteODParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[style] 35-35: The function 'getDepartureTime' is never used.
(unusedFunction)
[style] 36-36: The function 'getArrivalTime' is never used.
(unusedFunction)
[style] 36-36: The function 'accept' is never used.
(unusedFunction)
[style] 118-118: The function 'getScenario' is never used.
(unusedFunction)
[style] 119-119: The function 'getTimeOfTrip' is never used.
(unusedFunction)
[style] 120-120: The function 'getMinWaitingTimeSeconds' is never used.
(unusedFunction)
[style] 121-121: The function 'getMaxTotalTravelTimeSeconds' is never used.
(unusedFunction)
[style] 122-122: The function 'getMaxAccessWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 123-123: The function 'getMaxEgressWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 124-124: The function 'getMaxTransferWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 134-134: The function 'getMaxInnerTimeOfTripBufferSeconds' is never used.
(unusedFunction)
[style] 135-135: The function 'isForwardCalculation' is never used.
(unusedFunction)
[style] 136-136: The function 'getOnlyServices' is never used.
(unusedFunction)
[style] 137-137: The function 'getExceptServices' is never used.
(unusedFunction)
[style] 138-138: The function 'getOnlyLines' is never used.
(unusedFunction)
[style] 139-139: The function 'getExceptLines' is never used.
(unusedFunction)
[style] 140-140: The function 'getOnlyModes' is never used.
(unusedFunction)
[style] 141-141: The function 'getExceptModes' is never used.
(unusedFunction)
[style] 142-142: The function 'getOnlyAgencies' is never used.
(unusedFunction)
[style] 143-143: The function 'getExceptAgencies' is never used.
(unusedFunction)
[style] 144-144: The function 'getOnlyNodes' is never used.
(unusedFunction)
[style] 145-145: The function 'getExceptNodes' is never used.
(unusedFunction)
[style] 146-146: The function 'getWalkingSpeedFactor' is never used.
(unusedFunction)
[style] 147-147: The function 'getWalkingSpeedMetersPerSecond' is never used.
(unusedFunction)
[performance] 234-234: Range variable 'cacheName' should be declared as const reference.
(iterateByValue)
[style] 201-201: The function 'getMaxAlternatives' is never used.
(unusedFunction)
[style] 202-202: The function 'getAlternativesMaxTravelTimeRatio' is never used.
(unusedFunction)
[style] 203-203: The function 'getMinAlternativeMaxTravelTimeSeconds' is never used.
(unusedFunction)
[style] 204-204: The function 'getAlternativesMaxAddedTravelTimeSeconds' is never used.
(unusedFunction)
[style] 205-205: The function 'getMaxValidAlternatives' is never used.
(unusedFunction)
[style] 242-242: The function 'getPlace' is never used.
(unusedFunction)
🪛 markdownlint-cli2 (0.23.0)
README.md
[warning] 47-47: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (13)
configure.ac (2)
17-19: LGTM!
61-85: 🗄️ Data Integrity & IntegrationAC_CHECK_LIB already adds the drogon/trantor flags
AC_CHECK_LIB([trantor], [main], [], ...)and the drogon check still append-ltrantor/-ldrogontoLIBS; the empty action doesn’t suppress Autoconf’s default.> Likely an incorrect or invalid review comment.Dockerfile (1)
6-6: LGTM!README.md (1)
43-51: 📐 Maintainability & Code Quality | ⚡ Quick winAdd blank line before the relocated heading.
Moving "libmemcached is optional" next to
## Ubuntu 24.04 Installremoved the blank line before the heading, triggering markdownlint MD022.📝 Suggested fix
libmemcached is optional + ## Ubuntu 24.04 InstallSource: Linters/SAST tools
connection_scan_algorithm/src/transit_routing_http_server.cpp (5)
1-160: LGTM!
217-311: LGTM!
324-457: LGTM! Verified the JSON response contracts againstresult_to_v2.cpp/result_to_v2_summary.cpp/result_to_v2_accessibility.cpp— the exception handling and async dispatch look correct.
459-465: 🎯 Functional Correctness | ⚡ Quick winDefault (unmatched-route) handler returns HTTP 200 instead of 404.
makeJsonResponse(...)here uses its default status code (k200OK), even though this only fires for paths that don't match any registered route. A client checking status codes would see success for a nonexistent endpoint.🔧 Suggested fix
- callback(makeJsonResponse("{\"status\": \"error\", \"error\": \"missing params\"}")); + callback(makeJsonResponse("{\"status\": \"error\", \"error\": \"missing params\"}", drogon::k404NotFound));
467-476: LGTM! Fixed IO thread count + separate compute pool matches the stated goal of fixing high-thread-count performance issues.include/parameters.hpp (1)
149-151: LGTM!Also applies to: 216-218, 252-254
connection_scan_algorithm/src/parameters/common_parameters.cpp (1)
68-68: LGTM!connection_scan_algorithm/src/parameters/route_parameters.cpp (1)
58-58: LGTM!connection_scan_algorithm/src/parameters/accessibility_parameters.cpp (1)
42-42: LGTM!
| // closeServer and exit app: | ||
| server.resource["^/exit[/]?\\?([0-9a-zA-Z&=_,:/.-]+)$"]["GET"]=[&server](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> ) { | ||
|
|
||
| std::string response {""}; | ||
| *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response; | ||
|
|
||
| // todo | ||
|
|
||
| }; | ||
|
|
||
|
|
||
| registerGet("/exit", | ||
| [](const drogon::HttpRequestPtr &, HandlerCallback &&callback) { | ||
|
|
||
| callback(makeJsonResponse("")); | ||
|
|
||
| // todo (drogon::app().quit() will stop the event loops and return from run()) | ||
|
|
||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
/exit no longer stops the server — TODO left unimplemented.
The handler just returns an empty 200 response; drogon::app().quit() is never called, only mentioned in a comment. Calling /exit will silently no-op instead of shutting the server down, a behavior regression from the previous implementation.
🔧 Suggested fix
registerGet("/exit",
- [](const drogon::HttpRequestPtr &, HandlerCallback &&callback) {
+ [](const drogon::HttpRequestPtr &, HandlerCallback &&callback) {
callback(makeJsonResponse(""));
- // todo (drogon::app().quit() will stop the event loops and return from run())
+ drogon::app().quit();
});Want me to open an issue to track this, or is the shutdown-on-exit behavior intentionally deferred?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // closeServer and exit app: | |
| server.resource["^/exit[/]?\\?([0-9a-zA-Z&=_,:/.-]+)$"]["GET"]=[&server](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> ) { | |
| std::string response {""}; | |
| *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response; | |
| // todo | |
| }; | |
| registerGet("/exit", | |
| [](const drogon::HttpRequestPtr &, HandlerCallback &&callback) { | |
| callback(makeJsonResponse("")); | |
| // todo (drogon::app().quit() will stop the event loops and return from run()) | |
| }); | |
| // closeServer and exit app: | |
| registerGet("/exit", | |
| [](const drogon::HttpRequestPtr &, HandlerCallback &&callback) { | |
| callback(makeJsonResponse("")); | |
| drogon::app().quit(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp` around lines
313 - 321, Update the /exit handler registered by registerGet to invoke
drogon::app().quit() after sending the response, restoring shutdown behavior
while preserving the existing empty JSON response.
There was a problem hiding this comment.
it was already a TODO, no action here
There was a problem hiding this comment.
@greenscientist, understood — thanks for the context. Leaving this as-is since it's a pre-existing TODO, not a regression introduced by this PR.
a39f965 to
9e4c18e
Compare
|
Turns out, this was not the bottleneck. #352 is the actual fix. This would still be an interesting upgrade, but there's no urgency now. |
|
It works on macos ARM. I only get this warning: ld: warning: -bind_at_load is deprecated on macOS but it may not be new to this PR. |
Le -bind_at_load vient de libtool, donc c'est un peu externe à nous. (C'est un des fichier généré par autoreconf, donc ca devrait partir tout seul pour toi éventuellement |
9e4c18e to
011bd4d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/c-cpp.yml (2)
18-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the checkout action to a full commit SHA.
actions/checkout@v7is a mutable tag. Pin it to an immutable commit SHA, retaining the release version in a comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/c-cpp.yml around lines 18 - 20, Update the actions/checkout step to reference its full immutable commit SHA instead of the mutable v7 tag, and retain the v7 release identifier in an adjacent comment. Preserve the existing recursive submodules configuration.Source: Linters/SAST tools
18-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin all changed checkout actions by commit SHA.
.github/workflows/c-cpp.yml#L18-L20: replaceactions/checkout@v7with a full commit SHA..github/workflows/codeql-analysis.yml#L42-L42: replaceactions/checkout@v7with a full commit SHA.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/c-cpp.yml around lines 18 - 20, Pin both changed checkout action references to full commit SHAs: update actions/checkout@v7 in .github/workflows/c-cpp.yml lines 18-20 and .github/workflows/codeql-analysis.yml line 42, preserving the existing checkout configuration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/c-cpp.yml:
- Around line 26-27: Replace the duplicate Ubuntu dependency commands with one
command containing libdrogon-dev in .github/workflows/c-cpp.yml lines 26-27.
Update the documented installation command in README.md lines 51-52 to likewise
contain libdrogon-dev only once.
In `@configure.ac`:
- Around line 73-77: Keep the existing C++ header check, but replace the
AC_CHECK_LIB main-symbol probes for trantor and drogon with C++ compile/link
tests that include each library’s relevant header and call a representative API
symbol. Preserve the existing configure-time error messages when either test
fails.
- Line 94: Move JSONCPP_LIBS from the LDFLAGS assignment to the LIBS assignment
in the configure link flags setup. Preserve the existing value while ensuring
the -ljsoncpp linker option is emitted with LIBS after object files.
---
Outside diff comments:
In @.github/workflows/c-cpp.yml:
- Around line 18-20: Update the actions/checkout step to reference its full
immutable commit SHA instead of the mutable v7 tag, and retain the v7 release
identifier in an adjacent comment. Preserve the existing recursive submodules
configuration.
- Around line 18-20: Pin both changed checkout action references to full commit
SHAs: update actions/checkout@v7 in .github/workflows/c-cpp.yml lines 18-20 and
.github/workflows/codeql-analysis.yml line 42, preserving the existing checkout
configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7688900-bbba-455a-96cc-90ab8c031973
📒 Files selected for processing (12)
.github/workflows/c-cpp.yml.github/workflows/codeql-analysis.ymlDockerfileREADME.mdconfigure.acconnection_scan_algorithm/src/parameters/accessibility_parameters.cppconnection_scan_algorithm/src/parameters/common_parameters.cppconnection_scan_algorithm/src/parameters/route_parameters.cppconnection_scan_algorithm/src/transit_routing_http_server.cppinclude/parameters.hppinclude/server_http.hppinclude/server_https.hpp
💤 Files with no reviewable changes (2)
- include/server_https.hpp
- include/server_http.hpp
📜 Review details
🧰 Additional context used
🪛 Clang (14.0.6)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[warning] 58-58: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 58-58: method 'createRouteODParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 58-58: 2 adjacent parameters of 'createRouteODParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 58-58: the first parameter in the range is 'parameters'
(clang)
[note] 58-58: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[warning] 68-68: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 68-68: method 'createCommonParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 68-68: 2 adjacent parameters of 'createCommonParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 68-68: the first parameter in the range is 'parameters'
(clang)
[note] 68-68: the last parameter in the range is 'scenarios'
(clang)
include/parameters.hpp
[warning] 149-149: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 216-216: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 252-252: use a trailing return type for this function
(modernize-use-trailing-return-type)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[warning] 42-42: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 42-42: method 'createAccessibilityParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 42-42: 2 adjacent parameters of 'createAccessibilityParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 42-42: the first parameter in the range is 'parameters'
(clang)
[note] 42-42: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[warning] 35-35: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 36-36: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 105-105: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 105-105: 2 adjacent parameters of 'makeJsonResponse' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 105-105: the first parameter in the range is 'body'
(clang)
[note] 106-106: the last parameter in the range is 'code'
(clang)
[note] 105-105: after resolving type aliases, 'const std::string &' and 'int' are the same
(clang)
[note] 106-106: 'const std::string &' and 'int' parameters accept and bind the same kind of values
(clang)
[warning] 117-117: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 119-119: variable 'parametersWithValues' is not initialized
(cppcoreguidelines-init-variables)
[warning] 143-143: escaped string literal can be written as a raw string literal
(modernize-raw-string-literal)
[warning] 147-147: escaped string literal can be written as a raw string literal
(modernize-raw-string-literal)
[warning] 223-223: variable 'cacheNames' is not initialized
(cppcoreguidelines-init-variables)
[warning] 224-224: variable 'cacheNamesVector' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[style] 58-58: The function 'createRouteODParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[style] 68-68: The function 'createCommonParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[style] 42-42: The function 'createAccessibilityParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[style] 35-35: The function 'getDepartureTime' is never used.
(unusedFunction)
[style] 36-36: The function 'getArrivalTime' is never used.
(unusedFunction)
[style] 36-36: The function 'accept' is never used.
(unusedFunction)
[style] 118-118: The function 'getScenario' is never used.
(unusedFunction)
[style] 119-119: The function 'getTimeOfTrip' is never used.
(unusedFunction)
[style] 120-120: The function 'getMinWaitingTimeSeconds' is never used.
(unusedFunction)
[style] 121-121: The function 'getMaxTotalTravelTimeSeconds' is never used.
(unusedFunction)
[style] 122-122: The function 'getMaxAccessWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 123-123: The function 'getMaxEgressWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 124-124: The function 'getMaxTransferWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 134-134: The function 'getMaxInnerTimeOfTripBufferSeconds' is never used.
(unusedFunction)
[style] 135-135: The function 'isForwardCalculation' is never used.
(unusedFunction)
[style] 136-136: The function 'getOnlyServices' is never used.
(unusedFunction)
[style] 137-137: The function 'getExceptServices' is never used.
(unusedFunction)
[style] 138-138: The function 'getOnlyLines' is never used.
(unusedFunction)
[style] 139-139: The function 'getExceptLines' is never used.
(unusedFunction)
[style] 140-140: The function 'getOnlyModes' is never used.
(unusedFunction)
[style] 141-141: The function 'getExceptModes' is never used.
(unusedFunction)
[style] 142-142: The function 'getOnlyAgencies' is never used.
(unusedFunction)
[style] 143-143: The function 'getExceptAgencies' is never used.
(unusedFunction)
[style] 144-144: The function 'getOnlyNodes' is never used.
(unusedFunction)
[style] 145-145: The function 'getExceptNodes' is never used.
(unusedFunction)
[style] 146-146: The function 'getWalkingSpeedFactor' is never used.
(unusedFunction)
[style] 147-147: The function 'getWalkingSpeedMetersPerSecond' is never used.
(unusedFunction)
[performance] 234-234: Range variable 'cacheName' should be declared as const reference.
(iterateByValue)
[style] 201-201: The function 'getMaxAlternatives' is never used.
(unusedFunction)
[style] 202-202: The function 'getAlternativesMaxTravelTimeRatio' is never used.
(unusedFunction)
[style] 203-203: The function 'getMinAlternativeMaxTravelTimeSeconds' is never used.
(unusedFunction)
[style] 204-204: The function 'getAlternativesMaxAddedTravelTimeSeconds' is never used.
(unusedFunction)
[style] 205-205: The function 'getMaxValidAlternatives' is never used.
(unusedFunction)
[style] 242-242: The function 'getPlace' is never used.
(unusedFunction)
🪛 markdownlint-cli2 (0.23.0)
README.md
[warning] 48-48: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 zizmor (1.26.1)
.github/workflows/codeql-analysis.yml
[error] 42-42: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/c-cpp.yml
[warning] 18-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (10)
connection_scan_algorithm/src/transit_routing_http_server.cpp (4)
313-321:/exitstill doesn't stop the server — matches prior review thread.Same TODO flagged previously; author confirmed it's a pre-existing TODO, not a regression from this PR. No action needed here.
116-125: 🎯 Functional Correctness | 💤 Low value
extractParameterswill silently drop duplicate query keys.Drogon's
getParameters()returns astd::unordered_map<std::string,std::string>, so repeated keys in the query string collapse to a single value. The previous SimpleWeb server used aCaseInsensitiveMultimap, which preserved every occurrence. If any client ever sends a repeated key (e.g., intending multi-value filters), behavior now silently differs from before.Given none of the current parameter parsers (
only_lines,except_lines, etc.) are actually populated from request parameters yet, real-world impact is likely low today, but worth confirming intentional.
103-160: LGTM! The Drogon route registration, JSON response helper, and compute-pool offloading pattern (dispatch toConcurrentTaskQueue, invoke the response callback from the worker thread) match Drogon's documented threading model for CPU-bound handlers.Also applies to: 201-242, 324-482
1-9: 🩺 Stability & AvailabilityNo change needed —
boost::splitis already included.include/parameters.hpp (1)
149-151: LGTM! Tightening these factory parameters toconst std::vector<...>&matches the corresponding definitions and doesn't change parsing behavior.Also applies to: 216-218, 252-254
connection_scan_algorithm/src/parameters/common_parameters.cpp (1)
68-68: LGTM!connection_scan_algorithm/src/parameters/route_parameters.cpp (1)
58-58: LGTM!connection_scan_algorithm/src/parameters/accessibility_parameters.cpp (1)
42-42: LGTM!Dockerfile (1)
6-6: LGTM!.github/workflows/codeql-analysis.yml (1)
62-62: LGTM!
| AC_LANG_PUSH([C++]) | ||
| AC_CHECK_HEADER([drogon/drogon.h], [], [AC_MSG_ERROR([drogon headers not found])]) | ||
| AC_LANG_POP([C++]) | ||
| AC_CHECK_LIB([trantor], [main], [], [AC_MSG_ERROR([libtrantor not found])]) | ||
| AC_CHECK_LIB([drogon], [main], [], [AC_MSG_ERROR([libdrogon not found])]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## configure.ac relevant section\n'
if [ -f configure.ac ]; then
nl -ba configure.ac | sed -n '60,90p'
else
echo "configure.ac not found"
fi
printf '\n## search for libdrogon/trantor configure/header checks\n'
rg -n "AC_CHECK_LIB|drogon/drogon\.h|libtrantor|libdrogon|LDFLAGS|LIBS" -S .Repository: chairemobilite/trRouting
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## inspect nearby autoconf code and dependency flags\n'
sed -n '1,120p' configure.ac | nl -ba
printf '\n## deterministic probe of AC_CHECK_LIB macro expansion when present in build docs/configure.ac\n'
rg -n "AC_CHECK_LIB" autoconf m4 . 2>/dev/null || true
printf '\n## list build files\n'
git ls-files | sed -n '1,120p'Repository: chairemobilite/trRouting
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## configure.ac relevant section\n'
if [ -f configure.ac ]; then
awk '{printf "%6d: %s\n", NR, $0}' configure.ac | sed -n '60,90p'
else
echo "configure.ac not found"
fi
printf '\n## search for libdrogon/trantor configure/header checks\n'
grep -RIn "AC_CHECK_LIB\|drogon/drogon\.h\|libtrantor\|libdrogon\|LDFLAGS\|LIBS" . -- .mk 2>/dev/null || true
printf '\n## inspect nearby autoconf code and dependency flags\n'
awk '{printf "%6d: %s\n", NR, $0}' configure.ac | sed -n '1,120p'
printf '\n## deterministic probe of AC_CHECK_LIB macro expansion when present in build docs/configure.ac\n'
grep -RIn "AC_CHECK_LIB" . -- .mk 2>/dev/null || true
printf '\n## list repository files (first 120)\n'
git ls-files | awk '{printf "%s\n", $0}' | sed -n '1,120p'Repository: chairemobilite/trRouting
Length of output: 23509
Use a proper C++ link test for Drogon and Trantor.
AC_CHECK_LIB([trantor], [main], [], [AC_MSG_ERROR([libtrantor not found])])
AC_CHECK_LIB([drogon], [main], [], [AC_MSG_ERROR([libdrogon not found])])
Keep the current header check, but replace the main symbol probes with small C++ link tests that include and call each library’s API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` around lines 73 - 77, Keep the existing C++ header check, but
replace the AC_CHECK_LIB main-symbol probes for trantor and drogon with C++
compile/link tests that include each library’s relevant header and call a
representative API symbol. Preserve the existing configure-time error messages
when either test fails.
There was a problem hiding this comment.
@greenscientist, replace the two AC_CHECK_LIB probes with explicit C++ link tests:
AC_LANG_PUSH([C++])
AC_CHECK_HEADER([drogon/drogon.h], [], [AC_MSG_ERROR([drogon headers not found])])
+
+ AC_MSG_CHECKING([whether libtrantor is linkable])
+ saved_LIBS="$LIBS"
+ LIBS="-ltrantor $LIBS"
+ AC_LINK_IFELSE(
+ [AC_LANG_PROGRAM(
+ [[`#include` <trantor/net/EventLoop.h>]],
+ [[trantor::EventLoop loop;]])],
+ [AC_MSG_RESULT([yes])],
+ [AC_MSG_RESULT([no])
+ AC_MSG_ERROR([libtrantor not found])])
+ LIBS="$saved_LIBS"
+
+ AC_MSG_CHECKING([whether libdrogon is linkable])
+ LIBS="-ldrogon -ltrantor $LIBS"
+ AC_LINK_IFELSE(
+ [AC_LANG_PROGRAM(
+ [[`#include` <drogon/drogon.h>]],
+ [[drogon::app().setThreadNum(1);]])],
+ [AC_MSG_RESULT([yes])],
+ [AC_MSG_RESULT([no])
+ AC_MSG_ERROR([libdrogon not found])])
+ LIBS="$saved_LIBS"
AC_LANG_POP([C++])
-AC_CHECK_LIB([trantor], [main], [], [AC_MSG_ERROR([libtrantor not found])])
-AC_CHECK_LIB([drogon], [main], [], [AC_MSG_ERROR([libdrogon not found])])This tests actual APIs while preserving the current configure-time error messages.
| LDFLAGS+=" $JEMALLOC_LIBS" | ||
| LDFLAGS+=" $CAPNP_LIBS" | ||
| LDFLAGS+=" $SPDLOG_LIBS" | ||
| LDFLAGS+=" $JSONCPP_LIBS" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files =="
git ls-files | sed -n '1,120p'
echo
echo "== configure.ac around relevant lines =="
wc -l configure.ac
sed -n '1,140p' configure.ac
echo
echo "== jsoncpp references =="
rg -n "jsoncpp|JSONCPP|PKG_CHECK|LDFLAGS|LIBS|AC_CHECK" configure.ac acinclude.m4 m4 *.m4 2>/dev/null || trueRepository: chairemobilite/trRouting
Length of output: 19095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Autoconf documentation excerpt for LDFLAGS vs LIBS =="
for var in LDFLAGS LIBS; do
echo "--- $var ---"
info autoconf "$var@var:" 2>/dev/null | sed -n '1,80p' || true
done
echo
echo "== Linker --as-needed behavior probe =="
if command -v ld >/dev/null 2>&1 && command -v g++ >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
cat <<'EOF' > "$tmpdir/main.cpp"
`#ifdef` USE_JSONCPP
`#include` <json/json.h>
int main() {
Json::Value x;
x["ok"] = true;
return 0;
}
`#else`
int main() { return 0; }
`#endif`
EOF
echo "static library stub created:"
g++ -c "$tmpdir/main.cpp" -DUSE_JSONCPP -o "$tmpdir/object.o" || true
ar rcs "$tmpdir/libjsoncpp_stub.a" "$tmpdir/object.o"
echo "without --as-needed:"
g++ "$tmpdir/object.o" -L"$tmpdir" -ljsoncpp_stub -o "$tmpdir/without-ash" || true
echo "with --as-needed (object first):"
g++ "$tmpdir/object.o" -L"$tmpdir" --as-needed -ljsoncpp_stub --no-as-needed -o "$tmpdir/with-object-first" 2>&1 || true
echo "with --as-needed (library first in LDFLAGS-style position):"
g++ -L"$tmpdir" --as-needed -ljsoncpp_stub --no-as-needed "$tmpdir/object.o" -o "$tmpdir/with-lib-first" 2>&1 || true
rm -rf "$tmpdir"
else
echo "ld or g++ not available for linker probe"
fi
echo
echo "== jsoncpp pkg-config output if installed =="
pkg-config --libs jsoncpp 2>/dev/null || trueRepository: chairemobilite/trRouting
Length of output: 376
🌐 Web query:
GNU Autoconf LDFLAGS LIBS LDFLAGS appears before libraries target link commands
💡 Result:
In GNU build systems, LDFLAGS and LIBS (or LDLIBS in Make terminology) are distinct variables by convention because they serve different roles in the linker command line, and their relative order is critical [1][2][3]. The linker is a single-pass tool that resolves symbols based on the order in which files and libraries are presented [4]. Because of this, it is standard practice to place object files first, followed by required libraries [2][4]. The division between these variables reflects this requirement [2][5]: 1. LDFLAGS: Intended for linker flags that modify the linker's behavior or search paths, such as -L (library search path) or -Wl,--gc-sections [6][1][3]. These are typically placed early in the command line, before the object files [7][5]. 2. LIBS (or LDLIBS): Intended specifically for the libraries themselves (e.g., -lm, -lcurl) [6][2][3]. By convention, these are placed at the end of the command line, after the object files, ensuring that the linker has already identified the necessary symbols from the object files before it scans the libraries to resolve them [2][4]. Autoconf and Automake enforce or encourage this separation to ensure that configuration tests (which use LDFLAGS and LIBS) and final build rules behave predictably [7][6][8]. If you place library names (-l) in LDFLAGS, they may appear before the object files on the command line; since the linker has not yet encountered any references to those libraries, it may ignore them, leading to "undefined reference" errors [7][9][4]. To fix such issues: - Move -L search paths to LDFLAGS [2][3]. - Move -l library names to LIBS (or LDLIBS in Makefiles) [2][3]. - Ensure your build rules place
Citations:
- 1: https://runebook.dev/en/docs/gnu_make/implicit-variables/LDFLAGS
- 2: https://runebook.dev/en/docs/gnu_make/implicit-variables/LDLIBS
- 3: https://lists.libreplanet.org/archive/html/autoconf/2021-01/msg00040.html
- 4: https://stackoverflow.com/questions/9417169/why-does-the-library-linker-flag-sometimes-have-to-go-at-the-end-using-gcc
- 5: https://lists.sr.ht/~skeeto/public-inbox/%[email protected]%3E
- 6: http://odysseus.fi.muni.cz/pub/gnu/old-gnu/Manuals/autoconf-2.57/html_node/autoconf_23.html
- 7: https://stackoverflow.com/questions/13610572/correcting-the-gcc-command-line-ordering-using-automake
- 8: https://stackoverflow.com/questions/24081511/unable-to-pass-ldflags-to-autoconf-test-ac-link-ifelse
- 9: https://stackoverflow.com/questions/66359606/gnu-make-gcc-order-of-options
- 10: https://sourceware.org/pipermail/cygwin/2010-April/186340.html
Put JSONCPP_LIBS in LIBS, not LDFLAGS.
JSONCPP_LIBS contains -ljsoncpp, so appending it to LDFLAGS puts it early in the link command; with --as-needed, -ljsoncpp can come before the objects that need it and get dropped. Put it in LIBS so the target can link it after the object files.
Suggested adjustment
-LDFLAGS+=" $JSONCPP_LIBS"
+LIBS+=" $JSONCPP_LIBS"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LDFLAGS+=" $JSONCPP_LIBS" | |
| LIBS+=" $JSONCPP_LIBS" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` at line 94, Move JSONCPP_LIBS from the LDFLAGS assignment to
the LIBS assignment in the configure link flags setup. Preserve the existing
value while ensuring the -ljsoncpp linker option is emitted with LIBS after
object files.
There was a problem hiding this comment.
Noted. Please can you create an issue to follow up on this. Will not do it right now
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="chairemobilite/trRouting"
body=$(cat <<'EOF'
## Summary
Move `JSONCPP_LIBS` out of `LDFLAGS` and into `LIBS` in `configure.ac`.
## Rationale
`JSONCPP_LIBS` contains the jsoncpp library linkage flags (such as `-ljsoncpp`). Keeping library flags in `LIBS` ensures they are emitted after object files in generated link commands, avoiding potential link failures when linkers use `--as-needed`.
## Affected area
- `configure.ac` — JSONCPP linker flag setup
## Acceptance criteria
- `JSONCPP_LIBS` is appended to `LIBS`, not `LDFLAGS`.
- Configure and the project link successfully with jsoncpp enabled.
## Backlinks
- Deferred from PR `#351`: https://github.com/chairemobilite/trRouting/pull/351
- Review discussion: https://github.com/chairemobilite/trRouting/pull/351#discussion_r3634462717
Requested by `@greenscientist`.
EOF
)
gh issue create \
--repo "$repo" \
--title "Move JSONCPP_LIBS from LDFLAGS to LIBS" \
--body "$body" \
--assignee "greenscientist"Length of output: 1198
@greenscientist, created and assigned: #360.
c3b0236 to
5c932ef
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
connection_scan_algorithm/src/transit_routing_http_server.cpp (1)
217-311: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftOffload
/updateCachework to the compute pool and serialize cache writes.
/updateCachecallstransitData.updateAgencies/Nodes/...directly in the Drogon IO handler, while/v2/route,/v2/summary, and/v2/accessibilityreadTransitDataoncomputePool. Send the cache update throughcomputePooland add a shared-lock or single-thread queue so update paths cannot run at the same time as calculations. Also confirmTransitData::updateX()is documented as thread-safe with concurrent reads; if not, this needs explicit synchronization too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@connection_scan_algorithm/src/transit_routing_http_server.cpp` around lines 217 - 311, Change the /updateCache handler to dispatch TransitData::updateAgencies, updateServices, updateNodes, updateLines, updatePaths, updateScenarios, and updateSchedules onto computePool instead of executing them in the Drogon IO callback. Add shared synchronization or a serialized update queue covering all updateX calls so cache writes cannot overlap with route, summary, or accessibility calculations, and verify or enforce TransitData thread-safety for concurrent reads during updates. Build and send the response only after the queued update completes, preserving the existing validation and response behavior.README.md (1)
8-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Markdown reference definitions.
Markdownlint reports references
1and2as undefined. The[1]definition also has extra text after its quoted title, which makes the definition invalid. Use named labels and keep the publisher link outside the definition.Also applies to: 19-20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 8 - 9, Update the README references for the Connection Scan Algorithm and Trip-Based Algorithm to use named labels with matching definitions, remove the extra text from the quoted title in the first definition, and place the publisher link outside that reference definition.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configure.ac`:
- Line 95: Move JSONCPP_LIBS from LDFLAGS into LIBS in the configure setup,
preserving the existing value while appending the JSONCPP linker options to the
correct variable.
- Around line 76-78: Replace the AC_CHECK_LIB checks for trantor and drogon with
C++ AC_LINK_IFELSE probes that compile and link representative API calls from
each library, using the existing configure error messages when linking fails.
Ensure the probes reference actual Drogon and Trantor symbols rather than the
test program’s main function.
In `@README.md`:
- Around line 51-52: Remove the first duplicate Ubuntu dependency-install
command and retain only the second command, which includes libdrogon-dev.
- Around line 47-50: Add a blank line between the “libmemcached is optional”
text and the “Ubuntu 24.04 Install” heading, and change that section’s fenced
code block opener to specify the bash language.
---
Outside diff comments:
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp`:
- Around line 217-311: Change the /updateCache handler to dispatch
TransitData::updateAgencies, updateServices, updateNodes, updateLines,
updatePaths, updateScenarios, and updateSchedules onto computePool instead of
executing them in the Drogon IO callback. Add shared synchronization or a
serialized update queue covering all updateX calls so cache writes cannot
overlap with route, summary, or accessibility calculations, and verify or
enforce TransitData thread-safety for concurrent reads during updates. Build and
send the response only after the queued update completes, preserving the
existing validation and response behavior.
In `@README.md`:
- Around line 8-9: Update the README references for the Connection Scan
Algorithm and Trip-Based Algorithm to use named labels with matching
definitions, remove the extra text from the quoted title in the first
definition, and place the publisher link outside that reference definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e31a44ec-8184-4382-ba92-14e287dd90f4
📒 Files selected for processing (12)
.github/workflows/c-cpp.yml.github/workflows/codeql-analysis.ymlDockerfileREADME.mdconfigure.acconnection_scan_algorithm/src/parameters/accessibility_parameters.cppconnection_scan_algorithm/src/parameters/common_parameters.cppconnection_scan_algorithm/src/parameters/route_parameters.cppconnection_scan_algorithm/src/transit_routing_http_server.cppinclude/parameters.hppinclude/server_http.hppinclude/server_https.hpp
💤 Files with no reviewable changes (2)
- include/server_http.hpp
- include/server_https.hpp
📜 Review details
🧰 Additional context used
🪛 Clang (14.0.6)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[warning] 58-58: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 58-58: method 'createRouteODParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 58-58: 2 adjacent parameters of 'createRouteODParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 58-58: the first parameter in the range is 'parameters'
(clang)
[note] 58-58: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[warning] 68-68: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 68-68: method 'createCommonParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 68-68: 2 adjacent parameters of 'createCommonParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 68-68: the first parameter in the range is 'parameters'
(clang)
[note] 68-68: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[warning] 42-42: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 42-42: method 'createAccessibilityParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 42-42: 2 adjacent parameters of 'createAccessibilityParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 42-42: the first parameter in the range is 'parameters'
(clang)
[note] 42-42: the last parameter in the range is 'scenarios'
(clang)
include/parameters.hpp
[warning] 149-149: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 216-216: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 252-252: use a trailing return type for this function
(modernize-use-trailing-return-type)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[warning] 35-35: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 36-36: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 105-105: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 105-105: 2 adjacent parameters of 'makeJsonResponse' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 105-105: the first parameter in the range is 'body'
(clang)
[note] 106-106: the last parameter in the range is 'code'
(clang)
[note] 105-105: after resolving type aliases, 'const std::string &' and 'int' are the same
(clang)
[note] 106-106: 'const std::string &' and 'int' parameters accept and bind the same kind of values
(clang)
[warning] 117-117: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 119-119: variable 'parametersWithValues' is not initialized
(cppcoreguidelines-init-variables)
[warning] 223-223: variable 'cacheNames' is not initialized
(cppcoreguidelines-init-variables)
[warning] 224-224: variable 'cacheNamesVector' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[style] 58-58: The function 'createRouteODParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[style] 68-68: The function 'createCommonParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[style] 42-42: The function 'createAccessibilityParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[style] 35-35: The function 'getDepartureTime' is never used.
(unusedFunction)
[style] 36-36: The function 'getArrivalTime' is never used.
(unusedFunction)
[style] 36-36: The function 'accept' is never used.
(unusedFunction)
[style] 118-118: The function 'getScenario' is never used.
(unusedFunction)
[style] 119-119: The function 'getTimeOfTrip' is never used.
(unusedFunction)
[style] 120-120: The function 'getMinWaitingTimeSeconds' is never used.
(unusedFunction)
[style] 121-121: The function 'getMaxTotalTravelTimeSeconds' is never used.
(unusedFunction)
[style] 122-122: The function 'getMaxAccessWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 123-123: The function 'getMaxEgressWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 124-124: The function 'getMaxTransferWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 134-134: The function 'getMaxInnerTimeOfTripBufferSeconds' is never used.
(unusedFunction)
[style] 135-135: The function 'isForwardCalculation' is never used.
(unusedFunction)
[style] 136-136: The function 'getOnlyServices' is never used.
(unusedFunction)
[style] 137-137: The function 'getExceptServices' is never used.
(unusedFunction)
[style] 138-138: The function 'getOnlyLines' is never used.
(unusedFunction)
[style] 139-139: The function 'getExceptLines' is never used.
(unusedFunction)
[style] 140-140: The function 'getOnlyModes' is never used.
(unusedFunction)
[style] 141-141: The function 'getExceptModes' is never used.
(unusedFunction)
[style] 142-142: The function 'getOnlyAgencies' is never used.
(unusedFunction)
[style] 143-143: The function 'getExceptAgencies' is never used.
(unusedFunction)
[style] 144-144: The function 'getOnlyNodes' is never used.
(unusedFunction)
[style] 145-145: The function 'getExceptNodes' is never used.
(unusedFunction)
[style] 146-146: The function 'getWalkingSpeedFactor' is never used.
(unusedFunction)
[style] 147-147: The function 'getWalkingSpeedMetersPerSecond' is never used.
(unusedFunction)
[performance] 234-234: Range variable 'cacheName' should be declared as const reference.
(iterateByValue)
[style] 201-201: The function 'getMaxAlternatives' is never used.
(unusedFunction)
[style] 202-202: The function 'getAlternativesMaxTravelTimeRatio' is never used.
(unusedFunction)
[style] 203-203: The function 'getMinAlternativeMaxTravelTimeSeconds' is never used.
(unusedFunction)
[style] 204-204: The function 'getAlternativesMaxAddedTravelTimeSeconds' is never used.
(unusedFunction)
[style] 205-205: The function 'getMaxValidAlternatives' is never used.
(unusedFunction)
[style] 242-242: The function 'getPlace' is never used.
(unusedFunction)
🪛 markdownlint-cli2 (0.23.0)
README.md
[warning] 8-8: Reference links and images should use a label that is defined
Missing link or image reference definition: "1"
(MD052, reference-links-images)
[warning] 9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "2"
(MD052, reference-links-images)
[warning] 48-48: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
Dockerfile (1)
6-6: LGTM!.github/workflows/c-cpp.yml (1)
26-26: LGTM!Also applies to: 45-45
.github/workflows/codeql-analysis.yml (1)
62-62: LGTM!README.md (1)
44-45: LGTM!configure.ac (1)
15-15: LGTM!Also applies to: 63-65
connection_scan_algorithm/src/transit_routing_http_server.cpp (3)
314-321:/exitstill never callsdrogon::app().quit(). Noting this was already discussed and left as a pre-existing TODO.
103-159: LGTM!
324-369: LGTM!Also applies to: 372-417, 420-457, 459-476
include/parameters.hpp (1)
149-149: LGTM!Also applies to: 216-216, 252-252
connection_scan_algorithm/src/parameters/common_parameters.cpp (1)
68-68: LGTM!connection_scan_algorithm/src/parameters/route_parameters.cpp (1)
58-58: LGTM!connection_scan_algorithm/src/parameters/accessibility_parameters.cpp (1)
42-42: LGTM!
| # TODO The trantor and drogon should check actual lib functions | ||
| AC_CHECK_LIB([trantor], [main], [], [AC_MSG_ERROR([libtrantor not found])]) | ||
| AC_CHECK_LIB([drogon], [main], [], [AC_MSG_ERROR([libdrogon not found])]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use real C++ link probes for Drogon and Trantor.
AC_CHECK_LIB(..., [main]) does not reference either library’s API; main comes from the probe itself. These checks can therefore pass while the actual server link fails. Keep the checks in C++ and use AC_LINK_IFELSE with representative Drogon/Trantor API calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` around lines 76 - 78, Replace the AC_CHECK_LIB checks for
trantor and drogon with C++ AC_LINK_IFELSE probes that compile and link
representative API calls from each library, using the existing configure error
messages when linking fails. Ensure the probes reference actual Drogon and
Trantor symbols rather than the test program’s main function.
| LDFLAGS+=" $JEMALLOC_LIBS" | ||
| LDFLAGS+=" $CAPNP_LIBS" | ||
| LDFLAGS+=" $SPDLOG_LIBS" | ||
| LDFLAGS+=" $JSONCPP_LIBS" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Put JSONCPP_LIBS in LIBS, not LDFLAGS.
JSONCPP_LIBS contains linker options such as -ljsoncpp. Placing them in LDFLAGS can put the library before object files and cause failures with --as-needed.
Suggested fix
-LDFLAGS+=" $JSONCPP_LIBS"
+LIBS+=" $JSONCPP_LIBS"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` at line 95, Move JSONCPP_LIBS from LDFLAGS into LIBS in the
configure setup, preserving the existing value while appending the JSONCPP
linker options to the correct variable.
There was a problem hiding this comment.
yes, you created a new issue for that
There was a problem hiding this comment.
@greenscientist, thanks—sorry for the duplicate. I’ll treat this as already tracked separately.
The SimpleWeb http server library had performance issue with an high thread count. With this we migrate to a newer http library. Drogon is active and have good benchmark. It has a similar structure than what we had before. Updated doc to reflect new packages dependencies Tested with transition and all requests works as before.
5c932ef to
16c1ebb
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
connection_scan_algorithm/src/transit_routing_http_server.cpp (1)
217-311: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
/updateCacheruns on an IO event loop while calculations read the same data.Two concerns here, both new with this design:
- All the
transitData.updateXxx()calls run directly on a Drogon IO loop (only 4 of them), blocking connection handling for the whole cache reload.- Calculations now run concurrently on
computePooland readtransitDatawhile this handler mutates it — an unsynchronized data race, unlike the fully serialized/blocking previous model.Suggest dispatching this handler to a dedicated queue and guarding
transitDataaccess (e.g. a shared_mutex taken exclusively here and shared in the calculation lambdas).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@connection_scan_algorithm/src/transit_routing_http_server.cpp` around lines 217 - 311, Move the /updateCache handler’s cache-reload work out of the Drogon IO event loop onto a dedicated serialized queue, preserving its response behavior while preventing concurrent reloads. Add a shared synchronization primitive for transitData, acquire it exclusively around the updateXxx calls in the /updateCache handler, and acquire shared access in the calculation lambdas that read transitData. Ensure every relevant read and mutation uses the same lock.
♻️ Duplicate comments (1)
configure.ac (1)
76-78: 🎯 Functional Correctness | 🟠 MajorUse real C++ API link probes for Drogon and Trantor.
Line 76-78 still checks the probe’s
mainsymbol instead of representative library APIs, and the checks run afterAC_LANG_POP([C++]). Replace them with C++AC_LINK_IFELSEtests. This is the same unresolved issue reported previously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@configure.ac` around lines 76 - 78, Replace the AC_CHECK_LIB([trantor], [main]) and AC_CHECK_LIB([drogon], [main]) checks with C++ AC_LINK_IFELSE probes that compile and link against representative APIs from each library. Move or keep these probes within the C++ language scope established by AC_LANG_PUSH([C++]), and emit the existing missing-library errors when either probe fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configure.ac`:
- Line 95: Update the linker flag assignment near JSONCPP_LIBS so its value is
appended to LIBS rather than LDFLAGS, preserving the existing JSONCPP_LIBS
contents and ensuring the library is passed in the correct link order.
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp`:
- Around line 145-149: Add a trailing catch (...) safety net after the existing
std::exception handler in the request calculation flow, using the same
unknown-exception logging, PARAM_ERROR_UNKNOWN response, and 400 callback
behavior so non-standard exceptions cannot escape without invoking the client
callback.
- Around line 117-125: Update extractParameters to avoid relying on
request->getParameters(), which collapses duplicate keys and loses query-string
order. Parse the raw query string in request order, preserving repeated
parameter occurrences so later handling of shared fields such as
max_first_waiting_time and max_inner_time_of_trip_buffer has explicit
deterministic precedence.
---
Outside diff comments:
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp`:
- Around line 217-311: Move the /updateCache handler’s cache-reload work out of
the Drogon IO event loop onto a dedicated serialized queue, preserving its
response behavior while preventing concurrent reloads. Add a shared
synchronization primitive for transitData, acquire it exclusively around the
updateXxx calls in the /updateCache handler, and acquire shared access in the
calculation lambdas that read transitData. Ensure every relevant read and
mutation uses the same lock.
---
Duplicate comments:
In `@configure.ac`:
- Around line 76-78: Replace the AC_CHECK_LIB([trantor], [main]) and
AC_CHECK_LIB([drogon], [main]) checks with C++ AC_LINK_IFELSE probes that
compile and link against representative APIs from each library. Move or keep
these probes within the C++ language scope established by AC_LANG_PUSH([C++]),
and emit the existing missing-library errors when either probe fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05655050-5cff-4e09-9b5f-1244549568f7
📒 Files selected for processing (12)
.github/workflows/c-cpp.yml.github/workflows/codeql-analysis.ymlDockerfileREADME.mdconfigure.acconnection_scan_algorithm/src/parameters/accessibility_parameters.cppconnection_scan_algorithm/src/parameters/common_parameters.cppconnection_scan_algorithm/src/parameters/route_parameters.cppconnection_scan_algorithm/src/transit_routing_http_server.cppinclude/parameters.hppinclude/server_http.hppinclude/server_https.hpp
💤 Files with no reviewable changes (2)
- include/server_https.hpp
- include/server_http.hpp
📜 Review details
🧰 Additional context used
🪛 Clang (14.0.6)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[warning] 68-68: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 68-68: method 'createCommonParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 68-68: 2 adjacent parameters of 'createCommonParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 68-68: the first parameter in the range is 'parameters'
(clang)
[note] 68-68: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[warning] 42-42: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 42-42: method 'createAccessibilityParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 42-42: 2 adjacent parameters of 'createAccessibilityParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 42-42: the first parameter in the range is 'parameters'
(clang)
[note] 42-42: the last parameter in the range is 'scenarios'
(clang)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[warning] 58-58: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 58-58: method 'createRouteODParameter' can be made static
(readability-convert-member-functions-to-static)
[warning] 58-58: 2 adjacent parameters of 'createRouteODParameter' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 58-58: the first parameter in the range is 'parameters'
(clang)
[note] 58-58: the last parameter in the range is 'scenarios'
(clang)
include/parameters.hpp
[warning] 149-149: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 216-216: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 252-252: use a trailing return type for this function
(modernize-use-trailing-return-type)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[warning] 35-35: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 36-36: use 'using' instead of 'typedef'
(modernize-use-using)
[warning] 105-105: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 105-105: 2 adjacent parameters of 'makeJsonResponse' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 105-105: the first parameter in the range is 'body'
(clang)
[note] 106-106: the last parameter in the range is 'code'
(clang)
[note] 105-105: after resolving type aliases, 'const std::string &' and 'int' are the same
(clang)
[note] 106-106: 'const std::string &' and 'int' parameters accept and bind the same kind of values
(clang)
[warning] 117-117: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 119-119: variable 'parametersWithValues' is not initialized
(cppcoreguidelines-init-variables)
[warning] 223-223: variable 'cacheNames' is not initialized
(cppcoreguidelines-init-variables)
[warning] 224-224: variable 'cacheNamesVector' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
connection_scan_algorithm/src/parameters/common_parameters.cpp
[style] 68-68: The function 'createCommonParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/accessibility_parameters.cpp
[style] 42-42: The function 'createAccessibilityParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/parameters/route_parameters.cpp
[style] 58-58: The function 'createRouteODParameter' is never used.
(unusedFunction)
connection_scan_algorithm/src/transit_routing_http_server.cpp
[style] 35-35: The function 'getDepartureTime' is never used.
(unusedFunction)
[style] 36-36: The function 'getArrivalTime' is never used.
(unusedFunction)
[style] 36-36: The function 'accept' is never used.
(unusedFunction)
[style] 118-118: The function 'getScenario' is never used.
(unusedFunction)
[style] 119-119: The function 'getTimeOfTrip' is never used.
(unusedFunction)
[style] 120-120: The function 'getMinWaitingTimeSeconds' is never used.
(unusedFunction)
[style] 121-121: The function 'getMaxTotalTravelTimeSeconds' is never used.
(unusedFunction)
[style] 122-122: The function 'getMaxAccessWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 123-123: The function 'getMaxEgressWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 124-124: The function 'getMaxTransferWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 134-134: The function 'getMaxInnerTimeOfTripBufferSeconds' is never used.
(unusedFunction)
[style] 135-135: The function 'isForwardCalculation' is never used.
(unusedFunction)
[style] 136-136: The function 'getOnlyServices' is never used.
(unusedFunction)
[style] 137-137: The function 'getExceptServices' is never used.
(unusedFunction)
[style] 138-138: The function 'getOnlyLines' is never used.
(unusedFunction)
[style] 139-139: The function 'getExceptLines' is never used.
(unusedFunction)
[style] 140-140: The function 'getOnlyModes' is never used.
(unusedFunction)
[style] 141-141: The function 'getExceptModes' is never used.
(unusedFunction)
[style] 142-142: The function 'getOnlyAgencies' is never used.
(unusedFunction)
[style] 143-143: The function 'getExceptAgencies' is never used.
(unusedFunction)
[style] 144-144: The function 'getOnlyNodes' is never used.
(unusedFunction)
[style] 145-145: The function 'getExceptNodes' is never used.
(unusedFunction)
[style] 146-146: The function 'getWalkingSpeedFactor' is never used.
(unusedFunction)
[style] 147-147: The function 'getWalkingSpeedMetersPerSecond' is never used.
(unusedFunction)
[performance] 234-234: Range variable 'cacheName' should be declared as const reference.
(iterateByValue)
[style] 201-201: The function 'getMaxAlternatives' is never used.
(unusedFunction)
[style] 202-202: The function 'getAlternativesMaxTravelTimeRatio' is never used.
(unusedFunction)
[style] 203-203: The function 'getMinAlternativeMaxTravelTimeSeconds' is never used.
(unusedFunction)
[style] 204-204: The function 'getAlternativesMaxAddedTravelTimeSeconds' is never used.
(unusedFunction)
[style] 205-205: The function 'getMaxValidAlternatives' is never used.
(unusedFunction)
[style] 242-242: The function 'getPlace' is never used.
(unusedFunction)
🔇 Additional comments (15)
connection_scan_algorithm/src/transit_routing_http_server.cpp (6)
313-321: Already raised on a previous commit and acknowledged as a pre-existing TODO.
103-125: LGTM!
155-159: LGTM!
324-369: LGTM!
372-457: LGTM!
467-476: 📐 Maintainability & Code QualityNo action needed.
enableReusePortis part ofProgramOptions, and the current behavior is safe to leave as-is.include/parameters.hpp (1)
149-149: LGTM!Also applies to: 216-216, 252-252
connection_scan_algorithm/src/parameters/common_parameters.cpp (1)
68-68: LGTM!connection_scan_algorithm/src/parameters/route_parameters.cpp (1)
58-58: LGTM!connection_scan_algorithm/src/parameters/accessibility_parameters.cpp (1)
42-42: LGTM!Dockerfile (1)
6-6: LGTM!.github/workflows/c-cpp.yml (1)
26-26: LGTM!Also applies to: 45-45
README.md (1)
44-52: LGTM!configure.ac (1)
15-15: LGTM!Also applies to: 63-74, 89-94
.github/workflows/codeql-analysis.yml (1)
62-62: 🎯 Functional CorrectnessNo change needed here.
| LDFLAGS+=" $JEMALLOC_LIBS" | ||
| LDFLAGS+=" $CAPNP_LIBS" | ||
| LDFLAGS+=" $SPDLOG_LIBS" | ||
| LDFLAGS+=" $JSONCPP_LIBS" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Put JSONCPP_LIBS in LIBS, not LDFLAGS.
Line 95 places -ljsoncpp in LDFLAGS, which can put the library before object files and fail with --as-needed. This is the same unresolved issue reported previously.
Suggested fix
-LDFLAGS+=" $JSONCPP_LIBS"
+LIBS+=" $JSONCPP_LIBS"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` at line 95, Update the linker flag assignment near JSONCPP_LIBS
so its value is appended to LIBS rather than LDFLAGS, preserving the existing
JSONCPP_LIBS contents and ensuring the library is passed in the correct link
order.
| static std::vector<std::pair<std::string, std::string>> extractParameters(const drogon::HttpRequestPtr &request) | ||
| { | ||
| std::vector<std::pair<std::string, std::string>> parametersWithValues; | ||
| for (const auto &field : request->getParameters()) | ||
| { | ||
| parametersWithValues.push_back(std::make_pair(field.first, field.second)); | ||
| } | ||
| return parametersWithValues; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Drogon HttpRequest getParameters duplicate query parameters map type
💡 Result:
In the Drogon C++ web framework, the HttpRequest::getParameters method returns a map-like container (specifically, a SafeStringMap, which internally uses std::unordered_map) [1][2][3]. Due to this underlying storage type, the framework does not support duplicate query parameters with the same key [4][5]. When a request contains multiple parameters with the same key (e.g.,?key=hello&key=world), the default parsing behavior is to overwrite previous values, effectively keeping only the last occurrence [4][5]. If you need to handle multiple values for a single key, the recommended approach is to use req->getQuery to retrieve the raw query string and manually parse it using a custom function [4][5]. This allows you to implement your own logic for handling duplicates according to your specific requirements [5]. There is no native support for multi-value parameter maps (such as std::unordered_multimap) in the standard HttpRequest API, as this would be a breaking change to the existing interface [5].
Citations:
- 1: https://github.com/drogonframework/drogon/blob/master/lib/inc/drogon/HttpRequest.h
- 2: https://github.com/drogonframework/drogon/blob/59cd4366/lib/inc/drogon/HttpRequest.h
- 3: https://github.com/drogonframework/drogon/blob/acad9c8e/lib/src/HttpRequestImpl.h
- 4: getParameters drogonframework/drogon#1645
- 5: [FR] Alternative container for parsed request parameters drogonframework/drogon#808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'transit_routing_http_server.cpp|common_parameters.cpp' . | sed 's#^\./##'
echo "== relevant snippets =="
for f in $(fd 'transit_routing_http_server.cpp|common_parameters.cpp' .); do
echo "--- $f ---"
wc -l "$f"
rg -n "extractParameters|max_first_waiting_time|max_inner_time_of_trip_buffer|setParameter|getParameters|common_parameters" "$f" -C 4 || true
done
echo "== git diff stat/name-only =="
git diff --stat || true
git diff --name-only || trueRepository: chairemobilite/trRouting
Length of output: 3655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== common_parameters parameter parser context =="
sed -n '120,210p' connection_scan_algorithm/src/parameters/common_parameters.cpp | cat -n
echo "== previous query-string parser implementation =="
rg -n "parseQuery|extractQuery|getQuery|queryString|std::multimap|multimap|duplicate|duplicate" connection_scan_algorithm/src -S
echo "== route/summary/accessibility call sites around parameter extraction =="
for f in connection_scan_algorithm/src/transit_routing_http_server.cpp; do
sed -n '220,250p' "$f" | cat -n
sed -n '325,345p' "$f" | cat -n
sed -n '378,398p' "$f" | cat -n
sed -n '427,447p' "$f" | cat -n
doneRepository: chairemobilite/trRouting
Length of output: 4017
Handle order-sensitive or duplicate query parameters explicitly.
request->getParameters() is a map, so duplicate keys collapse and iteration order does not preserve request order. Since parameters such as max_first_waiting_time and max_inner_time_of_trip_buffer write the same field, preserve the raw query-string order or define deterministic precedence for repeated keys.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 117-117: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 119-119: variable 'parametersWithValues' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
[style] 118-118: The function 'getScenario' is never used.
(unusedFunction)
[style] 119-119: The function 'getTimeOfTrip' is never used.
(unusedFunction)
[style] 120-120: The function 'getMinWaitingTimeSeconds' is never used.
(unusedFunction)
[style] 121-121: The function 'getMaxTotalTravelTimeSeconds' is never used.
(unusedFunction)
[style] 122-122: The function 'getMaxAccessWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 123-123: The function 'getMaxEgressWalkingTravelTimeSeconds' is never used.
(unusedFunction)
[style] 124-124: The function 'getMaxTransferWalkingTravelTimeSeconds' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp` around lines
117 - 125, Update extractParameters to avoid relying on
request->getParameters(), which collapses duplicate keys and loses query-string
order. Parse the raw query string in request order, preserving repeated
parameter occurrences so later handling of shared fields such as
max_first_waiting_time and max_inner_time_of_trip_buffer has explicit
deterministic precedence.
There was a problem hiding this comment.
@coderabbitai Can we create an issue for this, thanks
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="chairemobilite/trRouting"
body=$(cat <<'EOF'
## Summary
Preserve duplicate query parameters and their request order when converting HTTP query strings into routing parameters.
## Rationale
The Drogon migration currently uses `HttpRequest::getParameters()` in `extractParameters` (`connection_scan_algorithm/src/transit_routing_http_server.cpp`). Its map-like storage collapses duplicate keys and does not preserve query-string order. This can change effective values when repeated or order-sensitive parameters are supplied, including parameters that update the same underlying field.
## Required changes
- Avoid relying solely on `request->getParameters()` for routing parameter extraction.
- Parse the raw query string in request order and preserve repeated parameter occurrences, or define and implement explicit deterministic precedence for repeated keys.
- Keep the existing route, summary, and accessibility endpoint parameter behavior compatible where possible.
## Affected area
- `connection_scan_algorithm/src/transit_routing_http_server.cpp`
- `extractParameters`
- `/v2/route`, `/v2/summary`, and `/v2/accessibility`
## Acceptance criteria
- Repeated query keys are handled deterministically.
- Query parameter order is preserved when it affects parsing precedence.
- Tests cover duplicate/order-sensitive query parameters.
## Backlinks
- PR: https://github.com/chairemobilite/trRouting/pull/351
- Review comment: https://github.com/chairemobilite/trRouting/pull/351#discussion_r3649498398
Requested by: `@greenscientist`
EOF
)
gh issue create \
--repo "$repo" \
--title "Preserve duplicate and ordered HTTP query parameters" \
--body "$body" \
--assignee "greenscientist"Length of output: 1927
@greenscientist, created and assigned: #361.
| } catch (const std::exception &e) { | ||
| spdlog::error("-- unknown exception in {} calculation -- {}", name, e.what()); | ||
| std::string response = "{\"status\": \"query_error\", \"errorCode\": \"PARAM_ERROR_UNKNOWN\"}"; | ||
| callback(makeJsonResponse(response, drogon::k400BadRequest)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider a catch (...) safety net.
Only std::exception is caught, so any non-standard exception escapes into the trantor worker thread: the client callback is never invoked (request hangs) and the process likely terminates. A trailing catch (...) keeps the old "unknown exception → 400" contract.
♻️ Suggested change
} catch (const std::exception &e) {
spdlog::error("-- unknown exception in {} calculation -- {}", name, e.what());
std::string response = "{\"status\": \"query_error\", \"errorCode\": \"PARAM_ERROR_UNKNOWN\"}";
callback(makeJsonResponse(response, drogon::k400BadRequest));
+ } catch (...) {
+ spdlog::error("-- unknown non-standard exception in {} calculation --", name);
+ std::string response = "{\"status\": \"query_error\", \"errorCode\": \"PARAM_ERROR_UNKNOWN\"}";
+ callback(makeJsonResponse(response, drogon::k400BadRequest));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (const std::exception &e) { | |
| spdlog::error("-- unknown exception in {} calculation -- {}", name, e.what()); | |
| std::string response = "{\"status\": \"query_error\", \"errorCode\": \"PARAM_ERROR_UNKNOWN\"}"; | |
| callback(makeJsonResponse(response, drogon::k400BadRequest)); | |
| } | |
| } catch (const std::exception &e) { | |
| spdlog::error("-- unknown exception in {} calculation -- {}", name, e.what()); | |
| std::string response = "{\"status\": \"query_error\", \"errorCode\": \"PARAM_ERROR_UNKNOWN\"}"; | |
| callback(makeJsonResponse(response, drogon::k400BadRequest)); | |
| } catch (...) { | |
| spdlog::error("-- unknown non-standard exception in {} calculation --", name); | |
| std::string response = "{\"status\": \"query_error\", \"errorCode\": \"PARAM_ERROR_UNKNOWN\"}"; | |
| callback(makeJsonResponse(response, drogon::k400BadRequest)); | |
| } |
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 145-145: The function 'getExceptNodes' is never used.
(unusedFunction)
[style] 146-146: The function 'getWalkingSpeedFactor' is never used.
(unusedFunction)
[style] 147-147: The function 'getWalkingSpeedMetersPerSecond' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp` around lines
145 - 149, Add a trailing catch (...) safety net after the existing
std::exception handler in the request calculation flow, using the same
unknown-exception logging, PARAM_ERROR_UNKNOWN response, and 400 callback
behavior so non-standard exceptions cannot escape without invoking the client
callback.
2 participants
The SimpleWeb http server library had performance issue with an high thread count. With this we migrate to a newer http library. Drogon is active and have good benchmark. It has a similar structure than what we had before.
Updated doc to reflect new packages dependencies
Tested with transition and all requests works as before.
Summary by CodeRabbit
New Features
/v2/*, cache updates, and shutdown.Build & Compatibility
Documentation