Feat: Add custom detectors to the built in detector #51
+240
−9
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
Reviewer's GuideThis PR extends the existing built-in detector service by integrating a custom detectors plugin: it introduces a CustomDetectorRegistry that discovers and wraps user-supplied functions, registers it in the FastAPI lifecycle, adjusts request handling to only invoke explicitly requested detectors, and adds logging, error handling, supporting modules, and tests. Sequence diagram for content detection with custom detectorssequenceDiagram
participant Client
participant FastAPI
participant CustomDetectorRegistry
participant custom_func_wrapper
Client->>FastAPI: POST /api/v1/text/contents (with detector_params)
FastAPI->>CustomDetectorRegistry: handle_request(content, detector_params)
CustomDetectorRegistry->>custom_func_wrapper: custom_func_wrapper(func, func_name, content)
custom_func_wrapper-->>CustomDetectorRegistry: ContentAnalysisResponse or None
CustomDetectorRegistry-->>FastAPI: List[ContentAnalysisResponse]
FastAPI-->>Client: ContentsAnalysisResponse
Class diagram for CustomDetectorRegistry and custom detector integrationclassDiagram
class BaseDetectorRegistry
class CustomDetectorRegistry {
+registry: dict
+__init__()
+handle_request(content: str, detector_params: dict): List[ContentAnalysisResponse]
}
class ContentAnalysisResponse
class custom_func_wrapper {
+custom_func_wrapper(func: Callable, func_name: str, s: str): Optional[ContentAnalysisResponse]
}
BaseDetectorRegistry <|-- CustomDetectorRegistry
CustomDetectorRegistry o-- custom_func_wrapper
CustomDetectorRegistry o-- ContentAnalysisResponse
custom_func_wrapper o-- ContentAnalysisResponse
class "custom_detectors.custom_detectors" {
+over_100_characters(text: str): bool
+contains_word(text: str): bool
}
CustomDetectorRegistry o-- "custom_detectors.custom_detectors"
File-Level Changes
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}
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> `detectors/built_in/app.py:33` </location>
<code_context>
-
@app.post("/api/v1/text/contents", response_model=ContentsAnalysisResponse)
def detect_content(request: ContentAnalysisHttpRequest):
+ logging.info(f"Request for {request.detector_params}")
+
detections = []
</code_context>
<issue_to_address>
**suggestion:** Consider using the module-level logger instead of the root logger for consistency.
Use 'logger.info' instead of 'logging.info' to ensure consistent logging and easier configuration management.
```suggestion
logger.info(f"Request for {request.detector_params}")
```
</issue_to_address>
### Comment 2
<location> `detectors/built_in/custom_detectors_wrapper.py:54` </location>
<code_context>
+
+ def handle_request(self, content: str, detector_params: dict) -> List[ContentAnalysisResponse]:
+ detections = []
+ if "custom" in detector_params and isinstance(detector_params["custom"], (list, str)):
+ custom_functions = detector_params["custom"]
+ custom_functions = [custom_functions] if isinstance(custom_functions, str) else custom_functions
</code_context>
<issue_to_address>
**suggestion:** Type checking for 'custom' parameter may miss edge cases with unexpected types.
Unexpected types for 'custom' are ignored without notice. Logging or handling these cases would improve debuggability.
</issue_to_address>
### Comment 3
<location> `tests/detectors/builtIn/test_custom.py:14-22` </location>
<code_context>
+
+
+
+ def test_custom_detectors(self, client):
+ payload = {
+ "contents": ["What is an apple?"],
+ "detector_params": {"custom": ["contains_word"]}
+ }
+ resp = client.post("/api/v1/text/contents", json=payload)
+ assert resp.status_code == 200
+ texts = [d["text"] for d in resp.json()[0]]
+ assert "What is an apple?" in texts
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing negative and error case tests for custom detectors.
Add tests for scenarios where the custom detector does not match any content and for invalid detector names to ensure proper error handling and coverage of negative cases.
```suggestion
def test_custom_detectors(self, client):
payload = {
"contents": ["What is an apple?"],
"detector_params": {"custom": ["contains_word"]}
}
resp = client.post("/api/v1/text/contents", json=payload)
assert resp.status_code == 200
texts = [d["text"] for d in resp.json()[0]]
assert "What is an apple?" in texts
def test_custom_detector_no_match(self, client):
payload = {
"contents": ["Bananas are yellow."],
"detector_params": {"custom": ["contains_word"]}
}
resp = client.post("/api/v1/text/contents", json=payload)
assert resp.status_code == 200
texts = [d["text"] for d in resp.json()[0]]
assert "Bananas are yellow." not in texts
def test_custom_detector_invalid_name(self, client):
payload = {
"contents": ["What is an apple?"],
"detector_params": {"custom": ["non_existent_detector"]}
}
resp = client.post("/api/v1/text/contents", json=payload)
assert resp.status_code == 400 or resp.status_code == 422
# Optionally check for error message in response
# assert "invalid detector" in resp.text.lower()
```
</issue_to_address>
### Comment 4
<location> `tests/detectors/builtIn/test_custom.py:4` </location>
<code_context>
+import pytest
+from fastapi.testclient import TestClient
+
+class TestRegexDetectors:
+ @pytest.fixture
+ def client(self):
</code_context>
<issue_to_address>
**nitpick:** Test class name is misleading.
Since this class only tests custom detectors, renaming it to 'TestCustomDetectors' would improve clarity.
</issue_to_address>
### Comment 5
<location> `detectors/built_in/custom_detectors_wrapper.py:58-67` </location>
<code_context>
def handle_request(self, content: str, detector_params: dict) -> List[ContentAnalysisResponse]:
detections = []
if "custom" in detector_params and isinstance(detector_params["custom"], (list, str)):
custom_functions = detector_params["custom"]
custom_functions = [custom_functions] if isinstance(custom_functions, str) else custom_functions
for custom_function in custom_functions:
if self.registry.get(custom_function):
try:
result = custom_func_wrapper(self.registry[custom_function], custom_function, content)
if result is not None:
detections.append(result)
except Exception as e:
logger.error(e)
raise HTTPException(status_code=400, detail="Detection error, check detector logs")
else:
raise HTTPException(status_code=400, detail=f"Unrecognized custom function: {custom_function}")
return detections
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
- Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| def handle_request(self, content: str, detector_params: dict) -> List[ContentAnalysisResponse]: | ||
| detections = [] | ||
| if "custom" in detector_params and isinstance(detector_params["custom"], (list, str)): |
There was a problem hiding this comment.
suggestion: Type checking for 'custom' parameter may miss edge cases with unexpected types.
Unexpected types for 'custom' are ignored without notice. Logging or handling these cases would improve debuggability.
Comment on lines
+14
to
+70
| def test_custom_detectors(self, client): | ||
| payload = { | ||
| "contents": ["What is an apple?"], | ||
| "detector_params": {"custom": ["contains_word"]} | ||
| } | ||
| resp = client.post("/api/v1/text/contents", json=payload) | ||
| assert resp.status_code == 200 | ||
| texts = [d["text"] for d in resp.json()[0]] | ||
| assert "What is an apple?" in texts |
There was a problem hiding this comment.
suggestion (testing): Missing negative and error case tests for custom detectors.
Add tests for scenarios where the custom detector does not match any content and for invalid detector names to ensure proper error handling and coverage of negative cases.
Suggested change
| def test_custom_detectors(self, client): | |
| payload = { | |
| "contents": ["What is an apple?"], | |
| "detector_params": {"custom": ["contains_word"]} | |
| } | |
| resp = client.post("/api/v1/text/contents", json=payload) | |
| assert resp.status_code == 200 | |
| texts = [d["text"] for d in resp.json()[0]] | |
| assert "What is an apple?" in texts | |
| def test_custom_detectors(self, client): | |
| payload = { | |
| "contents": ["What is an apple?"], | |
| "detector_params": {"custom": ["contains_word"]} | |
| } | |
| resp = client.post("/api/v1/text/contents", json=payload) | |
| assert resp.status_code == 200 | |
| texts = [d["text"] for d in resp.json()[0]] | |
| assert "What is an apple?" in texts | |
| def test_custom_detector_no_match(self, client): | |
| payload = { | |
| "contents": ["Bananas are yellow."], | |
| "detector_params": {"custom": ["contains_word"]} | |
| } | |
| resp = client.post("/api/v1/text/contents", json=payload) | |
| assert resp.status_code == 200 | |
| texts = [d["text"] for d in resp.json()[0]] | |
| assert "Bananas are yellow." not in texts | |
| def test_custom_detector_invalid_name(self, client): | |
| payload = { | |
| "contents": ["What is an apple?"], | |
| "detector_params": {"custom": ["non_existent_detector"]} | |
| } | |
| resp = client.post("/api/v1/text/contents", json=payload) | |
| assert resp.status_code == 400 or resp.status_code == 422 | |
| # Optionally check for error message in response | |
| # assert "invalid detector" in resp.text.lower() |
saichandrapandraju
requested changes
Oct 7, 2025
There was a problem hiding this comment.
Thanks @RobGeada , overall LGTM with one comment and 1 nit (+1 for sourcery's logger instead of logging suggestion)
m-misiura
reviewed
Oct 7, 2025
m-misiura
reviewed
Oct 7, 2025
m-misiura
reviewed
Oct 7, 2025
RobGeada
force-pushed
the
CustomDetectors
branch
from
650065c to
a86f66a
Compare
m-misiura
approved these changes
Oct 8, 2025
saichandrapandraju
approved these changes
Oct 8, 2025
|
PR image build completed successfully! 📦 Huggingface PR image: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary by Sourcery
Add support for custom detectors by introducing a new CustomDetectorRegistry to load and invoke user-defined functions, update the API to register and process only the detectors requested with improved error handling and logging, and include example detector functions along with an integration test.
New Features:
Enhancements:
Tests:
Chores: