[DAPS-1724] - feature python client support task and direct response allocation create #1751
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshuaSBrown
added
Component: Python API
Contributor
Reviewer's GuideThis PR extends the mock core server to handle password/token authentication and repo creation messages, integrates the mock core build into CMake and GitLab CI, adds Docker support, and introduces Python client integration tests for MessageLib and CommandLib. Class diagram for updated mock core server authentication and repo creationclassDiagram
class MockCoreServer {
+handlePasswordAuth()
+handleTokenAuth()
+handleRepoCreate()
}
class AuthenticationManager {
+verifyPassword()
+verifyToken()
}
class ClientWorker {
+processMessage()
+processRepoCreate()
}
MockCoreServer --> AuthenticationManager : uses
MockCoreServer --> ClientWorker : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `python/datafed_pkg/tests/integration/test_MessageLib.py:60-69` </location>
<code_context>
+class TestConnectionEstablishment:
</code_context>
<issue_to_address>
**suggestion (testing):** Missing tests for connection failures due to network issues.
Add tests simulating scenarios like unreachable server or blocked port to verify proper error handling and messaging.
</issue_to_address>
### Comment 2
<location> `python/datafed_pkg/tests/integration/test_MessageLib.py:188-197` </location>
<code_context>
+ def test_manual_auth_by_password_success(self, temp_key_files):
</code_context>
<issue_to_address>
**suggestion (testing):** No test for authentication with empty or None credentials.
Add tests for manual authentication with empty or None values for username, password, and token to verify proper handling of these cases.
Suggested implementation:
```python
def test_manual_auth_by_password_success(self, temp_key_files):
"""Test successful manual authentication by password."""
print(f"Test: Server public key: {temp_key_files['server_key']}")
print(f"Test: Client public key: {Defaults.mock_python_client_public_key}")
print(f"Test: Client private key: {Defaults.mock_python_client_private_key}")
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
def test_manual_auth_by_password_empty_username(self, temp_key_files):
"""Test manual authentication with empty username."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password("", "valid_password")
def test_manual_auth_by_password_empty_password(self, temp_key_files):
"""Test manual authentication with empty password."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password("valid_username", "")
def test_manual_auth_by_password_none_username(self, temp_key_files):
"""Test manual authentication with None username."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password(None, "valid_password")
def test_manual_auth_by_password_none_password(self, temp_key_files):
"""Test manual authentication with None password."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password("valid_username", None)
def test_manual_auth_by_token_empty_token(self, temp_key_files):
"""Test manual authentication with empty token."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_token("")
def test_manual_auth_by_token_none_token(self, temp_key_files):
"""Test manual authentication with None token."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_token(None)
```
- If `authenticate_by_password` or `authenticate_by_token` do not raise exceptions for invalid input, you may need to adjust the assertion to check for a failed authentication result instead.
- Ensure `pytest` is imported at the top of the file if not already present: `import pytest`
- If your API uses a different error handling mechanism, adapt the test to match the expected behavior (e.g., check for return value or error code).
</issue_to_address>
### Comment 3
<location> `python/datafed_pkg/tests/integration/test_MessageLib.py:264-273` </location>
<code_context>
+ server_pub_key=temp_key_files['server_key'],
+ )
+
+ def test_send_recv_synchronous(self, api_client):
+ """Test synchronous send/receive."""
+ # Create a test request (using GetAuthStatusRequest as example)
+ request = anon.GetAuthStatusRequest()
+
+ # Send and receive
+ reply, msg_type = api_client.sendRecv(request)
+
+ assert reply is not None
+ assert msg_type is not None
+
+ def test_send_recv_with_custom_timeout(self, api_client):
+ """Test send/receive with custom timeout."""
+ request = anon.GetAuthStatusRequest()
</code_context>
<issue_to_address>
**suggestion (testing):** No test for message sending with invalid or malformed requests.
Add a test that sends a malformed or invalid request to verify the client properly handles errors.
</issue_to_address>
### Comment 4
<location> `python/datafed_pkg/tests/integration/test_MessageLib.py:252-261` </location>
<code_context>
+class TestMessaging:
</code_context>
<issue_to_address>
**suggestion (testing):** No test for timeout and retry logic in message sending.
Please add tests to cover scenarios where the server does not respond, ensuring timeout and retry logic are properly handled.
Suggested implementation:
```python
class TestMessaging:
"""Test message sending and receiving functionality."""
@pytest.fixture
def api_client(self, temp_key_files):
"""Create an API client for testing."""
return MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
)
def test_message_send_timeout(self, api_client, mocker):
"""Test that message sending times out when server does not respond."""
# Mock the internal send method to simulate no response (timeout)
mocker.patch.object(api_client, "_send_message", side_effect=TimeoutError("Server did not respond"))
with pytest.raises(TimeoutError, match="Server did not respond"):
api_client.send_message("test_topic", "test_payload")
def test_message_send_retry_logic(self, api_client, mocker):
"""Test that message sending retries on timeout and eventually fails after max retries."""
# Simulate timeout for each retry attempt
max_retries = 3
mocker.patch.object(api_client, "_send_message", side_effect=TimeoutError("Server did not respond"))
mocker.patch.object(api_client, "max_retries", max_retries)
with pytest.raises(TimeoutError, match="Server did not respond"):
api_client.send_message("test_topic", "test_payload")
# Optionally, check that _send_message was called max_retries + 1 times (initial + retries)
assert api_client._send_message.call_count == max_retries + 1
```
- Ensure that `api_client.send_message` and the internal `_send_message` method exist and implement timeout and retry logic. If not, you will need to add or adjust these methods in the MessageLib.API class.
- The `max_retries` attribute should be present in the API client. If not, add it or adjust the test accordingly.
- The `mocker` fixture is provided by pytest-mock. Make sure pytest-mock is installed and available in your test environment.
- Adjust the topic/payload and error types to match your actual implementation if they differ.
</issue_to_address>
### Comment 5
<location> `python/datafed_pkg/tests/integration/test_CommandLib.py:77-86` </location>
<code_context>
+class TestCommandLibRepo:
</code_context>
<issue_to_address>
**suggestion (testing):** No negative tests for repoCreate (e.g., missing required fields, invalid values).
Add tests for repoCreate to cover cases with missing or invalid fields to verify proper input validation and error handling.
</issue_to_address>
### Comment 6
<location> `python/datafed_pkg/tests/integration/test_CommandLib.py:114-116` </location>
<code_context>
+ assert repo.address == repo_create_options['address']
+ assert repo.endpoint == repo_create_options['endpoint']
+ assert repo.admin == repo_create_options['admins']
+ assert repo.type == repo_create_options['type']
+
+if __name__ == "__main__":
+ pytest.main([__file__, "-v"])
</code_context>
<issue_to_address>
**suggestion (testing):** No test for repoCreate with duplicate repo_id.
Add a test case for creating a repository with a duplicate ID and verify that the API responds correctly.
</issue_to_address>
### Comment 7
<location> `doc_source/source/dev/testing.rst:86-87` </location>
<code_context>
+=========================================
+
+The python client integration tests can be run by running a mock core service
+in either directly on the host, or by building it in an container and running
+the container.
+
</code_context>
<issue_to_address>
**issue (typo):** Change 'in an container' to 'in a container' for correct grammar.
Use 'in a container' for proper grammar.
```suggestion
in either directly on the host, or by building it in a container and running
the container.
```
</issue_to_address>
### Comment 8
<location> `doc_source/source/dev/testing.rst:90-91` </location>
<code_context>
+the container.
+
+If using CMake and Ctest to run the tests it will build the mock core service
+and run it outside of a container. The guide below demonstrates the
+alternative or running the mock core service in a container.
+
+1. Build and Run python client against Mock Core Server
</code_context>
<issue_to_address>
**issue (typo):** Change 'alternative or running' to 'alternative of running' for correct phrasing.
Use 'alternative of running' for correct grammar.
```suggestion
and run it outside of a container. The guide below demonstrates the
alternative of running the mock core service in a container.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
188
to
196
| def test_manual_auth_by_password_success(self, temp_key_files): | ||
| """Test successful manual authentication by password.""" | ||
| print(f"Test: Server public key: {temp_key_files['server_key']}") | ||
| print(f"Test: Client public key: {Defaults.mock_python_client_public_key}") | ||
| print(f"Test: Client private key: {Defaults.mock_python_client_private_key}") | ||
| api = MessageLib.API( | ||
| server_host=Defaults.mock_core_server_host, | ||
| server_port=Defaults.mock_core_server_port, | ||
| server_pub_key=temp_key_files['server_key'], | ||
| manual_auth=True, |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test for authentication with empty or None credentials.
Add tests for manual authentication with empty or None values for username, password, and token to verify proper handling of these cases.
Suggested implementation:
def test_manual_auth_by_password_success(self, temp_key_files):
"""Test successful manual authentication by password."""
print(f"Test: Server public key: {temp_key_files['server_key']}")
print(f"Test: Client public key: {Defaults.mock_python_client_public_key}")
print(f"Test: Client private key: {Defaults.mock_python_client_private_key}")
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
def test_manual_auth_by_password_empty_username(self, temp_key_files):
"""Test manual authentication with empty username."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password("", "valid_password")
def test_manual_auth_by_password_empty_password(self, temp_key_files):
"""Test manual authentication with empty password."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password("valid_username", "")
def test_manual_auth_by_password_none_username(self, temp_key_files):
"""Test manual authentication with None username."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password(None, "valid_password")
def test_manual_auth_by_password_none_password(self, temp_key_files):
"""Test manual authentication with None password."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_password("valid_username", None)
def test_manual_auth_by_token_empty_token(self, temp_key_files):
"""Test manual authentication with empty token."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_token("")
def test_manual_auth_by_token_none_token(self, temp_key_files):
"""Test manual authentication with None token."""
api = MessageLib.API(
server_host=Defaults.mock_core_server_host,
server_port=Defaults.mock_core_server_port,
server_pub_key=temp_key_files['server_key'],
manual_auth=True,
client_pub_key=Defaults.mock_python_client_public_key,
client_priv_key=Defaults.mock_python_client_private_key,
)
with pytest.raises(Exception):
api.authenticate_by_token(None)- If
authenticate_by_passwordorauthenticate_by_tokendo not raise exceptions for invalid input, you may need to adjust the assertion to check for a failed authentication result instead. - Ensure
pytestis imported at the top of the file if not already present:import pytest - If your API uses a different error handling mechanism, adapt the test to match the expected behavior (e.g., check for return value or error code).
Comment on lines
77
to
84
| class TestCommandLibRepo: | ||
| """Test various connection establishment scenarios.""" | ||
|
|
||
| def test_successful_connection_with_key_files(self, command_lib_options, repo_create_options): | ||
| """Test successful connection using key files.""" | ||
| api = CommandLib.API(command_lib_options) | ||
|
|
||
| api.loginByPassword(Defaults.mock_authenticated_test_user, Defaults.test_user_password) | ||
|
|
||
| result = api.repoCreate( |
Contributor
There was a problem hiding this comment.
suggestion (testing): No negative tests for repoCreate (e.g., missing required fields, invalid values).
Add tests for repoCreate to cover cases with missing or invalid fields to verify proper input validation and error handling.
Comment on lines
114
to
116
| assert repo.type == repo_create_options['type'] | ||
|
|
||
| if __name__ == "__main__": |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test for repoCreate with duplicate repo_id.
Add a test case for creating a repository with a duplicate ID and verify that the API responds correctly.
Comment on lines
+86
to
+87
| in either directly on the host, or by building it in an container and running | ||
| the container. |
Contributor
There was a problem hiding this comment.
issue (typo): Change 'in an container' to 'in a container' for correct grammar.
Use 'in a container' for proper grammar.
Suggested change
| in either directly on the host, or by building it in an container and running | |
| the container. | |
| in either directly on the host, or by building it in a container and running | |
| the container. |
Comment on lines
+90
to
+91
| and run it outside of a container. The guide below demonstrates the | ||
| alternative or running the mock core service in a container. |
Contributor
There was a problem hiding this comment.
issue (typo): Change 'alternative or running' to 'alternative of running' for correct phrasing.
Use 'alternative of running' for correct grammar.
Suggested change
| and run it outside of a container. The guide below demonstrates the | |
| alternative or running the mock core service in a container. | |
| and run it outside of a container. The guide below demonstrates the | |
| alternative of running the mock core service in a container. |
nedvedba
approved these changes
Nov 11, 2025
Comment on lines
+36
to
+44
| COPY ./common ${BUILD_DIR}/common | ||
| COPY ./CMakeLists.txt ${BUILD_DIR} | ||
| COPY ./scripts/generate_datafed.sh ${BUILD_DIR}/scripts/ | ||
| COPY ./scripts/install_mock_core.sh ${BUILD_DIR}/scripts/ | ||
| COPY ./cmake ${BUILD_DIR}/cmake | ||
| COPY ./tests/mock_core/docker/entrypoint.sh ${BUILD_DIR}/tests/mock_core/docker/ | ||
| COPY ./tests/mock ${BUILD_DIR}/tests/mock | ||
| COPY ./tests/mock_core/source ${BUILD_DIR}/tests/mock_core/source/ | ||
| COPY ./tests/CMakeLists.txt ${BUILD_DIR}/tests/CMakeLists.txt |
Collaborator
There was a problem hiding this comment.
I think these could be reordered to be more optimal but since no operations are happening in between I don't think it's necessary.
Collaborator
Author
There was a problem hiding this comment.
Thanks for the suggestion!
JoshuaSBrown
changed the title
… separate folder and create docker folder and files.
…ker core server working.
JoshuaSBrown
force-pushed
the
1724-DAPS-feature-python-client-support-task-and-direct-response-allocation-create
branch
from
0569ffb to
2d137bc
Compare
JoshuaSBrown
added this to the
Objective - Enable Metadata Only Mode in DataFed milestone
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#1724
Description
How Has This Been Tested?
Artifacts (if appropriate):
Tasks
Summary by Sourcery
Add end-to-end support for authentication and repository creation in the mock core server and Python client, integrate mock core server into the build and CI pipelines, and add Python integration tests.
New Features:
Enhancements:
Build:
CI:
Tests:
Chores: