-
Notifications
You must be signed in to change notification settings - Fork 4
Draft: refactor(server-request-manager): fix build errors and major refactors #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft: refactor(server-request-manager): fix build errors and major refactors #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the server request manager to fix build errors and update the code to use the new IClientHandler interface and revised ClientInfo handling. Key changes include:
- Replacing the old ClientHandler class with the IClientHandler interface.
- Updating test functions (e.g., connect_test_client) to return a tuple with a success flag.
- Adjusting ConnectionManager and request manager implementations to use the new client information model.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/server/server_connection_manager_test.cpp | Updated test signatures and assertions to match the new ClientInfo tuple. |
| src/server/connection_manager.cpp | Modified client handling and request processing using the new interface. |
| src/server/client_info.cpp | Introduced the ClientInfo and FileSystemTree implementation. |
| include/server/request_manager.hpp | Revised the ClientHandler interface and added client_info dependency. |
| include/server/connection_manager.hpp | Updated to utilize IClientHandler instead of the old ClientHandler. |
| include/server/client_info.hpp | Added definitions for ClientInfo and FileSystemTree. |
Files not reviewed (1)
- src/server/CMakeLists.txt: Language not supported
| } | ||
| m_client_sockets.clear(); | ||
| } | ||
|
|
Copilot
AI
Apr 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding or updating documentation for connect_test_client() to clarify its new return type, including the success flag, for better clarity in test behavior.
| /** | |
| * Establishes a test client connection to the server. | |
| * | |
| * @return A pair containing: | |
| * - ClientInfo: Information about the connected client, including | |
| * its socket, encryption key, address, and port. | |
| * - bool: A success flag indicating whether the connection and | |
| * key exchange were successful (true) or not (false). | |
| */ |
9f69d0c to
9ded991
Compare
Signed-off-by: Praneeth Sarode <praneethsarode@gmail.com>
9ded991 to
e900a03
Compare
| fenris::Response handle_request(const fenris::Request &request, | ||
| ClientInfo &client_info); | ||
|
|
||
| void initialize_file_system_tree(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not defined anywhere. Please look into it.
|
|
||
| class ClientHandler : public IClientHandler { | ||
| public: | ||
| explicit ClientHandler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any need for declaring a custom constructor without a custom destructor? The constructor is not defined. Maybe you wanted to call initialize_file_system_tree inside the constructor.
|
closing this as already done in #64 |
No description provided.