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

Fix duplicate slashes in url #505

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

from typing import TYPE_CHECKING, Any
from urllib.parse import urljoin
from uuid import UUID

from great_expectations.core.http import create_session
Expand Down Expand Up @@ -75,7 +76,10 @@ def _get_table_names(self, datasource: Datasource) -> list[str]:

def _update_table_names_list(self, config_id: UUID, table_names: list[str]) -> None:
with create_session(access_token=self._auth_key) as session:
url = f"{self._base_url}/api/v1/organizations/{self._organization_id}/draft-table-names/{config_id}"
url = urljoin(
base=self._base_url,
url=f"/api/v1/organizations/{self._organization_id}/draft-table-names/{config_id}",
)
response = session.put(
url=url,
json={"data": {"table_names": table_names}},
Expand All @@ -89,9 +93,9 @@ def _update_table_names_list(self, config_id: UUID, table_names: list[str]) -> N
)

def get_draft_config(self, config_id: UUID) -> dict[str, Any]:
resource_url = (
f"{self._base_url}/api/v1/organizations/"
f"{self._organization_id}/draft-datasources/{config_id}"
resource_url = urljoin(
base=self._base_url,
url=f"/api/v1/organizations/{self._organization_id}/draft-datasources/{config_id}",
)
with create_session(access_token=self._auth_key) as session:
response = session.get(resource_url)
Expand Down
8 changes: 6 additions & 2 deletions great_expectations_cloud/agent/actions/list_table_names.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

from typing import TYPE_CHECKING
from urllib.parse import urljoin

from great_expectations.core.http import create_session
from great_expectations.datasource.fluent import SQLDatasource
Expand Down Expand Up @@ -49,9 +50,12 @@ def run(self, event: ListTableNamesEvent, id: str) -> ActionResult:

def _add_or_update_table_names_list(self, datasource_id: str, table_names: list[str]) -> None:
with create_session(access_token=self._auth_key) as session:
url = urljoin(
base=self._base_url,
url=f"/api/v1/organizations/{self._organization_id}/table-names/{datasource_id}",
)
response = session.put(
url=f"{self._base_url}/api/v1/organizations/"
f"{self._organization_id}/table-names/{datasource_id}",
url=url,
json={"data": {"table_names": table_names}},
)
if response.status_code != 200: # noqa: PLR2004
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

from urllib.parse import urljoin

from typing_extensions import override

from great_expectations_cloud.agent.actions.agent_action import (
Expand All @@ -16,7 +18,10 @@
class RunScheduledWindowCheckpointAction(AgentAction[RunScheduledWindowCheckpointEvent]):
@override
def run(self, event: RunScheduledWindowCheckpointEvent, id: str) -> ActionResult:
expectation_parameters_url = f"{self._base_url}/api/v1/organizations/{self._organization_id}/checkpoints/{event.checkpoint_id}/expectation-parameters"
expectation_parameters_url = urljoin(
base=self._base_url,
url=f"/api/v1/organizations/{self._organization_id}/checkpoints/{event.checkpoint_id}/expectation-parameters",
)
return run_window_checkpoint(
context=self._context,
event=event,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

from typing import TYPE_CHECKING
from urllib.parse import urljoin

from great_expectations.core.http import create_session
from great_expectations.exceptions import GXCloudError
Expand All @@ -24,7 +25,10 @@
class RunWindowCheckpointAction(AgentAction[RunWindowCheckpointEvent]):
@override
def run(self, event: RunWindowCheckpointEvent, id: str) -> ActionResult:
expectation_parameters_url = f"{self._base_url}/api/v1/organizations/{self._organization_id}/checkpoints/{event.checkpoint_id}/expectation-parameters"
expectation_parameters_url = urljoin(
base=self._base_url,
url=f"/api/v1/organizations/{self._organization_id}/checkpoints/{event.checkpoint_id}/expectation-parameters",
)
return run_window_checkpoint(
self._context,
event,
Expand Down
18 changes: 10 additions & 8 deletions great_expectations_cloud/agent/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from functools import partial
from importlib.metadata import version as metadata_version
from typing import TYPE_CHECKING, Any, Callable, Dict, Final, Literal
from urllib.parse import urljoin
from uuid import UUID

import orjson
Expand Down Expand Up @@ -413,9 +414,9 @@
) from validation_err

# obtain the broker url and queue name from Cloud
agent_sessions_url = (
f"{env_vars.gx_cloud_base_url}/api/v1/organizations/"
f"{env_vars.gx_cloud_organization_id}/agent-sessions"
agent_sessions_url = urljoin(

Check warning on line 417 in great_expectations_cloud/agent/agent.py

View check run for this annotation

Codecov / codecov/patch

great_expectations_cloud/agent/agent.py#L417

Added line #L417 was not covered by tests
env_vars.gx_cloud_base_url,
f"/api/v1/organizations/{env_vars.gx_cloud_organization_id}/agent-sessions",
)

session = create_session(access_token=env_vars.gx_cloud_access_token)
Expand Down Expand Up @@ -459,9 +460,9 @@
"organization_id": str(org_id),
},
)
agent_sessions_url = (
f"{self._config.gx_cloud_base_url}/api/v1/organizations/{org_id}"
+ f"/agent-jobs/{correlation_id}"
agent_sessions_url = urljoin(

Check warning on line 463 in great_expectations_cloud/agent/agent.py

View check run for this annotation

Codecov / codecov/patch

great_expectations_cloud/agent/agent.py#L463

Added line #L463 was not covered by tests
self._config.gx_cloud_base_url,
f"/api/v1/organizations/{org_id}/agent-jobs/{correlation_id}",
)
with create_session(access_token=self.get_auth_key()) as session:
data = UpdateJobStatusRequest(data=status).json()
Expand Down Expand Up @@ -503,8 +504,9 @@
},
)

agent_sessions_url = (
f"{self._config.gx_cloud_base_url}/api/v1/organizations/{org_id}" + "/agent-jobs"
agent_sessions_url = urljoin(

Check warning on line 507 in great_expectations_cloud/agent/agent.py

View check run for this annotation

Codecov / codecov/patch

great_expectations_cloud/agent/agent.py#L507

Added line #L507 was not covered by tests
self._config.gx_cloud_base_url,
f"/api/v1/organizations/{org_id}/agent-jobs",
)
with create_session(access_token=self.get_auth_key()) as session:
payload = Payload(data=data)
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "great_expectations_cloud"
version = "20241017.0.dev4"
version = "20241022.0.dev1"
description = "Great Expectations Cloud"
authors = ["The Great Expectations Team <[email protected]>"]
repository = "https://github.com/great-expectations/cloud"
Expand Down
56 changes: 48 additions & 8 deletions tests/agent/test_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from time import sleep
from typing import TYPE_CHECKING, Callable, Literal
from unittest.mock import call
from urllib.parse import urljoin

import pytest
import requests
Expand All @@ -19,7 +20,10 @@

from great_expectations_cloud.agent import GXAgent
from great_expectations_cloud.agent.actions.agent_action import ActionResult
from great_expectations_cloud.agent.agent import GXAgentConfig, Payload
from great_expectations_cloud.agent.agent import (
GXAgentConfig,
Payload,
)
from great_expectations_cloud.agent.constants import USER_AGENT_HEADER, HeaderName
from great_expectations_cloud.agent.exceptions import GXAgentConfigError
from great_expectations_cloud.agent.message_service.asyncio_rabbit_mq_client import (
Expand Down Expand Up @@ -285,7 +289,7 @@ def test_gx_agent_updates_cloud_on_job_status(
):
correlation_id = "4ae63677-4dd5-4fb0-b511-870e7a286e77"
url = (
f"{gx_agent_config.gx_cloud_base_url}/api/v1/organizations/"
f"http://localhost:5000/api/v1/organizations/"
f"{gx_agent_config.gx_cloud_organization_id}/agent-jobs/{correlation_id}"
)
job_started_data = UpdateJobStatusRequest(data=JobStarted()).json()
Expand Down Expand Up @@ -362,7 +366,7 @@ def test_gx_agent_sends_request_to_create_scheduled_job(
"""
correlation_id = "4ae63677-4dd5-4fb0-b511-870e7a286e77"
post_url = (
f"{gx_agent_config.gx_cloud_base_url}/api/v1/organizations/"
f"http://localhost:5000/api/v1/organizations/"
f"{gx_agent_config.gx_cloud_organization_id}/agent-jobs"
)

Expand Down Expand Up @@ -519,6 +523,7 @@ def test_correlation_id_header(
gx_agent_config: GXAgentConfig,
fake_subscriber: FakeSubscriber,
random_uuid: str,
local_mercury: str,
):
"""Ensure agent-job-id/correlation-id header is set on GX Cloud api calls and updated for every new job."""
agent_correlation_ids: list[str] = [str(uuid.uuid4()) for _ in range(3)]
Expand Down Expand Up @@ -552,7 +557,7 @@ def test_correlation_id_header(
),
]
)
base_url = gx_agent_config.gx_cloud_base_url
base_url = local_mercury
org_id = gx_agent_config.gx_cloud_organization_id
with responses.RequestsMock() as rsps:
rsps.add(
Expand All @@ -562,7 +567,7 @@ def test_correlation_id_header(
)
rsps.add(
responses.GET,
f"{base_url}/api/v1/organizations/{org_id}/draft-datasources/{datasource_config_id_1}",
f"{base_url}api/v1/organizations/{org_id}/draft-datasources/{datasource_config_id_1}",
json={
"data": {
"config": {
Expand All @@ -575,7 +580,7 @@ def test_correlation_id_header(
)
rsps.add(
responses.GET,
f"{base_url}/api/v1/organizations/{org_id}/draft-datasources/{datasource_config_id_2}",
f"{base_url}api/v1/organizations/{org_id}/draft-datasources/{datasource_config_id_2}",
json={
"data": {
"config": {
Expand All @@ -588,7 +593,7 @@ def test_correlation_id_header(
)
rsps.add(
responses.PUT,
f"{base_url}/api/v1/organizations/{org_id}/draft-table-names/{datasource_config_id_1}",
f"{base_url}api/v1/organizations/{org_id}/draft-table-names/{datasource_config_id_1}",
json={
"data": {
"table_names": [],
Expand All @@ -597,7 +602,7 @@ def test_correlation_id_header(
)
rsps.add(
responses.PUT,
f"{base_url}/api/v1/organizations/{org_id}/draft-table-names/{datasource_config_id_2}",
f"{base_url}api/v1/organizations/{org_id}/draft-table-names/{datasource_config_id_2}",
json={
"data": {
"table_names": [],
Expand All @@ -622,3 +627,38 @@ def test_raise_gx_cloud_err_on_http_error_success_response():
test_response.status_code = 200
# no Exception raised
GXAgent._raise_gx_cloud_err_on_http_error(test_response, "Test error message")


@pytest.mark.parametrize(
"base,path,expected_url",
[
pytest.param(
"http://localhost:5000/",
"/api/v1/something",
"http://localhost:5000/api/v1/something",
id="base_slash + path_slash",
),
pytest.param(
"http://localhost:5000",
"/api/v1/something",
"http://localhost:5000/api/v1/something",
id="base_no_slash + path_slash",
),
pytest.param(
"http://localhost:5000/",
"api/v1/something",
"http://localhost:5000/api/v1/something",
id="base_slash + path_no_slash",
),
pytest.param(
"http://localhost:5000",
"api/v1/something",
"http://localhost:5000/api/v1/something",
id="base_no_slash + path_no_slash",
),
],
)
def test_construct_url_from_base_plus_path(base: str, path: str, expected_url: str):
"""Test that urljoin constructs all combinations of urls correctly."""
constructed_url = urljoin(base, path)
assert constructed_url == expected_url
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to test urljoin itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you're right - I wrote this test when I had a custom fn. Will clean it up.

Loading