Skip to content

MON-199535 Factorize Tools Implementation#231

Open
gregory-lvtx wants to merge 18 commits into
developfrom
MON-199535-factorize-tools-implementation
Open

MON-199535 Factorize Tools Implementation#231
gregory-lvtx wants to merge 18 commits into
developfrom
MON-199535-factorize-tools-implementation

Conversation

@gregory-lvtx
Copy link
Copy Markdown
Contributor

@gregory-lvtx gregory-lvtx commented Jun 2, 2026

Ticket Jira: MON-199535

What ?

Factorize tool implementations.

Why ?

Some components related to models inheriting from the same mixins have a common implementation of tools such as create, list, and delete. Defining generic CRUD tools will simplify the code by removing redundancy.

How ?

  • Define generic _create, _list, _delete, _patch methods in a new components.base.py.
  • Update each relevant component with these new generic methods.
  • Update unit tests to align with the new generic methods.

@gregory-lvtx gregory-lvtx requested a review from denrou June 2, 2026 14:22
Comment thread centreon_mcp/components/base.py Outdated
Copy link
Copy Markdown
Contributor

@denrou denrou left a comment

Choose a reason for hiding this comment

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

🔍 Automated Review — Action Required

Reviewed with the Centreon automated review skill. Complexity: medium–high — Recommended reviewers: 2. All 84 tests pass and CI is green; the refactor itself is clean and consistent. See inline comments for details.

🔴 Critical

  • Default sort order dropped for all list endpointscomponents/base.py. The new _list sends sort_by=None when no order is passed, where the old helper always applied each model's default sort field. This silently changes default result ordering (and pagination) for all 11 list tools. See inline comment.

🟠 Important

  • No test coverage for the new generic helpers — the component tests mock _list/_create/_delete/_patch, so components/base.py is never executed (regression for _delete's aggregation logic). See inline comment.
  • Dead code: the old generic _list in utils/base.py (~L37) is now unused after this refactor and can be removed.

🟡 Suggestion

  • Stale test comment in test_component_host.py. See inline comment.

⚠️ The Critical item is a silent behavior change — please confirm whether it's intentional before merge.

🤖 Generated with Claude Code

Comment thread centreon_mcp/components/base.py
return await model.list(search, limit, page, sort_by)


async def _delete[CentreonModel: DeleteMixin](
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These new generic helpers have no direct test coverage — every component test mocks _list/_create/_delete/_patch at the module level, so the real implementations here are never executed. This is a coverage regression for _delete in particular: the asyncio.gather(..., return_exceptions=True) + dict(zip(..., strict=True)) aggregation previously ran for real in each component delete test (only Model.delete was mocked). A dedicated test_component_base.py covering _delete success/exception paths and _list's sort_by would also catch the default-ordering issue above.

Comment thread tests/components/test_component_host.py Outdated
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.

2 participants