report: align OpenAPI schemas with actual API responses and introduce DTO service contracts - #342
report: align OpenAPI schemas with actual API responses and introduce DTO service contracts#342ol-zayatsm wants to merge 3 commits into
Conversation
b4b7391 to
725b522
Compare
ol-zayatsm
changed the title
| response=ReportConfigListResponseSerializer, | ||
| description='Configurations were successfully retrieved', | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Missing 404 — self.get_object() can raise it if run_id doesn't exist.
| class ReportRetrieveResponseSerializer(serializers.Serializer): | ||
| warnings = serializers.ListField(child=serializers.CharField()) | ||
| config = ReportConfigSerializer() | ||
| content = serializers.ListField(child=serializers.DictField()) |
There was a problem hiding this comment.
content has a fully known 4-level structure (test → args-vals → measurement → record), each with fixed fields — DictField() loses all of it in the generated schema. Consider proper nested serializers for all four levels.
725b522 to
35826d7
Compare
| exclude=['type', 'is_active', 'user', 'content'], | ||
| RunReportConfigDTO( | ||
| id=report_config.id, | ||
| created=report_config.created, |
There was a problem hiding this comment.
created wasn't in the old response — model_to_dict() silently drops it, since auto_now_add=True makes it non-editable and non-editable fields are skipped regardless of exclude. This PR is meant to align the schema with the actual response, not change it, so please remove created from RunReportConfigDTO (and the serializer) to keep the response as it was.
| name: str | ||
| description: str | ||
| version: int | ||
| content: dict[str, Any] |
There was a problem hiding this comment.
nit: ReportConfigDTO doubles as both the response contract (only name/description/version are serialized) and the internal carrier of content used to build the report. Not a bug, but could be confusing later. If you want to keep the DTO pattern consistent with the rest of the PR, consider a small internal-only wrapper (e.g. ReportConfigContentDTO{config: ReportConfigDTO, content: dict}) instead of embedding content in the response DTO — up to you, not blocking.
35826d7 to
26a5fbf
Compare
eb3ac25 to
ffda007
Compare
|
|
||
| Returns: | ||
| Tuple of (config_obj, config_data, config_content) | ||
| ReportConfigDTO |
There was a problem hiding this comment.
nit: docstring says Returns: ReportConfigDTO, but the method now returns ReportConfigContentDTO (signature says so too). Please update the docstring to match.
| """ | ||
| try: | ||
| report_config_obj = Config.objects.get(id=config_id) | ||
| except ObjectDoesNotExist as e: |
There was a problem hiding this comment.
Non-numeric config (e.g. ?config=abc) raises a plain ValueError here, which isn't caught — it bubbles up as a 500 instead of 400. Please catch ValueError alongside ObjectDoesNotExist.
This is a behavior fix (500 → 400), unrelated to the DTO/schema refactor — please put it in its own commit before the introduce DTO-based service contracts commit, not mixed into it.
792ddde to
3876a39
Compare
| from __future__ import annotations | ||
|
|
||
| from django.core.exceptions import ObjectDoesNotExist | ||
| from django.core.exceptions import ObjectDoesNotExist, ValidationError |
There was a problem hiding this comment.
ValidationError here is django.core.exceptions.ValidationError, not DRF's. It's not an APIException, so DRF's exception_handler returns None for it and this still falls through to a 500 (verified by running it through custom_exception_handler directly — status 500). Use from rest_framework.exceptions import ValidationError instead — same as views.py already does for the "config not passed" case.
3876a39 to
d0e4d7e
Compare
| ReportConfigContentDTO | ||
|
|
||
| Raises: | ||
| NotFoundError: if config not found |
There was a problem hiding this comment.
Raises: is missing ValidationError, which the method now also raises for an invalid config ID or content.
| response=ReportConfigListResponseSerializer, | ||
| description='Configurations were successfully retrieved', | ||
| ), | ||
| 400: OpenApiResponse( |
There was a problem hiding this comment.
configs documents 400 for "Current run does not exist", but self.get_object() raises Http404 → DRF's default exception_handler converts it to exceptions.NotFound, i.e. actual status is 404 — same as retrieve correctly documents for the same situation. Please change this to 404 to match reality.
Values of the `config` query parameter that aren't valid integers (e.g. `?config=abc` or `?config=1.5`) raised an unhandled `ValueError` when used in the database lookup, surfacing as an opaque 500 response instead of telling the client what was wrong. Catch `ValueError` alongside `ObjectDoesNotExist` in `get_report_config` and raise a `ValidationError` with a descriptive message, so an invalid config ID returns a clear 400 error. Signed-off-by: Mikhail Zayats <mikhail.zayats@oktetlabs.ru>
Replace dict-based service responses with typed DTOs to introduce explicit and structured data contracts across service boundaries and move serialization to API boundary to improve separation of concerns between service layer and API layers. Signed-off-by: Mikhail Zayats <mikhail.zayats@oktetlabs.ru>
Ensure consistency between OpenAPI schemas and API responses by introducing explicit request/response serializers and binding them via drf-spectacular. Signed-off-by: Mikhail Zayats <mikhail.zayats@oktetlabs.ru>
d0e4d7e to
0539514
Compare
2 participants
Description
Summary
Update API v2 OpenAPI schema definitions for report endpoints and introduce
DTOs for report service responses to keep API documentation and service-layer
contracts aligned with current response payloads.
Changes
GET /report/{run_id}/configsendpoint.GET /report/{run_id}endpoint.configquery parameter for report generation.unprocessed iterations.
iterations.
200) and error (400,404) responses.