fix(io): correct IO type name in AsyncIOParameter JSON constructor#1723
Hidden character warning
fix(io): correct IO type name in AsyncIOParameter JSON constructor#1723LHT129 wants to merge 2 commits intoantgroup:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request corrects an issue in the AsyncIOParameter JSON constructor where an incorrect IO type was being used. The change aligns the constructor with the rest of the class. I have included one suggestion to refactor the constructors using delegation. This would improve maintainability by reducing code duplication and help prevent similar errors from occurring in the future.
src/io/async_io_parameter.cpp
Outdated
| AsyncIOParameter::AsyncIOParameter(const vsag::JsonType& json) | ||
| : IOParameter(IO_TYPE_VALUE_BUFFER_IO) { | ||
| : IOParameter(IO_TYPE_VALUE_ASYNC_IO) { | ||
| this->FromJson(json); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) | ||
| } |
There was a problem hiding this comment.
To improve maintainability and prevent similar copy-paste errors, consider using a delegating constructor. This follows the Don't Repeat Yourself (DRY) principle by ensuring the IO_TYPE_VALUE_ASYNC_IO is only specified in one place (the default constructor), which would have prevented the original bug.
AsyncIOParameter::AsyncIOParameter(const vsag::JsonType& json) : AsyncIOParameter() {
this->FromJson(json); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall)
}There was a problem hiding this comment.
Good suggestion! Created follow-up commit to use delegating constructor pattern. See commit below.
There was a problem hiding this comment.
Pull request overview
Fixes a copy-paste error where AsyncIOParameter’s JSON constructor was initializing its base IOParameter with the wrong IO type constant, causing inconsistent type identification for JSON-based construction.
Changes:
- Updated
AsyncIOParameter(const vsag::JsonType&)to passIO_TYPE_VALUE_ASYNC_IOto theIOParameterbase constructor (matching the default constructor)
e423c5a to
bbf6987
Compare
Use AsyncIOParameter() delegating constructor instead of directly calling IOParameter(IO_TYPE_VALUE_BUFFER_IO), which was incorrect. Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
bbf6987 to
f2f8aae
Compare
There was a problem hiding this comment.
Pull request overview
Fixes incorrect IO type assignment when constructing AsyncIOParameter from JSON by ensuring it uses the same initialization path as the default constructor, aligning serialization/deserialization behavior and type identification.
Changes:
- Replace incorrect base-class initialization (
IO_TYPE_VALUE_BUFFER_IO) with delegating construction viaAsyncIOParameter(). - Keep JSON parsing behavior (
FromJson(json)) unchanged after correct initialization.
| AsyncIOParameter::AsyncIOParameter(const vsag::JsonType& json) | ||
| : IOParameter(IO_TYPE_VALUE_BUFFER_IO) { | ||
| : AsyncIOParameter() { | ||
| this->FromJson(json); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) |
There was a problem hiding this comment.
FromJson appears to be virtual (per the NOLINT), and calling virtual functions from constructors can cause surprising behavior if AsyncIOParameter is used as a base class (virtual dispatch won’t reach further-derived overrides during base construction). Consider making the class final, making FromJson non-virtual, or moving JSON parsing into a non-virtual helper/static factory that runs after full object construction.
| this->FromJson(json); // NOLINT(clang-analyzer-optin.cplusplus.VirtualCall) | |
| CHECK_ARGUMENT(json.Contains(IO_FILE_PATH_KEY), "miss file_path param in async io type"); | |
| this->path_ = json[IO_FILE_PATH_KEY].GetString(); |
Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Summary
Fix incorrect IO type name in
AsyncIOParameterJSON constructor. The constructor was usingIO_TYPE_VALUE_BUFFER_IOinstead ofIO_TYPE_VALUE_ASYNC_IO.Changes
IO_TYPE_VALUE_BUFFER_IOtoIO_TYPE_VALUE_ASYNC_IOin the JSON constructor ofAsyncIOParameteratsrc/io/async_io_parameter.cpp:26Background
The JSON constructor was incorrectly passing
IO_TYPE_VALUE_BUFFER_IOto the base class, while the default constructor correctly usesIO_TYPE_VALUE_ASYNC_IO. This was a copy-paste error that could cause:AsyncIOParameterfrom JSON configurationToJson()serialization/deserializationTesting
make releaseRelated Issues
Fixes #1722
Checklist