-
Notifications
You must be signed in to change notification settings - Fork 3
Feat: Add Configurable Discovery Validation Capabilities #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: typed-safe-data-preview
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,12 @@ | ||||||||||
| import logging | ||||||||||
| import uuid | ||||||||||
| from typing import Optional | ||||||||||
|
|
||||||||||
| from datamasque.client.base import BaseClient | ||||||||||
| from datamasque.client.exceptions import DataMasqueApiError | ||||||||||
| from datamasque.client.models.discovery_config import DiscoveryConfigType | ||||||||||
| from datamasque.client.models.discovery_config_library import DiscoveryConfigLibrary, DiscoveryConfigLibraryId | ||||||||||
| from datamasque.client.models.status import ValidationStatus | ||||||||||
|
|
||||||||||
| logger = logging.getLogger(__name__) | ||||||||||
|
|
||||||||||
|
|
@@ -95,6 +97,7 @@ def create_discovery_config_library(self, library: DiscoveryConfigLibrary) -> Di | |||||||||
| library.id = created.id | ||||||||||
| library.is_valid = created.is_valid | ||||||||||
| library.validation_error = created.validation_error | ||||||||||
| library.validation_error_details = created.validation_error_details | ||||||||||
| library.created = created.created | ||||||||||
| library.modified = created.modified | ||||||||||
| logger.info('Creation of discovery config library "%s" successful', library.name) | ||||||||||
|
|
@@ -123,6 +126,7 @@ def update_discovery_config_library(self, library: DiscoveryConfigLibrary) -> Di | |||||||||
| updated = DiscoveryConfigLibrary.model_validate(response.json()) | ||||||||||
| library.is_valid = updated.is_valid | ||||||||||
| library.validation_error = updated.validation_error | ||||||||||
| library.validation_error_details = updated.validation_error_details | ||||||||||
| library.modified = updated.modified | ||||||||||
| logger.debug('Update of discovery config library "%s" successful', library.name) | ||||||||||
| return library | ||||||||||
|
|
@@ -141,6 +145,45 @@ def create_or_update_discovery_config_library(self, library: DiscoveryConfigLibr | |||||||||
|
|
||||||||||
| return self.create_discovery_config_library(library) | ||||||||||
|
|
||||||||||
| def validate_discovery_config_library( | ||||||||||
| self, | ||||||||||
| library: DiscoveryConfigLibrary, | ||||||||||
| *, | ||||||||||
| timeout: float = 60.0, | ||||||||||
| poll_interval: float = 1.0, | ||||||||||
|
Comment on lines
+152
to
+153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Magic numbers: Can we make these named constants with units in
Suggested change
|
||||||||||
| ) -> DiscoveryConfigLibrary: | ||||||||||
| """Validate a discovery config library's YAML server-side and return it with the verdict populated.""" | ||||||||||
|
|
||||||||||
| temp = DiscoveryConfigLibrary( | ||||||||||
| name=f"__dm_validate_{uuid.uuid4().hex}", | ||||||||||
| namespace=library.namespace, | ||||||||||
| yaml=library.yaml, | ||||||||||
| config_type=library.config_type, | ||||||||||
| ) | ||||||||||
| created = self.create_discovery_config_library(temp) | ||||||||||
| settled = created | ||||||||||
| temp_id = created.id | ||||||||||
| if temp_id is not None: | ||||||||||
| library_id = temp_id | ||||||||||
| try: | ||||||||||
| settled = self._poll_until_done( | ||||||||||
| lambda: self.get_discovery_config_library(library_id), | ||||||||||
| lambda lib: lib.is_valid is not ValidationStatus.in_progress, | ||||||||||
| created, | ||||||||||
| timeout=timeout, | ||||||||||
| poll_interval=poll_interval, | ||||||||||
| ) | ||||||||||
| finally: | ||||||||||
| self._delete_best_effort( | ||||||||||
| lambda: self.delete_discovery_config_library_by_id_if_exists(library_id), | ||||||||||
| f"discovery config library `{library_id}`", | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| library.is_valid = settled.is_valid | ||||||||||
| library.validation_error = settled.validation_error | ||||||||||
| library.validation_error_details = settled.validation_error_details | ||||||||||
| return library | ||||||||||
|
|
||||||||||
| def delete_discovery_config_library_by_id_if_exists( | ||||||||||
| self, library_id: DiscoveryConfigLibraryId, *, force: bool = False | ||||||||||
| ) -> None: | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,9 +2,9 @@ | |||||
| from datetime import datetime | ||||||
| from typing import Any, NewType, Optional | ||||||
|
|
||||||
| from pydantic import BaseModel, ConfigDict, Field | ||||||
| from pydantic import BaseModel, ConfigDict, Field, model_validator | ||||||
|
|
||||||
| from datamasque.client.models.status import ValidationStatus | ||||||
| from datamasque.client.models.status import ValidationErrorDetails, ValidationStatus, promote_error_locations | ||||||
|
|
||||||
| DiscoveryConfigId = NewType("DiscoveryConfigId", str) | ||||||
|
|
||||||
|
|
@@ -49,5 +49,14 @@ class DiscoveryConfig(BaseModel): | |||||
| """Validation status; may be `in_progress` briefly after creating a large config.""" | ||||||
| validation_error: Optional[str] = Field(default=None, exclude=True) | ||||||
| """Human-readable validation error, or `None` when valid.""" | ||||||
| validation_error_details: list[ValidationErrorDetails] = Field(default_factory=list, exclude=True) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Although I prefer
Suggested change
|
||||||
| """Structured, positional validation errors.""" | ||||||
| created: Optional[datetime] = Field(default=None, exclude=True) | ||||||
| modified: Optional[datetime] = Field(default=None, exclude=True) | ||||||
|
|
||||||
| @model_validator(mode="before") | ||||||
| @classmethod | ||||||
| def _promote_error_locations(cls, data: object) -> object: | ||||||
| """Flatten the server's `errors` payload into `validation_error_details`.""" | ||||||
|
|
||||||
| return promote_error_locations(data) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,25 @@ class ValidationErrorDetails(BaseModel): | |
| column_number: Optional[int] = None | ||
|
|
||
|
|
||
| def promote_error_locations(data: object) -> object: | ||
| """Flatten a server `errors` payload into a `validation_error_details` list.""" | ||
|
|
||
| if not isinstance(data, dict) or "errors" not in data: | ||
| return data | ||
| data = dict(data) | ||
| raw_errors = data.pop("errors") | ||
| entries: list[object] = [] | ||
| if isinstance(raw_errors, dict): | ||
| for value in raw_errors.values(): | ||
| if isinstance(value, list): | ||
| entries.extend(value) | ||
| elif isinstance(raw_errors, list): | ||
| entries = raw_errors | ||
| if entries: | ||
| data["validation_error_details"] = entries | ||
| return data | ||
|
Comment on lines
+34
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we declare the shape when defining validation_error_details: list[ValidationErrorDetails] = Field(
default_factory=list,
exclude=True,
validation_alias=AliasChoices(AliasPath("errors", "config_yaml"), "validation_error_details"),
)(docs to pydantic aliasing) - Does this work? |
||
|
|
||
|
|
||
| class MaskingRunStatus(enum.Enum): | ||
| """List of valid masking run statuses.""" | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning drops the underlying error, so finding the cause of it may be guesswork after it happens.
Suggestion: catch as
eand include it in the message