BAF-1250 gateway crashes when endpoint contains module missing in module paths #48
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce safer module lookup patterns and add cross-reference validation to ensure modules referenced in external connection endpoints exist in module configurations. Three files are modified: one implements defensive map access, one adds validation logic, and one introduces test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/bringauto/settings/SettingsParser.cpp (1)
112-122: Validation logic is correct, but relies on C++20 and only reports the first missing moduleThe new cross-check correctly makes settings invalid if any endpoint module is not present in
modulePaths, and it logs an error on detection. Two points to consider:
Toolchain requirements
std::ranges::any_ofandsettings_->modulePaths.contains(externalModuleId)are C++20 features. Ensure your build uses C++20; if you must stay on C++17, you’ll need to replace them (e.g. regular loops +count(...) == 0).Diagnostics only for the first missing module
- Because both
std::ranges::any_ofcalls short‑circuit, you only log for the first missing module in the first failing endpoint. If you’d like to report all missing modules across all endpoints (in line with other validations that iterate over everything), you could switch to explicit loops:- isCorrect &= !std::ranges::any_of(settings_->externalConnectionSettingsList, [&](auto& externalConnectionSettings){ - return std::ranges::any_of(externalConnectionSettings.modules, [&](auto const& externalModuleId) { - bool isMissing = !settings_->modulePaths.contains(externalModuleId); - if (isMissing) - { - std::cerr << "Module " << externalModuleId << - " is defined in external-connection endpoint modules but is not specified in module-paths" << std::endl; - } - return isMissing; - }); - }); + bool hasMissingModules = false; + for (const auto& externalConnectionSettings : settings_->externalConnectionSettingsList) { + for (const auto externalModuleId : externalConnectionSettings.modules) { + const bool isMissing = !settings_->modulePaths.contains(externalModuleId); + if (isMissing) { + std::cerr << "Module " << externalModuleId + << " is defined in external-connection endpoint modules but is not specified in module-paths" + << std::endl; + hasMissingModules = true; + } + } + } + isCorrect &= !hasMissingModules;This keeps the same semantics but improves diagnostics and avoids nested
any_ofcomplexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
source/bringauto/external_client/connection/ExternalConnection.cpp(1 hunks)source/bringauto/settings/SettingsParser.cpp(2 hunks)test/source/SettingsParserTests.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-10-03T11:24:43.894Z
Learnt from: Mayt34
Repo: bringauto/transparent-module PR: 1
File: source/bringauto/modules/devices/testing_device/testing_module_manager.cpp:25-34
Timestamp: 2024-10-03T11:24:43.894Z
Learning: In the `testing_device_aggregate_status` function for the testing device, status aggregation of current and new status is not necessary due to the module's simple logic, similar to the io-module which works without aggregation.
Applied to files:
source/bringauto/external_client/connection/ExternalConnection.cpp
📚 Learning: 2024-10-04T07:17:14.935Z
Learnt from: Mayt34
Repo: bringauto/transparent-module PR: 1
File: source/external_server_api.cpp:231-263
Timestamp: 2024-10-04T07:17:14.935Z
Learning: In `external_server_api.cpp`, the number of devices will always be small, so it's acceptable to use a vector for device storage in functions like `device_connected` and `device_disconnected`, and optimizing with a hash table is unnecessary.
Applied to files:
source/bringauto/external_client/connection/ExternalConnection.cpp
📚 Learning: 2024-11-06T13:26:30.538Z
Learnt from: MarioIvancik
Repo: bringauto/virtual-vehicle PR: 19
File: test/source/SettingsParserTest.cpp:264-264
Timestamp: 2024-11-06T13:26:30.538Z
Learning: In `test/source/SettingsParserTest.cpp`, the exception messages expected in the unit tests are correct and consistent, matching the actual messages thrown by `parseConfig`, and the unit tests are passing.
Applied to files:
test/source/SettingsParserTests.cppsource/bringauto/settings/SettingsParser.cpp
📚 Learning: 2024-11-06T13:25:36.211Z
Learnt from: MarioIvancik
Repo: bringauto/virtual-vehicle PR: 19
File: test/source/SettingsParserTest.cpp:259-268
Timestamp: 2024-11-06T13:25:36.211Z
Learning: In `SettingsParserTest.cpp`, the `parseConfig` function adds the `--config` parameter by default, so providing another `--config` parameter results in duplicate command-line options for testing purposes.
Applied to files:
test/source/SettingsParserTests.cpp
📚 Learning: 2024-10-31T10:45:48.494Z
Learnt from: MarioIvancik
Repo: bringauto/module-gateway PR: 38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in `include/bringauto/structures/LoggingSettings.hpp` are validated in `SettingsParser.cpp`, so additional validation within the structures is unnecessary.
Applied to files:
test/source/SettingsParserTests.cppsource/bringauto/settings/SettingsParser.cpp
📚 Learning: 2024-10-31T10:43:48.276Z
Learnt from: MarioIvancik
Repo: bringauto/module-gateway PR: 38
File: include/bringauto/structures/LoggingSettings.hpp:17-24
Timestamp: 2024-10-31T10:43:48.276Z
Learning: In the C++ codebase, the `FileSettings` structure's values are validated in `SettingsParser`.
Applied to files:
source/bringauto/settings/SettingsParser.cpp
📚 Learning: 2024-10-31T10:44:39.657Z
Learnt from: MarioIvancik
Repo: bringauto/module-gateway PR: 38
File: include/bringauto/structures/LoggingSettings.hpp:26-31
Timestamp: 2024-10-31T10:44:39.657Z
Learning: In the `ModuleGateway` project, logging settings are validated in `SettingsParser`, so additional validation in the `LoggingSettings` structure is not necessary.
Applied to files:
source/bringauto/settings/SettingsParser.cpp
🧬 Code graph analysis (2)
source/bringauto/external_client/connection/ExternalConnection.cpp (1)
include/bringauto/settings/LoggerWrapper.hpp (2)
logWarning(51-56)logWarning(51-51)
test/source/SettingsParserTests.cpp (1)
test/include/SettingsParserTests.hpp (2)
config(22-39)config(22-22)
🔇 Additional comments (2)
source/bringauto/settings/SettingsParser.cpp (1)
8-8: Confirm C++20 availability for<ranges>The new
#include <ranges>is required forstd::ranges::any_ofused below. Please confirm the project is compiled with at least C++20 (or a libstdc++/libc++ that fully supports<ranges>); otherwise this will fail to build on older toolchains.test/source/SettingsParserTests.cpp (1)
189-205: NewMissingModulestest correctly covers the validation pathThe test setup (module_paths
{1,2,3}vs endpoint.modules{1,2,3,4}) and assertion onstd::invalid_argument("Arguments are not correct.")align with the new settings validation and existing test patterns. Looks good.
|
When there is module in config file defined under external-connection::endpoints that is not defined in global module-paths config, it crashed the module-gateway when it attempted to connect to external server.
This pull request fixes the crash and prevents start of gateway with incorrect configuration file.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.