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.

Sign up for GitHub

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: add data config checker for property value types #2328

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+138 −59
Open

feat: add data config checker for property value types #2328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa8f2b7
feat: add data config checker for property value types
joaoandre Dec 13, 2024
70c47f1
feat: update unit tests
joaoandre-avaiga Dec 22, 2024
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
64 changes: 22 additions & 42 deletions taipy/core/config/checkers/_data_node_config_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# specific language governing permissions and limitations under the License.

from datetime import timedelta
from typing import Any, Callable, Dict, List, Tuple, cast
from typing import Callable, Dict, List, cast

from taipy.common.config._config import _Config
from taipy.common.config.checker._checkers._config_checker import _ConfigChecker
Expand All @@ -23,27 +23,6 @@


class _DataNodeConfigChecker(_ConfigChecker):
_PROPERTIES_TYPES: Dict[str, List[Tuple[Any, List[str]]]] = {
DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: [
(
Callable,
[
DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY,
DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY,
],
)
],
DataNodeConfig._STORAGE_TYPE_VALUE_SQL: [
(
Callable,
[
DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY,
DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY,
],
),
],
}

def __init__(self, config: _Config, collector: IssueCollector):
super().__init__(config, collector)

Expand All @@ -67,7 +46,7 @@ def _check(self) -> IssueCollector:
self._check_scope(data_node_config_id, data_node_config)
self._check_validity_period(data_node_config_id, data_node_config)
self._check_required_properties(data_node_config_id, data_node_config)
self._check_class_type(data_node_config_id, data_node_config)
self._check_property_types(data_node_config_id, data_node_config)
self._check_generic_read_write_fct_and_args(data_node_config_id, data_node_config)
self._check_exposed_type(data_node_config_id, data_node_config)
return self._collector
Expand Down Expand Up @@ -217,25 +196,26 @@ def _check_generic_read_write_fct_and_args(self, data_node_config_id: str, data_
f"DataNodeConfig `{data_node_config_id}` must be populated with a Callable function.",
)

def _check_class_type(self, data_node_config_id: str, data_node_config: DataNodeConfig):
if data_node_config.storage_type in self._PROPERTIES_TYPES.keys():
for class_type, prop_keys in self._PROPERTIES_TYPES[data_node_config.storage_type]:
for prop_key in prop_keys:
prop_value = data_node_config.properties.get(prop_key) if data_node_config.properties else None
if prop_value and not isinstance(prop_value, class_type):
self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a {'Callable' if class_type == Callable else class_type.__name__}.",
)
if class_type == Callable and callable(prop_value) and prop_value.__name__ == "<lambda>":
self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a serializable Callable function but not a lambda.",
)
def _check_property_types(self, data_node_config_id: str, data_node_config: DataNodeConfig):
Copy link
Member

Choose a reason for hiding this comment

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

can you help propagate this change on enterprise as well? I'm quite certain it will fail on enterprise :D thanks!

if property_types := data_node_config._PROPERTIES_TYPES.get(data_node_config.storage_type):
for prop_key, prop_type in property_types.items():
prop_value = data_node_config.properties.get(prop_key) if data_node_config.properties else None

if prop_value and not isinstance(prop_value, prop_type):
Copy link
Member

Choose a reason for hiding this comment

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

prop_type is the value in the _PROPERTIES_TYPES dictionary. Today, it must be a Python type.

Can we imagine prop_type being either a type or a list of types? Then, we should check that the type of prop_value is among the types in the prop_type list of types.

Does it make sense, or is it just over-engineering?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isinstance can take a tuple of type as the second parameter. So we only need to define the proprety as a tuple of expected types and it should be good to go. I had to do that for a couple of properties already.

self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a {prop_type}.",
)

if prop_type == Callable and callable(prop_value) and prop_value.__name__ == "<lambda>":
self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a serializable typing.Callable function but not a lambda.",
)

def _check_exposed_type(self, data_node_config_id: str, data_node_config: DataNodeConfig):
if not isinstance(data_node_config.exposed_type, str):
Expand Down
95 changes: 94 additions & 1 deletion taipy/core/config/data_node_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class DataNodeConfig(Section):
"""

name = "DATA_NODE"

_ALL_TYPES = (str, int, float, bool, list, dict, tuple, set, type(None), callable)
_STORAGE_TYPE_KEY = "storage_type"
_STORAGE_TYPE_VALUE_PICKLE = "pickle"
_STORAGE_TYPE_VALUE_SQL_TABLE = "sql_table"
Expand Down Expand Up @@ -150,6 +150,99 @@ class DataNodeConfig(Section):
_OPTIONAL_AWS_S3_GET_OBJECT_PARAMETERS_PROPERTY = "aws_s3_get_object_parameters"
_OPTIONAL_AWS_S3_PUT_OBJECT_PARAMETERS_PROPERTY = "aws_s3_put_object_parameters"

_PROPERTIES_TYPES: Dict[str, Dict[str, Any]] = {
_STORAGE_TYPE_VALUE_GENERIC: {
_OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY: Callable,
_OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY: Callable,
_OPTIONAL_READ_FUNCTION_ARGS_GENERIC_PROPERTY: list,
_OPTIONAL_WRITE_FUNCTION_ARGS_GENERIC_PROPERTY: list,
},
_STORAGE_TYPE_VALUE_SQL: {
_REQUIRED_DB_NAME_SQL_PROPERTY: str,
_REQUIRED_DB_ENGINE_SQL_PROPERTY: str,
_REQUIRED_READ_QUERY_SQL_PROPERTY: str,
_REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY: Callable,
_OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY: Callable,
_OPTIONAL_DB_USERNAME_SQL_PROPERTY: str,
_OPTIONAL_DB_PASSWORD_SQL_PROPERTY: str,
_OPTIONAL_HOST_SQL_PROPERTY: str,
_OPTIONAL_PORT_SQL_PROPERTY: int,
_OPTIONAL_DRIVER_SQL_PROPERTY: str,
_OPTIONAL_FOLDER_PATH_SQLITE_PROPERTY: str,
_OPTIONAL_FILE_EXTENSION_SQLITE_PROPERTY: str,
_OPTIONAL_DB_EXTRA_ARGS_SQL_PROPERTY: dict,
_OPTIONAL_EXPOSED_TYPE_SQL_PROPERTY: (str, Callable),
},
_STORAGE_TYPE_VALUE_SQL_TABLE: {
_REQUIRED_DB_NAME_SQL_PROPERTY: str,
_REQUIRED_DB_ENGINE_SQL_PROPERTY: str,
_REQUIRED_TABLE_NAME_SQL_TABLE_PROPERTY: str,
_OPTIONAL_DB_USERNAME_SQL_PROPERTY: str,
_OPTIONAL_DB_PASSWORD_SQL_PROPERTY: str,
_OPTIONAL_HOST_SQL_PROPERTY: str,
_OPTIONAL_PORT_SQL_PROPERTY: int,
_OPTIONAL_DRIVER_SQL_PROPERTY: str,
_OPTIONAL_FOLDER_PATH_SQLITE_PROPERTY: str,
_OPTIONAL_FILE_EXTENSION_SQLITE_PROPERTY: str,
_OPTIONAL_DB_EXTRA_ARGS_SQL_PROPERTY: dict,
_OPTIONAL_EXPOSED_TYPE_SQL_PROPERTY: (str, Callable),
},
_STORAGE_TYPE_VALUE_CSV: {
_OPTIONAL_DEFAULT_PATH_CSV_PROPERTY: str,
_OPTIONAL_ENCODING_PROPERTY: str,
_OPTIONAL_HAS_HEADER_CSV_PROPERTY: bool,
_OPTIONAL_EXPOSED_TYPE_CSV_PROPERTY: (str, Callable),
},
_STORAGE_TYPE_VALUE_EXCEL: {
_OPTIONAL_DEFAULT_PATH_EXCEL_PROPERTY: str,
_OPTIONAL_HAS_HEADER_EXCEL_PROPERTY: bool,
_OPTIONAL_SHEET_NAME_EXCEL_PROPERTY: (str, list),
_OPTIONAL_EXPOSED_TYPE_EXCEL_PROPERTY: (str, Callable),
},
_STORAGE_TYPE_VALUE_IN_MEMORY: {
_OPTIONAL_DEFAULT_DATA_IN_MEMORY_PROPERTY: _ALL_TYPES,
},
_STORAGE_TYPE_VALUE_PICKLE: {
_OPTIONAL_DEFAULT_PATH_PICKLE_PROPERTY: str,
_OPTIONAL_DEFAULT_DATA_PICKLE_PROPERTY: _ALL_TYPES,
},
_STORAGE_TYPE_VALUE_JSON: {
_OPTIONAL_DEFAULT_PATH_JSON_PROPERTY: str,
_OPTIONAL_ENCODING_PROPERTY: str,
_OPTIONAL_ENCODER_JSON_PROPERTY: json.JSONEncoder,
_OPTIONAL_DECODER_JSON_PROPERTY: json.JSONDecoder,
},
_STORAGE_TYPE_VALUE_PARQUET: {
_OPTIONAL_DEFAULT_PATH_PARQUET_PROPERTY: str,
_OPTIONAL_ENGINE_PARQUET_PROPERTY: str,
_OPTIONAL_COMPRESSION_PARQUET_PROPERTY: str,
_OPTIONAL_READ_KWARGS_PARQUET_PROPERTY: dict,
_OPTIONAL_WRITE_KWARGS_PARQUET_PROPERTY: dict,
_OPTIONAL_EXPOSED_TYPE_PARQUET_PROPERTY: (str, Callable),
},
_STORAGE_TYPE_VALUE_MONGO_COLLECTION: {
_REQUIRED_DB_NAME_MONGO_PROPERTY: str,
_REQUIRED_COLLECTION_NAME_MONGO_PROPERTY: str,
_OPTIONAL_CUSTOM_DOCUMENT_MONGO_PROPERTY: str,
_OPTIONAL_USERNAME_MONGO_PROPERTY: str,
_OPTIONAL_PASSWORD_MONGO_PROPERTY: str,
_OPTIONAL_HOST_MONGO_PROPERTY: str,
_OPTIONAL_PORT_MONGO_PROPERTY: int,
_OPTIONAL_DRIVER_MONGO_PROPERTY: str,
_OPTIONAL_DB_EXTRA_ARGS_MONGO_PROPERTY: dict,
},
_STORAGE_TYPE_VALUE_S3_OBJECT: {
_REQUIRED_AWS_ACCESS_KEY_ID_PROPERTY: str,
_REQUIRED_AWS_SECRET_ACCESS_KEY_PROPERTY: str,
_REQUIRED_AWS_STORAGE_BUCKET_NAME_PROPERTY: str,
_REQUIRED_AWS_S3_OBJECT_KEY_PROPERTY: str,
_OPTIONAL_AWS_REGION_PROPERTY: str,
_OPTIONAL_AWS_S3_CLIENT_PARAMETERS_PROPERTY: dict,
_OPTIONAL_AWS_S3_GET_OBJECT_PARAMETERS_PROPERTY: dict,
_OPTIONAL_AWS_S3_PUT_OBJECT_PARAMETERS_PROPERTY: dict,
},
}

_REQUIRED_PROPERTIES: Dict[str, List] = {
_STORAGE_TYPE_VALUE_PICKLE: [],
_STORAGE_TYPE_VALUE_SQL_TABLE: [
Expand Down
38 changes: 22 additions & 16 deletions tests/core/config/checkers/test_data_node_config_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,12 +513,12 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 2
expected_error_message_1 = (
"`write_query_builder` of DataNodeConfig `new` must be populated with a Callable."
"`write_query_builder` of DataNodeConfig `new` must be populated with a typing.Callable."
" Current value of property `write_query_builder` is 1."
)
assert expected_error_message_1 in caplog.text
expected_error_message_2 = (
"`append_query_builder` of DataNodeConfig `new` must be populated with a Callable."
"`append_query_builder` of DataNodeConfig `new` must be populated with a typing.Callable."
" Current value of property `append_query_builder` is 2."
)
assert expected_error_message_2 in caplog.text
Expand All @@ -530,7 +530,7 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 1
expected_error_messages = [
"`write_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
"`write_fct` of DataNodeConfig `new` must be populated with a typing.Callable. Current value"
" of property `write_fct` is 12.",
]
assert all(message in caplog.text for message in expected_error_messages)
Expand All @@ -542,7 +542,7 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 1
expected_error_messages = [
"`read_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
"`read_fct` of DataNodeConfig `new` must be populated with a typing.Callable. Current value"
" of property `read_fct` is 5.",
]
assert all(message in caplog.text for message in expected_error_messages)
Expand All @@ -554,9 +554,9 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 2
expected_error_messages = [
"`write_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
"`write_fct` of DataNodeConfig `new` must be populated with a typing.Callable. Current value"
" of property `write_fct` is 9.",
"`read_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
"`read_fct` of DataNodeConfig `new` must be populated with a typing.Callable. Current value"
" of property `read_fct` is 5.",
]
assert all(message in caplog.text for message in expected_error_messages)
Expand Down Expand Up @@ -588,10 +588,10 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 2
expected_error_messages = [
"`write_fct` of DataNodeConfig `new` must be populated with a serializable Callable function but"
"`write_fct` of DataNodeConfig `new` must be populated with a serializable typing.Callable function but"
" not a lambda. Current value of property `write_fct` is <function TestDataNodeConfigChecker."
"test_check_callable_properties.<locals>.<lambda>",
"`read_fct` of DataNodeConfig `new` must be populated with a serializable Callable function but"
"`read_fct` of DataNodeConfig `new` must be populated with a serializable typing.Callable function but"
" not a lambda. Current value of property `read_fct` is <function TestDataNodeConfigChecker."
"test_check_callable_properties.<locals>.<lambda>",
]
Expand All @@ -616,12 +616,15 @@ def test_check_read_write_fct_args(self, caplog):
with pytest.raises(SystemExit):
Config._collector = IssueCollector()
Config.check()
assert len(Config._collector.errors) == 1
expected_error_message = (
assert len(Config._collector.errors) == 2

expected_error_messages = (
"`write_fct_args` of DataNodeConfig `default` must be populated with a <class 'list'>."
' Current value of property `write_fct_args` is "foo".',
"`write_fct_args` field of DataNodeConfig `default` must be populated with a List value."
' Current value of property `write_fct_args` is "foo".'
' Current value of property `write_fct_args` is "foo".',
)
assert expected_error_message in caplog.text
assert all(message in caplog.text for message in expected_error_messages)
config._sections[DataNodeConfig.name]["default"].storage_type = "generic"
config._sections[DataNodeConfig.name]["default"].properties = {
"write_fct": print,
Expand All @@ -641,12 +644,15 @@ def test_check_read_write_fct_args(self, caplog):
with pytest.raises(SystemExit):
Config._collector = IssueCollector()
Config.check()
assert len(Config._collector.errors) == 1
expected_error_message = (
assert len(Config._collector.errors) == 2

expected_error_messages = (
"`read_fct_args` of DataNodeConfig `default` must be populated with a <class 'list'>."
" Current value of property `read_fct_args` is 1.",
"`read_fct_args` field of DataNodeConfig `default` must be populated with a List value."
" Current value of property `read_fct_args` is 1."
" Current value of property `read_fct_args` is 1.",
)
assert expected_error_message in caplog.text
assert all(message in caplog.text for message in expected_error_messages)

config._sections[DataNodeConfig.name]["default"].storage_type = "generic"
config._sections[DataNodeConfig.name]["default"].properties = {
Expand Down
Loading