-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: add rest config and rest config checker #2432
base: develop
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 |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Copyright 2021-2025 Avaiga Private Limited | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
# the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
# an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations under the License. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
# Copyright 2021-2025 Avaiga Private Limited | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
# the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
# an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations under the License. | ||
|
||
from typing import cast | ||
|
||
from taipy.common.config._config import _Config | ||
from taipy.common.config.checker._checkers._config_checker import _ConfigChecker | ||
from taipy.common.config.checker.issue_collector import IssueCollector | ||
|
||
from .rest_config import RestConfig | ||
|
||
|
||
class _RestConfigChecker(_ConfigChecker): | ||
def __init__(self, config: _Config, collector: IssueCollector): | ||
super().__init__(config, collector) | ||
|
||
def _check(self) -> IssueCollector: | ||
rest_configs = cast(dict, self._config._sections.get(RestConfig.name, {})) | ||
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. If the RestConfig is a unique section, this line should be simpler. |
||
|
||
for rest_config_id, rest_config in rest_configs.items(): | ||
if rest_config_id != _Config.DEFAULT_KEY: | ||
self._check_port(rest_config_id, rest_config) | ||
self._check_host(rest_config_id, rest_config) | ||
self._check_https_settings(rest_config_id, rest_config) | ||
|
||
return self._collector | ||
|
||
def _check_port(self, rest_config_id: str, rest_config: RestConfig): | ||
if not isinstance(rest_config.port, int) or not (1 <= rest_config.port <= 65535): | ||
self._error( | ||
"port", | ||
rest_config.port, | ||
f"The port of RestConfig `{rest_config_id}` must be an integer between 1 and 65535.", | ||
) | ||
|
||
def _check_host(self, rest_config_id: str, rest_config: RestConfig): | ||
if not isinstance(rest_config.host, str) or not rest_config.host: | ||
self._error( | ||
"host", rest_config.host, f"The host of RestConfig `{rest_config_id}` must be a non-empty string." | ||
) | ||
|
||
def _check_https_settings(self, rest_config_id: str, rest_config: RestConfig): | ||
if rest_config.use_https: | ||
if not rest_config.ssl_cert or not rest_config.ssl_key: | ||
self._error( | ||
"ssl_cert/ssl_key", | ||
(rest_config.ssl_cert, rest_config.ssl_key), | ||
f"When HTTPS is enabled in RestConfig `{rest_config_id}`, both ssl_cert and ssl_key must be set.", | ||
) | ||
elif not isinstance(rest_config.ssl_cert, str) or not isinstance(rest_config.ssl_key, str): | ||
self._error( | ||
"ssl_cert/ssl_key", | ||
(rest_config.ssl_cert, rest_config.ssl_key), | ||
f"The ssl_cert and ssl_key of RestConfig `{rest_config_id}` must be valid strings.", | ||
) |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,116 @@ | ||||||||||
# Copyright 2021-2025 Avaiga Private Limited | ||||||||||
# | ||||||||||
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||||||||||
# the License. You may obtain a copy of the License at | ||||||||||
# | ||||||||||
# http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||
# | ||||||||||
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||||||||||
# an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||||||||||
# specific language governing permissions and limitations under the License. | ||||||||||
|
||||||||||
from typing import Dict, Optional, Tuple | ||||||||||
|
||||||||||
from taipy.common.config import Section, _inject_section | ||||||||||
|
||||||||||
|
||||||||||
class RestConfig(Section): | ||||||||||
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. It should be a unique section, no? Only one RestConfig should be created in the Config. No? |
||||||||||
name: str = "REST" | ||||||||||
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.
Suggested change
|
||||||||||
|
||||||||||
def __init__(self): | ||||||||||
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. The init method should expose all the attributes as optional. |
||||||||||
super().__init__("rest") | ||||||||||
self._port: int = 5000 | ||||||||||
self._host: str = "127.0.0.1" | ||||||||||
self._use_https: bool = False | ||||||||||
self._ssl_cert: Optional[str] = None | ||||||||||
self._ssl_key: Optional[str] = None | ||||||||||
|
||||||||||
def __copy__(self): | ||||||||||
new_instance = RestConfig() | ||||||||||
new_instance._port = self._port | ||||||||||
new_instance._host = self._host | ||||||||||
new_instance._use_https = self._use_https | ||||||||||
new_instance._ssl_cert = self._ssl_cert | ||||||||||
new_instance._ssl_key = self._ssl_key | ||||||||||
return new_instance | ||||||||||
|
||||||||||
@classmethod | ||||||||||
def _clean(cls): | ||||||||||
return RestConfig() | ||||||||||
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. The clean def _clean(self):
self._port = self._DEFAULT_PORT
self._port = self._DEFAULT_HOST
... |
||||||||||
|
||||||||||
@classmethod | ||||||||||
def _to_dict(cls): | ||||||||||
return {"port": cls.port, "host": cls.host, "use_https": cls.use_https, "ssl_cert": cls.ssl_cert} | ||||||||||
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. If some values are None, they must not be added to the dict. |
||||||||||
|
||||||||||
@classmethod | ||||||||||
def _from_dict(cls, data: Dict): | ||||||||||
Check failure on line 46 in taipy/rest/config/rest_config.py GitHub Actions / partial-tests / linter
Check failure on line 46 in taipy/rest/config/rest_config.py GitHub Actions / partial-tests / linter
Check failure on line 46 in taipy/rest/config/rest_config.py GitHub Actions / partial-tests / linter
Check failure on line 46 in taipy/rest/config/rest_config.py GitHub Actions / partial-tests / linter
|
||||||||||
instance = RestConfig() | ||||||||||
instance.port = data.get("port", 5000) | ||||||||||
instance.host = data.get("host", "127.0.0.1") | ||||||||||
instance.use_https = data.get("use_https", False) | ||||||||||
instance.ssl_cert = data.get("ssl_cert", None) | ||||||||||
Comment on lines
+48
to
+51
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. We cannot set the default values here when an attribute is not in the dictionary. We must use None instead. Otherwise, the compilation mechanism won't work when the user overrides only one attribute from a TOML file (or from an env variable) |
||||||||||
return instance | ||||||||||
|
||||||||||
@property | ||||||||||
def port(self) -> int: | ||||||||||
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. All the properties (the getters) should have a docstring. 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. All the getters should use the templating system to allow users to overwrite values from env variables. Use |
||||||||||
return self._port | ||||||||||
|
||||||||||
@port.setter | ||||||||||
def port(self, value: int): | ||||||||||
self._port = value | ||||||||||
|
||||||||||
@property | ||||||||||
def host(self) -> str: | ||||||||||
return self._host | ||||||||||
|
||||||||||
@host.setter | ||||||||||
def host(self, value: str): | ||||||||||
self._host = value | ||||||||||
|
||||||||||
@property | ||||||||||
def use_https(self) -> bool: | ||||||||||
return self._use_https | ||||||||||
|
||||||||||
@use_https.setter | ||||||||||
def use_https(self, value: bool): | ||||||||||
self._use_https = value | ||||||||||
|
||||||||||
@property | ||||||||||
def ssl_cert(self) -> Optional[str]: | ||||||||||
return self._ssl_cert | ||||||||||
|
||||||||||
@ssl_cert.setter | ||||||||||
def ssl_cert(self, value: Optional[str]): | ||||||||||
self._ssl_cert = value | ||||||||||
|
||||||||||
@property | ||||||||||
def ssl_key(self) -> Optional[str]: | ||||||||||
return self._ssl_key | ||||||||||
|
||||||||||
@ssl_key.setter | ||||||||||
def ssl_key(self, value: Optional[str]): | ||||||||||
self._ssl_key = value | ||||||||||
|
||||||||||
@property | ||||||||||
def ssl_context(self) -> Optional[Tuple[Optional[str], Optional[str]]]: | ||||||||||
return (self._ssl_cert, self._ssl_key) if self._use_https else None | ||||||||||
|
||||||||||
def configure_rest( | ||||||||||
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. I would use the same design pattern as the existing section implementations for legacy and consistency, even if your proposal seems smoother. Changing the design pattern could be a good idea (the factory pattern can be cumbersome), but it requires some deep discussions before, as it is an important breaking change. For the other sections, we reserve the Something like: @staticmethod
def _configure(
port: int = 5000,
host: str = "127.0.0.1",
use_https: bool = False,
ssl_cert: Optional[str] = None,
ssl_key: Optional[str] = None,
):
section = RestConfig(port, host, use_https, ssl_cert, ssl_key)
Config._register(section)
return Config.unique_sections[RestConfig.name] 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. The method name should be prefixed by an |
||||||||||
self, | ||||||||||
port: int = 5000, | ||||||||||
host: str = "127.0.0.1", | ||||||||||
use_https: bool = False, | ||||||||||
ssl_cert: Optional[str] = None, | ||||||||||
ssl_key: Optional[str] = None, | ||||||||||
): | ||||||||||
self.port = port | ||||||||||
self.host = host | ||||||||||
self.use_https = use_https | ||||||||||
self.ssl_cert = ssl_cert | ||||||||||
self.ssl_key = ssl_key | ||||||||||
|
||||||||||
|
||||||||||
# At the end of the file | ||||||||||
_inject_section( | ||||||||||
RestConfig, "rest", default=RestConfig(), configuration_methods=[("configure_rest", RestConfig.configure_rest)] | ||||||||||
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. The We usually use a factory method named |
||||||||||
) | ||||||||||
Comment on lines
+114
to
+116
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. We usually do it in the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
# Copyright 2021-2025 Avaiga Private Limited | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
# the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
# an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations under the License. | ||
|
||
from taipy.common.config._config import _Config | ||
from taipy.common.config.checker.issue_collector import IssueCollector | ||
from taipy.rest.config.rest_checker import _RestConfigChecker | ||
from taipy.rest.config.rest_config import RestConfig | ||
|
||
|
||
def test_rest_config_default_values(): | ||
rest_config = RestConfig() | ||
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. We should not need to do anything to retrieve the default values. Config.rest.port = RestConfig._DEFAULT_PORT
Config.rest.host = RestConfig._DEFAULT_HOST
... # etc for all the attributes
# Now we use the configure method
Config.configure_rest(port=1)
Config.rest.port = 1
Config.rest.host = RestConfig._DEFAULT_HOST
# now we load a toml file
toml_cfg = NamedTemporaryFile(
content="""
[TAIPY]
[REST]
port = 2
host = "192.168.0.87"
"""
)
Config.load(toml_cfg.filename)
Config.rest.port = 2
Config.rest.host = "192.168.0.87"
# And finally we use env variables
assert Config.rest.port == RestConfig._DEFAULT_PORT
assert Config.rest.host== RestConfig._DEFAULT_HOST
Config.configure_rest(port="ENV[PORT]", host="ENV[HOST]")
with pytest.raises(MissingEnvVariableError):
_ = Config.rest.port
with pytest.raises(MissingEnvVariableError):
_ = Config.rest.host
... # etc for all the attributes
with patch.dict(os.environ, {"PORT": "3:int", "HOST": "1.2.3.4"}):
assert Config.rest.port == 3
assert Config.rest.bar == "1.2.3.4" |
||
assert rest_config.port == 5000 | ||
assert rest_config.host == "127.0.0.1" | ||
assert rest_config.use_https is False | ||
assert rest_config.ssl_cert is None | ||
assert rest_config.ssl_key is None | ||
|
||
|
||
def test_rest_config_custom_values(): | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(port=8080, host="0.0.0.0", use_https=True, ssl_cert="cert.pem", ssl_key="key.pem") | ||
|
||
assert rest_config.port == 8080 | ||
assert rest_config.host == "0.0.0.0" | ||
assert rest_config.use_https is True | ||
assert rest_config.ssl_cert == "cert.pem" | ||
assert rest_config.ssl_key == "key.pem" | ||
|
||
|
||
def test_rest_config_copy(): | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(port=8080, host="0.0.0.0", use_https=True, ssl_cert="cert.pem", ssl_key="key.pem") | ||
rest_config_copy = rest_config.__copy__() | ||
|
||
assert rest_config_copy.port == 8080 | ||
assert rest_config_copy.host == "0.0.0.0" | ||
assert rest_config_copy.use_https is True | ||
assert rest_config_copy.ssl_cert == "cert.pem" | ||
assert rest_config_copy.ssl_key == "key.pem" | ||
|
||
# Ensure it's a deep copy | ||
rest_config_copy.port = 9090 | ||
assert rest_config.port == 8080 | ||
|
||
|
||
def test_rest_config_checker_valid_config(): | ||
config = _Config() | ||
collector = IssueCollector() | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(port=8080, host="0.0.0.0", use_https=True, ssl_cert="cert.pem", ssl_key="key.pem") | ||
|
||
config._sections[RestConfig.name] = {"test_rest_config": rest_config} | ||
checker = _RestConfigChecker(config, collector) | ||
issues = checker._check() | ||
|
||
assert len(issues.errors) == 0 | ||
assert len(issues.warnings) == 0 | ||
|
||
|
||
def test_rest_config_checker_invalid_port(): | ||
config = _Config() | ||
collector = IssueCollector() | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(port=70000) # Invalid port | ||
|
||
config._sections[RestConfig.name] = {"test_rest_config": rest_config} | ||
checker = _RestConfigChecker(config, collector) | ||
issues = checker._check() | ||
|
||
assert len(issues.errors) == 1 | ||
assert "port" in issues.errors[0].field | ||
|
||
|
||
def test_rest_config_checker_invalid_host(): | ||
config = _Config() | ||
collector = IssueCollector() | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(host="") # Invalid host | ||
|
||
config._sections[RestConfig.name] = {"test_rest_config": rest_config} | ||
checker = _RestConfigChecker(config, collector) | ||
issues = checker._check() | ||
|
||
assert len(issues.errors) == 1 | ||
assert "host" in issues.errors[0].field | ||
|
||
|
||
def test_rest_config_checker_https_missing_cert_and_key(): | ||
config = _Config() | ||
collector = IssueCollector() | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(use_https=True) # Missing ssl_cert and ssl_key | ||
|
||
config._sections[RestConfig.name] = {"test_rest_config": rest_config} | ||
checker = _RestConfigChecker(config, collector) | ||
issues = checker._check() | ||
|
||
assert len(issues.errors) == 1 | ||
assert "ssl_cert/ssl_key" in issues.errors[0].field | ||
|
||
|
||
def test_rest_config_checker_https_invalid_cert_and_key(): | ||
config = _Config() | ||
collector = IssueCollector() | ||
rest_config = RestConfig() | ||
rest_config.configure_rest(use_https=True, ssl_cert=123, ssl_key=456) # Invalid types for ssl_cert and ssl_key | ||
|
||
config._sections[RestConfig.name] = {"test_rest_config": rest_config} | ||
checker = _RestConfigChecker(config, collector) | ||
issues = checker._check() | ||
|
||
assert len(issues.errors) == 1 | ||
assert "ssl_cert/ssl_key" in issues.errors[0].field |
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.