Skip to content

Conversation

@jonpspri
Copy link
Contributor

Summary

Refactored two utility functions from config.py to their appropriate usage locations, improving module cohesion and reducing the scope of the configuration module.

Functions relocated:

  1. extract_using_jq: config.pyservices/tool_service.py

    • Only used for processing REST tool responses with jq filters
    • Now located directly in the service that uses it
  2. jsonpath_modifier: config.pymain.py

    • Used directly in API endpoints for dynamic JSONPath filtering
    • Now located in the main application module alongside its usage

Benefits

  • Improved module cohesion: Functions now reside near their usage points
  • Reduced config.py scope: Configuration module is now focused solely on settings management
  • Cleaner imports: Removed unused imports from config.py (jq, jsonpath_ng, FastAPI HTTPException)
  • Better type safety: Added proper type annotations to Settings.__init__ and Settings.log_summary

Changes

Code Movement

  • Moved extract_using_jq with all docstring examples and error handling
  • Moved jsonpath_modifier with complete implementation and docstrings
  • Updated all import statements across the codebase

Test Coverage

  • Migrated extract_using_jq tests from test_config.pytest_tool_service.py
  • Migrated jsonpath_modifier tests from test_config.pytest_main.py
  • Updated fuzz test imports in fuzz_jsonpath.py and test_jsonpath_fuzz.py
  • Updated mock patches in tool service tests to reflect new import paths

Additional Improvements

  • Re-enabled Black formatter in pre-commit config
  • Added type hints: __init__(self, **kwargs: Any) -> None
  • Added type hints: log_summary(self) -> None

Testing

All existing tests pass with updated imports:

  • Unit tests for both functions maintained
  • Fuzz tests updated and functional
  • Mock patches in integration tests updated

Test Plan

  • Run unit tests: pytest tests/unit/mcpgateway/test_config.py
  • Run unit tests: pytest tests/unit/mcpgateway/test_main.py
  • Run unit tests: pytest tests/unit/mcpgateway/services/test_tool_service.py
  • Run fuzz tests: pytest tests/fuzz/test_jsonpath_fuzz.py
  • Verify no import errors
  • Run full test suite

@Copilot Copilot AI review requested due to automatic review settings October 24, 2025 09:41
Copy link
Contributor

Copilot AI left a 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 two utility functions (extract_using_jq and jsonpath_modifier) from the config.py module to their actual usage locations, improving code organization and module cohesion. The functions are moved to services/tool_service.py and main.py respectively, where they are actually used.

Key Changes:

  • Relocated extract_using_jq from config.py to services/tool_service.py where it processes REST tool responses
  • Relocated jsonpath_modifier from config.py to main.py where it's used in API endpoints
  • Updated all import statements and test mocks across the codebase to reflect the new locations

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
mcpgateway/config.py Removed extract_using_jq and jsonpath_modifier functions, cleaned up unused imports, added type hints to __init__ and log_summary
mcpgateway/main.py Added jsonpath_modifier function with complete implementation and necessary imports
mcpgateway/services/tool_service.py Added extract_using_jq function with complete implementation and jq import
tests/unit/mcpgateway/test_config.py Removed tests for relocated functions
tests/unit/mcpgateway/test_main.py Added tests for jsonpath_modifier with new import
tests/unit/mcpgateway/services/test_tool_service.py Added tests for extract_using_jq and updated mock patches
tests/fuzz/test_jsonpath_fuzz.py Updated import path for jsonpath_modifier
tests/fuzz/fuzzers/fuzz_jsonpath.py Updated import path for jsonpath_modifier
.pre-commit-config.yaml Re-enabled Black formatter

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"""Test reading resource content."""
# Clear the resource cache to avoid stale/cached values
from mcpgateway import main as mcpgateway_main

Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The blank line at 695 appears to be an unnecessary addition that doesn't improve readability. Consider removing it to maintain consistency with the original code style.

Suggested change

Copilot uses AI. Check for mistakes.
@jonpspri jonpspri marked this pull request as draft October 24, 2025 15:23
@jonpspri jonpspri force-pushed the chore/jq-refactor branch 4 times, most recently from 4389a0c to 832bf71 Compare October 25, 2025 12:45
@jonpspri
Copy link
Contributor Author

Leaving this in draft until after #1331 is committed -- that should resolve the last failing test.

Signed-off-by: Jonathan Springer <jonpspri@gmail.com>

Additional linting improvements

Signed-off-by: Jonathan Springer <jonpspri@gmail.com>

Lots of cleanup of secrets in code

Signed-off-by: Jonathan Springer <jonpspri@gmail.com>

fix: Correct Flake8 docstring errors in config.py

- Remove type annotations from parameter docstrings (DAR103)
- Add missing Raises sections for ValueError exceptions (DAR401)
- Fix return type descriptions to match TypedDict types (DAR203)

Resolves all DAR (docstring argument/return) validation errors.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Jonathan Springer <jonpspri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant