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

Refactor Http class to utilize ParameterHelper for http_params conver… #130

Merged
2 changes: 2 additions & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ It started after version 2.0.5 released
* Remove attribute tenant from pydolphinscheduler.core.workflow.workflow ([#54](https://github.com/apache/dolphinscheduler-sdk-python/pull/54))
and please change tenant name in ``config.yaml`` in ``PYDS_HOME``
* Drop support of python3.6 and python3.7 ([#126](https://github.com/apache/dolphinscheduler-sdk-python/pull/126))
* Deprecated old format of `http_params` in the Http class to support a new format. Users should now use the updated format.([#88])([#130])(https://github.com/apache/dolphinscheduler-sdk-python/pull/130)
zhongjiajie marked this conversation as resolved.
Show resolved Hide resolved


## 4.0.0

Expand Down
66 changes: 53 additions & 13 deletions src/pydolphinscheduler/tasks/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
from pydolphinscheduler.constants import TaskType
from pydolphinscheduler.core.task import Task
from pydolphinscheduler.exceptions import PyDSParamException
from pydolphinscheduler.core.parameter import ParameterHelper
import warnings


class HttpMethod:
Expand Down Expand Up @@ -51,7 +53,29 @@ class HttpCheckCondition:


class Http(Task):
"""Task HTTP object, declare behavior for HTTP task to dolphinscheduler."""
"""Task HTTP object, declare behavior for HTTP task to dolphinscheduler.
Attributes:
_task_custom_attr (set): A set containing custom attributes specific to the Http task,
including 'url', 'http_method', 'http_params', and more.
Args:
:param str name: The name or identifier for the HTTP task.
:param str url: The URL endpoint for the HTTP request.
:param str http_method: The HTTP method for the request (GET, POST, etc.). Defaults to HttpMethod.GET.
:param dict http_params: Parameters for the HTTP request. Defaults to None.
:param str http_check_condition: Condition for checking the HTTP response status.
Defaults to HttpCheckCondition.STATUS_CODE_DEFAULT.
:param str condition: Additional condition to evaluate if `http_check_condition` is not STATUS_CODE_DEFAULT.
:param int connect_timeout: Connection timeout for the HTTP request in milliseconds. Defaults to 60000.
:param int socket_timeout: Socket timeout for the HTTP request in milliseconds. Defaults to 60000.
zhongjiajie marked this conversation as resolved.
Show resolved Hide resolved


Raises:
PyDSParamException: Exception raised for invalid parameters, such as unsupported HTTP methods or conditions.

Example:
Usage example for creating an HTTP task:
http_task = Http(name="http_task", url="https://api.example.com", http_method="POST", http_params={"key": "value"})
Comment on lines +77 to +79
Copy link
Member

Choose a reason for hiding this comment

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

you add a good example 👍

"""

_task_custom_attr = {
"url",
Expand All @@ -68,7 +92,7 @@ def __init__(
name: str,
url: str,
http_method: str | None = HttpMethod.GET,
http_params: str | None = None,
http_params: dict | None = None,
http_check_condition: str | None = HttpCheckCondition.STATUS_CODE_DEFAULT,
condition: str | None = None,
connect_timeout: int | None = 60000,
Expand All @@ -82,20 +106,36 @@ def __init__(
raise PyDSParamException(
"Parameter http_method %s not support.", http_method
)

if isinstance(http_params, list):
warnings.warn(
"The `http_params` parameter currently accepts a dictionary instead of a list. Your parameter is being ignored.",
DeprecationWarning,
)

self.http_method = http_method
self.http_params = http_params or []
if not hasattr(HttpCheckCondition, http_check_condition):
raise PyDSParamException(
self._http_params = http_params

@property
def http_params(self):
"""Property to convert http_params using ParameterHelper when accessed."""
return ParameterHelper.convert_params(self._http_params, direction=Direction.IN) if self._http_params else []




if not hasattr(HttpCheckCondition, http_check_condition):
raise PyDSParamException(
"Parameter http_check_condition %s not support.", http_check_condition
)
self.http_check_condition = http_check_condition
if (
http_check_condition != HttpCheckCondition.STATUS_CODE_DEFAULT
self.http_check_condition = http_check_condition
if (
http_check_condition != HttpCheckCondition.STATUS_CODE_DEFAULT
and condition is None
):
raise PyDSParamException(
"Parameter condition must provider if http_check_condition not equal to STATUS_CODE_DEFAULT"
raise PyDSParamException(
"Parameter condition must provider if http_check_condition not equal to STATUS_CODE_DEFAULT"
)
self.condition = condition
self.connect_timeout = connect_timeout
self.socket_timeout = socket_timeout
self.condition = condition
self.connect_timeout = connect_timeout
self.socket_timeout = socket_timeout
32 changes: 32 additions & 0 deletions src/pydolphinscheduler/tasks/test_http_conversion.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
from pydolphinscheduler.core.parameter import ParameterHelper, ParameterType
from http.py import Http # Replace 'your_module_path' with the actual path or import structure

def test_http_params_conversion():
Copy link
Member

Choose a reason for hiding this comment

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

Great! you add some tests for this, but we already have tests for task http in https://github.com/apache/dolphinscheduler-sdk-python/blob/f82e07d232380fe9f5582db4dcca920ae00befd4/tests/tasks/test_http.py could you merge into that file?

# Create a sample http_params dictionary
http_params_dict = {
"prop1": "value1",
"prop2": "value2",
"prop3": "value3"
}

# Create an instance of the Http class with http_params as a dictionary
http_instance = Http(
name="test_http",
url="http://www.example.com",
http_method="GET",
http_params=http_params_dict
)

# Print the initialized http_params attribute (should be converted to the desired format)
print("Converted http_params:", http_instance.http_params)

# Add any assertions or additional tests as required based on your project's logic
assert isinstance(http_instance.http_params, list)
assert len(http_instance.http_params) == len(http_params_dict)

# Add more assertions if necessary to validate the content or structure of the converted parameters
# For instance, check if certain properties exist or match expected values in the converted list format
# assert some_condition, "Assertion failed: Custom message if condition is not met"

if __name__ == "__main__":
test_http_params_conversion()
30 changes: 29 additions & 1 deletion tests/tasks/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@

from pydolphinscheduler.exceptions import PyDSParamException
from pydolphinscheduler.tasks.http import Http, HttpCheckCondition, HttpMethod

from pydolphinscheduler.core.parameter import ParameterHelper, ParameterType
from http.py import Http # Replace 'your_module_path' with the actual path or import structure

@pytest.mark.parametrize(
"class_name, attrs",
Expand Down Expand Up @@ -126,3 +127,30 @@ def test_http_get_define():
):
http = Http(name, url)
assert http.task_params == expect_task_params


def test_http_params_conversion():
Copy link
Member

Choose a reason for hiding this comment

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

due to you already copy and paste it from file test_http_conversion.py I thing you can remove the file test_http_conversion.py.

Copy link
Member

Choose a reason for hiding this comment

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

BTW we should add one more test for the warning when user still pass list to http task parameter http_params. we have an example in

def test_deprecated_sub_workflow_get_define(mock_workflow_definition):
"""Test deprecated task sub_process still work and raise warning."""
code = 123
version = 1
name = "test_sub_workflow_get_define"
expect_task_params = {
"resourceList": [],
"localParams": [],
"processDefinitionCode": TEST_SUB_WORKFLOW_CODE,
"dependence": {},
"conditionResult": {"successNode": [""], "failedNode": [""]},
"waitStartTimeout": {},
}
with patch(
"pydolphinscheduler.core.task.Task.gen_code_and_version",
return_value=(code, version),
):
with warnings.catch_warnings(record=True) as w:
from pydolphinscheduler.tasks.sub_process import SubProcess
assert len(w) == 1
assert issubclass(w[-1].category, DeprecationWarning)
assert "deprecated" in str(w[-1].message)
with Workflow(TEST_WORKFLOW_NAME):
sub_workflow = SubProcess(name, TEST_SUB_WORKFLOW_NAME)
assert sub_workflow.task_params == expect_task_params
wish it can help you

# Create a sample http_params dictionary
http_params_dict = {
"prop1": "value1",
"prop2": "value2",
"prop3": "value3"
HarshitNagpal29 marked this conversation as resolved.
Show resolved Hide resolved
}

# Create an instance of the Http class with http_params as a dictionary
http_instance = Http(
zhongjiajie marked this conversation as resolved.
Show resolved Hide resolved
name="test_http",
url="http://www.example.com",
http_method="GET",
http_params=http_params_dict
)

# Print the initialized http_params attribute (should be converted to the desired format)
print("Converted http_params:", http_instance.http_params)
HarshitNagpal29 marked this conversation as resolved.
Show resolved Hide resolved

# Add any assertions or additional tests as required based on your project's logic
assert isinstance(http_instance.http_params, list)
assert len(http_instance.http_params) == len(http_params_dict)
Copy link
Member

Choose a reason for hiding this comment

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

we should also add one more test for http_instance.http_params, cause this is the core concept about what we change in this PR. For example

expect = [
        {"prop": "prop1", "direct": "IN", "type": "VARCHAR", "value": "value1"},
        {"prop": "prop2", "direct": "IN", "type": "VARCHAR", "value": "value2"},
        {"prop": "prop3", "direct": "IN", "type": "VARCHAR", "value": "value3"},
]
assert http.http_params == expect