Update dependency version#1499
Conversation
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
Signed-off-by: JaylinYu <letrangerjaylin@gmail.com>
📝 WalkthroughWalkthroughThis PR includes maintenance updates across build configuration, logging, authentication state handling, and encryption code. Changes remove C++ standard configuration, update a log message, reset authentication state during parsing, add parquet library dependencies, and wrap encryption keys in secure containers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
|
|
This may breaks current CI workflow @xinyi-xs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/supplemental/nanolib/conf.c (1)
1609-1625:⚠️ Potential issue | 🟠 MajorOpen the new config before dropping existing auth state.
Line 1609 clears the active auth entries before Line 1623 proves the config can be read. If
fopenfails, the function returns withauth->enable = falseand all previous credentials freed.🛡️ Proposed fix
- if (auth->count > 0) { - for (size_t i = 0; i < auth->count; i++) { - free(auth->usernames[i]); - free(auth->passwords[i]); - } - cvector_free(auth->usernames); - cvector_free(auth->passwords); - auth->usernames = NULL; - auth->passwords = NULL; - auth->count = 0; - auth->enable = false; - } - FILE *fp; if ((fp = fopen(path, "r")) == NULL) { log_error("File %s open failed", path); return; } + + if (auth->count > 0) { + for (size_t i = 0; i < auth->count; i++) { + free(auth->usernames[i]); + free(auth->passwords[i]); + } + cvector_free(auth->usernames); + cvector_free(auth->passwords); + auth->usernames = NULL; + auth->passwords = NULL; + auth->count = 0; + auth->enable = false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/supplemental/nanolib/conf.c` around lines 1609 - 1625, The code currently frees existing credentials (auth->usernames, auth->passwords, sets auth->count=0 and auth->enable=false) before trying to open the new config file with fopen(path, "r"), which loses credentials if fopen fails; change the logic so the file is opened first (call fopen(path, "r") and verify it succeeded) and only after successful open proceed to free or replace the existing auth state (the blocks that free auth->usernames/auth->passwords and set auth->count/auth->enable). Ensure fopen failure returns without touching auth, and close the file when done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/supplemental/nanolib/parquet/CMakeLists.txt`:
- Around line 3-16: Replace the raw target_link_libraries(nng PRIVATE ...) block
with calls to the nng_link_libraries helper after the existing
find_package(Arrow) / find_package(Parquet) calls so both nng and nng_testing
get linked and Arrow/Parquet imported targets supply their transitive deps;
remove the hard-coded bare library names (thrift, brotlienc, brotlidec,
brotlicommon, bz2, lz4, snappy, zstd, ssl, crypto, dl, z) and instead rely on
the Arrow/Parquet imported targets (arrow_shared/parquet_shared or
arrow_static/parquet_static) via nng_link_libraries (NNGHelpers.cmake); if a
platform-only system lib is actually required (e.g., dl on Linux), add it
conditionally after find_package using a platform guard (e.g., only add dl on
UNIX AND NOT APPLE) and link it via nng_link_libraries so nng_testing is handled
too.
In `@src/supplemental/nanolib/parquet/parquet.cc`:
- Around line 807-824: The FileDecryptionProperties built into
decryption_configuration is single-use and must not be reused across multiple
ParquetFileReader::OpenFile calls; inspect the callers (parquet_read, the
parquet_read overload, and parquet_read_span_by_column) to ensure each creates
its own reader_properties and uses the decryption_configuration exactly once,
and then add a short inline comment next to the creation/assignment of
decryption_configuration (around
reader_properties.file_decryption_properties(...)) stating the single-use
invariant (e.g., "FileDecryptionProperties is consumed by a single OpenFile
call; do not reuse or remove DeepClone unless this invariant holds") so future
maintainers do not unknowingly reuse the same properties; if you find any code
path that shares the same reader_properties/decryption_configuration across
multiple OpenFile calls, restore DeepClone() or otherwise allocate per-open
instances to ensure each OpenFile receives a fresh FileDecryptionProperties.
---
Outside diff comments:
In `@src/supplemental/nanolib/conf.c`:
- Around line 1609-1625: The code currently frees existing credentials
(auth->usernames, auth->passwords, sets auth->count=0 and auth->enable=false)
before trying to open the new config file with fopen(path, "r"), which loses
credentials if fopen fails; change the logic so the file is opened first (call
fopen(path, "r") and verify it succeeded) and only after successful open proceed
to free or replace the existing auth state (the blocks that free
auth->usernames/auth->passwords and set auth->count/auth->enable). Ensure fopen
failure returns without touching auth, and close the file when done.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0d05729-fdcc-4892-bf81-6828c514e2ca
📒 Files selected for processing (5)
CMakeLists.txtsrc/sp/protocol/mqtt/nmq_mqtt.csrc/supplemental/nanolib/conf.csrc/supplemental/nanolib/parquet/CMakeLists.txtsrc/supplemental/nanolib/parquet/parquet.cc
💤 Files with no reviewable changes (1)
- CMakeLists.txt
| target_link_libraries(nng PRIVATE | ||
| thrift | ||
| brotlienc | ||
| brotlidec | ||
| brotlicommon | ||
| bz2 | ||
| lz4 | ||
| snappy | ||
| zstd | ||
| ssl | ||
| crypto | ||
| dl | ||
| z | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how other CMakeLists link external deps and whether nng_testing is used with parquet.
fd -a 'CMakeLists.txt' src/supplemental | xargs rg -nP -C2 '(nng_link_libraries|target_link_libraries\(nng\b)'
rg -nP -C3 'NNG_ENABLE_PARQUET'
rg -nP -C3 'arrow_bundled_dependencies|Arrow::|Parquet::'Repository: nanomq/NanoNNG
Length of output: 12513
🏁 Script executed:
cat cmake/NNGHelpers.cmake | head -100Repository: nanomq/NanoNNG
Length of output: 3632
🏁 Script executed:
rg -A 20 'function\(nng_link_libraries' cmake/Repository: nanomq/NanoNNG
Length of output: 2968
🏁 Script executed:
rg -n 'nng_testing' src/supplemental/nanolib/parquet/ CMakeLists.txtRepository: nanomq/NanoNNG
Length of output: 498
Use nng_link_libraries helper instead of raw target_link_libraries, and defer hardcoded deps until after find_package(Arrow/Parquet) with imported targets.
All four concerns are valid:
-
Missing
nng_testinglinkage. Thenng_link_librarieshelper (incmake/NNGHelpers.cmake) links bothnngandnng_testing. Using rawtarget_link_libraries(nng PRIVATE ...)skipsnng_testing, which will break tests whenNNG_ENABLE_PARQUETis on. -
Hard-coded bare library names break portability. The project consistently uses imported targets elsewhere (
OpenSSL::SSL,MbedTLS::mbedtls, etc.). Linking plain names likethrift,bz2,lz4,snappy,zstd,ssl,crypto,brotli*,z,dlfails in cross-compilation, vcpkg/Conan builds, and non-Linux platforms (e.g.,dldoes not exist on Windows;ssl/crypto/znames differ). -
Redundant with Arrow/Parquet imported targets. The same file later uses
nng_link_libraries(arrow_shared parquet_shared)/nng_link_libraries(arrow_static parquet_static), indicating Arrow/Parquet provide imported targets carrying their own transitive dependencies. Duplicating them manually risks version skew and breakage. -
Wrong order. The
find_package(Arrow)/find_package(Parquet)calls come at lines 17–18, after the problematic block. Imported targets are not available yet.
Refactor: Remove the hardcoded block entirely. After find_package(Arrow/Parquet), use the imported targets via nng_link_libraries. For truly missing system libs (e.g., dl on POSIX), add them conditionally after find_package:
if(UNIX AND NOT APPLE)
nng_link_libraries(dl)
endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/supplemental/nanolib/parquet/CMakeLists.txt` around lines 3 - 16, Replace
the raw target_link_libraries(nng PRIVATE ...) block with calls to the
nng_link_libraries helper after the existing find_package(Arrow) /
find_package(Parquet) calls so both nng and nng_testing get linked and
Arrow/Parquet imported targets supply their transitive deps; remove the
hard-coded bare library names (thrift, brotlienc, brotlidec, brotlicommon, bz2,
lz4, snappy, zstd, ssl, crypto, dl, z) and instead rely on the Arrow/Parquet
imported targets (arrow_shared/parquet_shared or arrow_static/parquet_static)
via nng_link_libraries (NNGHelpers.cmake); if a platform-only system lib is
actually required (e.g., dl on Linux), add it conditionally after find_package
using a platform guard (e.g., only add dl on UNIX AND NOT APPLE) and link it via
nng_link_libraries so nng_testing is handled too.
| if (conf->encryption.enable) { | ||
| map<string, | ||
| shared_ptr<parquet::ColumnDecryptionProperties>> | ||
| map<string, shared_ptr<parquet::ColumnDecryptionProperties>> | ||
| decryption_cols; | ||
| parquet::FileDecryptionProperties::Builder | ||
| file_decryption_builder_3; | ||
| shared_ptr<parquet::FileDecryptionProperties> | ||
| decryption_configuration = | ||
| file_decryption_builder_3 | ||
| .footer_key(conf->encryption.key) | ||
| .footer_key(arrow::util::SecureString( | ||
| conf->encryption.key)) | ||
| ->column_keys(decryption_cols) | ||
| ->build(); | ||
|
|
||
| // Add the current decryption configuration to | ||
| // ReaderProperties. | ||
| reader_properties.file_decryption_properties( | ||
| decryption_configuration->DeepClone()); | ||
| decryption_configuration); | ||
| } |
There was a problem hiding this comment.
Passing the built FileDecryptionProperties without DeepClone() — confirm single-use semantics.
Dropping DeepClone() is fine iff reader_properties (and this decryption_configuration shared_ptr) are used for exactly one ParquetFileReader::OpenFile call. Arrow's FileDecryptionProperties is documented as single-use: once consumed by a reader it cannot be reused for another file, which was the original reason for DeepClone().
Looking at the callers (parquet_read, parquet_read overload, parquet_read_span_by_column), each builds its own reader_properties locally and opens one file, so this should be safe today. Please double-check there's no path where the same reader_properties/builder output is fed to more than one OpenFile, and ideally add a short comment noting the single-use invariant to prevent future regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/supplemental/nanolib/parquet/parquet.cc` around lines 807 - 824, The
FileDecryptionProperties built into decryption_configuration is single-use and
must not be reused across multiple ParquetFileReader::OpenFile calls; inspect
the callers (parquet_read, the parquet_read overload, and
parquet_read_span_by_column) to ensure each creates its own reader_properties
and uses the decryption_configuration exactly once, and then add a short inline
comment next to the creation/assignment of decryption_configuration (around
reader_properties.file_decryption_properties(...)) stating the single-use
invariant (e.g., "FileDecryptionProperties is consumed by a single OpenFile
call; do not reuse or remove DeepClone unless this invariant holds") so future
maintainers do not unknowingly reuse the same properties; if you find any code
path that shares the same reader_properties/decryption_configuration across
multiple OpenFile calls, restore DeepClone() or otherwise allocate per-open
instances to ensure each OpenFile receives a fresh FileDecryptionProperties.
There was a problem hiding this comment.
Pull request overview
Updates the Parquet/Arrow integration to match newer dependency APIs and adjusts build/config behavior to support the updated dependency requirements across the project.
Changes:
- Updated Parquet encryption/decryption key handling to use
arrow::util::SecureString. - Adjusted auth config parsing to free/reset previously loaded username/password entries on re-parse.
- Modified CMake linkage for the Parquet supplemental target and removed the global C++ standard setting; also tweaked an MQTT log message.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/supplemental/nanolib/parquet/parquet.cc |
Updates Parquet encryption/decryption APIs (SecureString, decryption props handling). |
src/supplemental/nanolib/parquet/CMakeLists.txt |
Adds explicit link dependencies for Parquet support. |
src/supplemental/nanolib/conf.c |
Frees/reset auth entries on re-parse and recalculates count from vector size. |
src/sp/protocol/mqtt/nmq_mqtt.c |
Refines a log message for a missing preset-session qos_db case. |
CMakeLists.txt |
Removes the global CMAKE_CXX_STANDARD setting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nng_sources(parquet.cc parquet_file_queue.cc) | ||
| target_link_libraries(nng PRIVATE | ||
| thrift | ||
| brotlienc | ||
| brotlidec | ||
| brotlicommon | ||
| bz2 |
There was a problem hiding this comment.
This target_link_libraries(nng PRIVATE ...) block only links these extra dependencies to nng, but nng_sources(...) also adds the Parquet sources to nng_testing. If these libraries are actually required to satisfy Parquet/Arrow symbols, the nng_testing target can fail to link; use nng_link_libraries(...) (or also link nng_testing) so both targets get consistent dependencies.
| target_link_libraries(nng PRIVATE | ||
| thrift | ||
| brotlienc | ||
| brotlidec | ||
| brotlicommon | ||
| bz2 | ||
| lz4 | ||
| snappy | ||
| zstd | ||
| ssl | ||
| crypto | ||
| dl | ||
| z | ||
| ) |
There was a problem hiding this comment.
Linking against raw library names like ssl, crypto, dl, z, and compression libs is not portable and is inconsistent with the rest of the build, which uses imported targets (e.g., OpenSSL::Crypto) and helper wrappers. Prefer relying on Arrow/Parquet imported targets’ transitive link interfaces, or link via imported targets / CMake variables (e.g., OpenSSL::*, ZLIB::ZLIB, ${CMAKE_DL_LIBS}) to avoid platform-specific link failures.
| target_link_libraries(nng PRIVATE | |
| thrift | |
| brotlienc | |
| brotlidec | |
| brotlicommon | |
| bz2 | |
| lz4 | |
| snappy | |
| zstd | |
| ssl | |
| crypto | |
| dl | |
| z | |
| ) |
|
|
||
| set (CMAKE_CXX_STANDARD 11) | ||
| # add_definitions(-std=c++11) | ||
| set(CMAKE_C_STANDARD 99) |
There was a problem hiding this comment.
This project builds C++ sources (e.g., Parquet/BLF support uses C++17 features like std::optional). After removing the explicit C++ standard setting, builds may fall back to an older compiler default and fail. Please set an explicit minimum C++ standard (likely C++17) via CMAKE_CXX_STANDARD or target_compile_features(nng PRIVATE cxx_std_17) (and similarly for nng_testing) to make the build deterministic.
| set(CMAKE_C_STANDARD 99) | |
| set(CMAKE_C_STANDARD 99) | |
| set(CMAKE_CXX_STANDARD 17) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |
| set(CMAKE_CXX_EXTENSIONS OFF) |
| if (auth->count > 0) { | ||
| for (size_t i = 0; i < auth->count; i++) { | ||
| free(auth->usernames[i]); | ||
| free(auth->passwords[i]); | ||
| } | ||
| cvector_free(auth->usernames); | ||
| cvector_free(auth->passwords); | ||
| auth->usernames = NULL; | ||
| auth->passwords = NULL; | ||
| auth->count = 0; | ||
| auth->enable = false; | ||
| } |
There was a problem hiding this comment.
conf_auth_parse now supports re-parsing by freeing/resetting existing auth entries. There are existing unit tests for config parsing in conf_test.c, but none cover re-parsing auth entries (e.g., parse once with N users, then parse again with a different set and assert count, enable, and contents update correctly). Adding a focused unit test would help prevent regressions in this new behavior.
| parquet::FileEncryptionProperties::Builder file_encryption_builder( | ||
| conf->encryption.key); | ||
| arrow::util::SecureString(conf->encryption.key)); |
There was a problem hiding this comment.
arrow::util::SecureString is used here but this file doesn’t include the SecureString header, which can cause build failures depending on Arrow header transitive includes. Add the appropriate Arrow include (e.g., the SecureString header) near the top of this file so the type is declared explicitly.
wanghaEMQ
left a comment
wanghaEMQ
left a comment
There was a problem hiding this comment.
And you didn't say which version of parquet and thrift are you using now
| @@ -1,5 +1,19 @@ | |||
| if(NNG_ENABLE_PARQUET) | |||
| nng_sources(parquet.cc parquet_file_queue.cc) | |||
| target_link_libraries(nng PRIVATE | |||
There was a problem hiding this comment.
Not a reasonable way to add dependency for nng
| // the same key. (uniform encryption) | ||
| parquet::FileEncryptionProperties::Builder file_encryption_builder( | ||
| conf->encryption.key); | ||
| arrow::util::SecureString(conf->encryption.key)); |
Workflows: sdv_master and master |
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements