Skip to content
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

feat: Better error messages when config validation fails #768

Merged
merged 39 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
108a87c
feat: Validate unsupported config options
edgarrmondragon Jun 30, 2022
cd13367
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jun 30, 2022
9b59f61
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jul 1, 2022
d1c3fa8
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jul 6, 2022
07464d7
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 9, 2022
c05087e
Get rid of empty warnings list
edgarrmondragon Aug 9, 2022
03ca8e0
Test --discover should not raise error
edgarrmondragon Aug 9, 2022
ad77afa
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 23, 2022
90cc541
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 23, 2022
39af208
Tests cleanup
edgarrmondragon Aug 23, 2022
7e1d10a
Add target class init test
edgarrmondragon Aug 23, 2022
a05c61f
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 23, 2022
487ad8f
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 24, 2022
8b8e75c
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 24, 2022
f79e5bb
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 24, 2022
63b8e13
Add JSONTypeHelper.to_json wrapper
edgarrmondragon Aug 25, 2022
965dbb2
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Aug 31, 2022
7caa5c8
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Sep 14, 2022
f3ccdc3
Test mapper class configuration
edgarrmondragon Sep 14, 2022
a7af1e8
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Sep 14, 2022
b2f6366
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Sep 15, 2022
5c47e8d
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Feb 4, 2023
ee039fc
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Feb 16, 2023
03d1cae
Split the CLI tests
edgarrmondragon Feb 16, 2023
77cfca9
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Feb 22, 2023
79b7878
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Feb 24, 2023
968c6d2
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Mar 25, 2023
ef04031
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Apr 25, 2023
5b8d67c
Get rid of color
edgarrmondragon Apr 25, 2023
3fb6dfb
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jul 18, 2023
624a658
Address lint issues
edgarrmondragon Jul 18, 2023
2852dbe
Do not validate config in selection test
edgarrmondragon Jul 18, 2023
94a7cfa
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jul 19, 2023
5889975
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jul 19, 2023
8fb99fd
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Jul 19, 2023
e8443f8
Revert unnecessary change
edgarrmondragon Jul 20, 2023
05dc8d6
Merge branch 'main' into 753-validate-taptarget-config-options
edgarrmondragon Nov 17, 2023
96b8193
Address lint issues
edgarrmondragon Nov 17, 2023
93e29c0
Use logger instead of click.secho
edgarrmondragon Nov 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions singer_sdk/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,31 @@
"""Defines a common set of exceptions which developers can raise and/or catch."""

from __future__ import annotations

import requests


class ConfigValidationError(Exception):
"""Raised when a user's config settings fail validation."""

def __init__(
self,
message: str,
*,
errors: list[str] | None = None,
edgarrmondragon marked this conversation as resolved.
Show resolved Hide resolved
warnings: list[str] | None = None,
) -> None:
"""Initialize a ConfigValidationError.

Args:
message: A message describing the error.
errors: A list of errors which caused the validation error.
warnings: A list of warnings which caused the validation error.
"""
super().__init__(message)
self.errors = errors or []
self.warnings = warnings or []


class FatalAPIError(Exception):
"""Exception raised when a failed request should not be considered retriable."""
Expand Down
5 changes: 3 additions & 2 deletions singer_sdk/plugin_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def _validate_config(
f"JSONSchema was: {config_jsonschema}"
)
if raise_errors:
raise ConfigValidationError(summary)
raise ConfigValidationError(summary, errors=errors)

log_fn = self.logger.warning
else:
Expand All @@ -259,7 +259,8 @@ def _validate_config(
summary += f"\n{warning}"
if warnings_as_errors and raise_errors and warnings:
raise ConfigValidationError(
f"One or more warnings ocurred during validation: {warnings}"
f"One or more warnings ocurred during validation: {warnings}",
warnings=warnings,
)
log_fn(summary)
return warnings, errors
Expand Down
24 changes: 16 additions & 8 deletions singer_sdk/tap_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import click

from singer_sdk.cli import common_options
from singer_sdk.exceptions import MaxRecordsLimitException
from singer_sdk.exceptions import ConfigValidationError, MaxRecordsLimitException
from singer_sdk.helpers import _state
from singer_sdk.helpers._classproperty import classproperty
from singer_sdk.helpers._compat import final
Expand Down Expand Up @@ -453,6 +453,7 @@ def cli(

Raises:
FileNotFoundError: If the config file does not exist.
Abort: If the configuration is not valid.
"""
if version:
cls.print_version()
Expand Down Expand Up @@ -486,13 +487,20 @@ def cli(

config_files.append(Path(config_path))

tap = cls( # type: ignore # Ignore 'type not callable'
config=config_files or None,
state=state,
catalog=catalog,
parse_env_config=parse_env_config,
validate_config=validate_config,
)
try:
tap = cls( # type: ignore # Ignore 'type not callable'
config=config_files or None,
state=state,
catalog=catalog,
parse_env_config=parse_env_config,
validate_config=validate_config,
)
except ConfigValidationError as exc:
for error in exc.errors:
click.secho(error, fg="red")
for warning in exc.warnings:
click.secho(warning, fg="warning")
raise click.Abort("Configuration is not valid.")

if discover:
tap.run_discovery()
Expand Down
13 changes: 8 additions & 5 deletions singer_sdk/typing.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,14 +411,14 @@ class ObjectType(JSONTypeHelper):
def __init__(
self,
*properties: Property,
additional_properties: W | type[W] | None = None,
additional_properties: W | type[W] | bool | None = None,
) -> None:
"""Initialize ObjectType from its list of properties.

Args:
properties: Zero or more attributes for this JSON object.
additional_properties: A schema to match against unnamed properties in
this object.
this object or a boolean indicating if extra properties are allowed.
"""
self.wrapped: list[Property] = list(properties)
self.additional_properties = additional_properties
Expand All @@ -436,13 +436,16 @@ def type_dict(self) -> dict: # type: ignore # OK: @classproperty vs @property
merged_props.update(w.to_dict())
if not w.optional:
required.append(w.name)
result = {"type": "object", "properties": merged_props}
result: dict = {"type": "object", "properties": merged_props}

if required:
result["required"] = required

if self.additional_properties:
result["additionalProperties"] = self.additional_properties.type_dict
if self.additional_properties is not None:
if isinstance(self.additional_properties, bool):
result["additionalProperties"] = self.additional_properties
else:
result["additionalProperties"] = self.additional_properties.type_dict

return result

Expand Down
84 changes: 67 additions & 17 deletions tests/core/test_jsonschema_helpers.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
"""Test sample sync."""

from __future__ import annotations

import re
from typing import List

import pytest

Expand Down Expand Up @@ -47,7 +48,7 @@ class ConfigTestTap(Tap):
Property("batch_size", IntegerType, default=-1),
).to_dict()

def discover_streams(self) -> List[Stream]:
def discover_streams(self) -> list[Stream]:
return []


Expand Down Expand Up @@ -291,7 +292,7 @@ def test_array_type():


@pytest.mark.parametrize(
"properties,addtional_properties",
"properties,additional_properties",
[
(
[
Expand All @@ -311,6 +312,15 @@ def test_array_type():
],
StringType,
),
(
[
Property("id", StringType),
Property("email", StringType),
Property("username", StringType),
Property("phone_number", StringType),
],
False,
),
(
[
Property("id", StringType),
Expand All @@ -331,6 +341,16 @@ def test_array_type():
],
StringType,
),
(
[
Property("id", StringType),
Property("id", StringType),
Property("email", StringType),
Property("username", StringType),
Property("phone_number", StringType),
],
False,
),
(
[
Property("id", StringType),
Expand All @@ -349,6 +369,15 @@ def test_array_type():
],
StringType,
),
(
[
Property("id", StringType),
Property("email", StringType, True),
Property("username", StringType, True),
Property("phone_number", StringType),
],
False,
),
(
[
Property("id", StringType),
Expand All @@ -369,39 +398,60 @@ def test_array_type():
],
StringType,
),
(
[
Property("id", StringType),
Property("email", StringType, True),
Property("email", StringType, True),
Property("username", StringType, True),
Property("phone_number", StringType),
],
False,
),
],
ids=[
"no requried, no duplicates, no additional properties",
"no requried, no duplicates, additional properties",
"no requried, duplicates, no additional properties",
"no requried, duplicates, additional properties",
"requried, no duplicates, no additional properties",
"requried, no duplicates, additional properties",
"requried, duplicates, no additional properties",
"requried, duplicates, additional properties",
"no required, no duplicates, no additional properties",
"no required, no duplicates, additional properties",
"no required, no duplicates, no additional properties allowed",
"no required, duplicates, no additional properties",
"no required, duplicates, additional properties",
"no required, duplicates, no additional properties allowed",
"required, no duplicates, no additional properties",
"required, no duplicates, additional properties",
"required, no duplicates, no additional properties allowed",
"required, duplicates, no additional properties",
"required, duplicates, additional properties",
"required, duplicates, no additional properties allowed",
],
)
def test_object_type(properties: List[Property], addtional_properties: JSONTypeHelper):
def test_object_type(
properties: list[Property],
additional_properties: JSONTypeHelper | bool,
):
merged_property_schemas = {
name: schema for p in properties for name, schema in p.to_dict().items()
}

required = [p.name for p in properties if not p.optional]
required_schema = {"required": required} if required else {}
addtional_properties_schema = (
{"additionalProperties": addtional_properties.type_dict}
if addtional_properties
additional_properties_schema = (
{
"additionalProperties": additional_properties
if isinstance(additional_properties, bool)
else additional_properties.type_dict
}
if additional_properties is not None
else {}
)

expected_json_schema = {
"type": "object",
"properties": merged_property_schemas,
**required_schema,
**addtional_properties_schema,
**additional_properties_schema,
}

object_type = ObjectType(*properties, additional_properties=addtional_properties)
object_type = ObjectType(*properties, additional_properties=additional_properties)
assert object_type.type_dict == expected_json_schema


Expand Down
55 changes: 48 additions & 7 deletions tests/core/test_streams.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
"""Stream tests."""

from __future__ import annotations

import logging
from typing import Any, Dict, Iterable, List, Optional, cast
from typing import Any, Iterable, cast

import pendulum
import pytest
import requests

from singer_sdk.exceptions import ConfigValidationError
from singer_sdk.helpers._classproperty import classproperty
from singer_sdk.helpers.jsonpath import _compile_jsonpath
from singer_sdk.streams.core import (
Expand Down Expand Up @@ -41,7 +44,7 @@ def __init__(self, tap: Tap):
"""Create a new stream."""
super().__init__(tap, schema=self.schema, name=self.name)

def get_records(self, context: Optional[dict]) -> Iterable[Dict[str, Any]]:
def get_records(self, context: dict | None) -> Iterable[dict[str, Any]]:
"""Generate records."""
yield {"id": 1, "value": "Egypt"}
yield {"id": 2, "value": "Germany"}
Expand Down Expand Up @@ -78,9 +81,14 @@ class SimpleTestTap(Tap):
"""Test tap class."""

name = "test-tap"
settings_jsonschema = PropertiesList(Property("start_date", DateTimeType)).to_dict()
config_jsonschema = PropertiesList(
Property("username", StringType, required=True),
Property("password", StringType, required=True),
Property("start_date", DateTimeType),
additional_properties=False,
edgarrmondragon marked this conversation as resolved.
Show resolved Hide resolved
).to_dict()

def discover_streams(self) -> List[Stream]:
def discover_streams(self) -> list[Stream]:
"""List all streams."""
return [SimpleTestStream(self)]

Expand All @@ -101,7 +109,11 @@ def tap() -> SimpleTestTap:
]
}
return SimpleTestTap(
config={"start_date": "2021-01-01"},
config={
"username": "utest",
"password": "ptest",
"start_date": "2021-01-01",
},
parse_env_config=False,
catalog=catalog_dict,
)
Expand Down Expand Up @@ -214,7 +226,7 @@ def test_stream_starting_timestamp(tap: SimpleTestTap, stream: SimpleTestStream)
],
)
def test_jsonpath_rest_stream(
tap: SimpleTestTap, path: str, content: str, result: List[dict]
tap: SimpleTestTap, path: str, content: str, result: list[dict]
):
"""Validate records are extracted correctly from the API response."""
fake_response = requests.Response()
Expand Down Expand Up @@ -370,7 +382,7 @@ def test_sync_costs_calculation(tap: SimpleTestTap, caplog):
def calculate_test_cost(
request: requests.PreparedRequest,
response: requests.Response,
context: Optional[Dict],
context: dict | None,
):
return {"dim1": 1, "dim2": 2}

Expand All @@ -387,3 +399,32 @@ def calculate_test_cost(
for record in caplog.records:
assert record.levelname == "INFO"
assert f"Total Sync costs for stream {stream.name}" in record.message


@pytest.mark.parametrize(
"config_dict,errors",
[
(
{},
["'username' is a required property"],
),
(
{"username": "utest"},
["'password' is a required property"],
),
(
{"username": "utest", "password": "ptest", "extra": "not valid"},
["Additional properties are not allowed ('extra' was unexpected)"],
),
],
ids=[
"missing_username",
"missing_password",
"extra_property",
],
)
def test_config_errors(config_dict: dict, errors: list[str]):
with pytest.raises(ConfigValidationError, match="Config validation failed") as exc:
SimpleTestTap(config_dict, validate_config=True)

assert exc.value.errors == errors