Fix empty path segments in Data Path transformations #453

Andre-Lx-Costa · 2024-07-18T07:52:13Z

Fixes #449

Scope

Implemented:

Fixed path transformations between S3 DataPath and paths to handle empty path segments

Checklist

GitHub issue exists for this change.
Unit tests added and they pass.
Pylint 10.0/10.0 without bloating .pylintrc with exceptions.
Review requested on latest commit.

…ker interface for AccessKeyCredentials in the AWS context.

…rage-client

…rage-client # Conflicts: # adapta/security/clients/aws/_aws_client.py

…rage-client

github-actions · 2024-07-18T07:54:00Z

Coverage Report

File	Stmts	Miss	Cover	Missing
adapta/connectors/service_bus
__init__.py	1	1	0%	18
_connector.py	17	17	0%	19–61
adapta/logs
_async_logger.py	81	3	96%	55, 78–79
_base.py	55	6	89%	35, 41, 44, 94–97
_internal_logger.py	109	6	94%	276–286
adapta/logs/handlers
datadog_api_handler.py	109	31	72%	89, 106–113, 124, 136–152, 161–195, 204, 210, 238
adapta/metrics/providers
datadog_provider.py	43	43	0%	19–147
adapta/ml
__init__.py	1	1	0%	19
_model.py	10	10	0%	17–42
adapta/ml/mlflow
__init__.py	2	2	0%	17–18
_client.py	46	46	0%	19–164
_functions.py	47	47	0%	17–121
adapta/process_communication
_models.py	33	6	82%	90–96
adapta/security/clients
__init__.py	26	12	54%	27–28, 34–35, 41–42, 48–49, 53–54, 58–59
_azure_client.py	73	50	32%	42, 55–65, 75–78, 81, 84–86, 95–153, 156, 159–197
adapta/security/clients/aws
_aws_client.py	38	17	55%	37–40, 47, 57, 63, 75, 100–108, 114–117
_aws_credentials.py	73	30	59%	60–79, 83, 87, 91, 95, 99, 108–112, 116, 120, 124, 128, 132
adapta/security/clients/hashicorp_vault
hashicorp_vault_client.py	31	4	87%	46, 87, 91, 95
kubernetes_client.py	21	5	76%	45–48, 67–68
oidc_client.py	45	20	56%	33–62, 80–83, 92
token_client.py	17	7	59%	42–45, 52–53, 56, 59
adapta/storage/blob
azure_storage_client.py	119	51	57%	71–78, 88, 95–96, 99–105, 127–128, 131–156, 159, 175–188, 196–200, 219, 240–242, 251, 267–271, 281–283, 286–305, 312
local_storage_client.py	54	7	87%	42, 67, 74, 87, 101, 104, 107
s3_storage_client.py	117	77	34%	57, 61, 68–80, 91–96, 117–125, 133–134, 148–165, 181–185, 203–212, 222–243, 254–275
adapta/storage/cache
redis_cache.py	37	37	0%	19–107
adapta/storage/database/v2
azure_sql.py	34	34	0%	21–140
odbc.py	73	73	0%	21–219
snowflake_sql.py	69	69	0%	6–228
trino_sql.py	39	39	0%	21–127
adapta/storage/database/v2/models
__init__.py	1	1	0%	19
_models.py	11	11	0%	20–54
adapta/storage/database/v3
azure_sql.py	32	20	38%	55, 70, 84–122, 132
odbc.py	72	11	85%	94–104, 114, 125–126, 140, 149–155, 182
snowflake_sql.py	77	23	70%	64–78, 93–95, 106–115, 142, 178, 193–196
trino_sql.py	38	38	0%	20–119
adapta/storage/delta_lake/v2
_functions.py	68	45	34%	73–106, 153–168, 210–294
adapta/storage/delta_lake/v3
_functions.py	66	12	82%	72, 156, 161, 163, 165, 221, 231, 241–251, 257, 292
adapta/storage/distributed_object_store/v2/datastax_astra
_models.py	19	9	53%	54–59, 62, 65, 68
astra_client.py	196	140	29%	40–43, 113–137, 143–184, 190–191, 197–198, 201, 211, 223–224, 233, 273–361, 369, 393–520, 530, 541–556, 579–594, 616–632, 656–673
adapta/storage/distributed_object_store/v3/datastax_astra
_model_mappers.py	161	23	86%	64, 130, 136, 138, 142, 148, 152, 167–170, 186–187, 218, 275–285, 370, 411–423, 455
_models.py	45	31	31%	63–69, 72, 75, 78–122, 125
astra_client.py	139	68	51%	35–38, 133–174, 180–181, 187–188, 191, 201, 213–214, 223, 267–353, 361, 373, 384–403, 424–440, 462–482, 507–519
adapta/storage/models
astra.py	35	10	71%	37, 40, 44, 59–61, 64, 70–73
aws.py	37	4	89%	53–55, 63
azure.py	60	17	72%	32, 36–40, 47, 67, 70–71, 74–75, 89–93, 100, 113, 120, 123–124, 127
filter_expression.py	125	5	96%	55, 183–184, 238, 329
hive.py	57	25	56%	37, 41, 44, 92–99, 111, 114–115, 124–172, 175
local.py	21	4	81%	31, 35, 38, 50
adapta/storage/query_enabled_store
_models.py	61	10	84%	76, 138–141, 147–150, 156–157, 163
_qes_astra.py	57	13	77%	65–66, 78, 83–96, 106–109
_qes_delta.py	39	4	90%	33, 60, 72, 80
adapta/storage/secrets
azure_secret_client.py	20	20	0%	19–66
adapta/utils
_common.py	95	15	84%	36–37, 70–83, 94, 122, 142, 162, 245
concurrent_task_runner.py	27	1	96%	109
adapta/utils/data_structures
_functions.py	34	1	97%	134
adapta/utils/decorators
_logging.py	41	1	98%	32
_rate_limit.py	25	1	96%	58
adapta/utils/python_typing
_functions.py	11	1	91%	24
tests
test_filtering_api.py	32	2	94%	197–198
test_s3_storage_client.py	43	2	95%	34–35
test_utils.py	166	1	99%	371
test_vault_client.py	80	18	78%	33–35, 40–42, 47–51, 56–57, 62–66
TOTAL	4570	1338	71%

Tests	Skipped	Failures	Errors	Time
215	5 💤	0 ❌	0 🔥	1m 2s ⏱️

github-actions · 2024-07-18T07:54:05Z

Coverage Report

File	Stmts	Miss	Cover	Missing
adapta/connectors/service_bus
__init__.py	1	1	0%	18
_connector.py	17	17	0%	19–61
adapta/logs
_async_logger.py	81	3	96%	55, 78–79
_base.py	55	6	89%	35, 41, 44, 94–97
_internal_logger.py	109	6	94%	276–286
adapta/logs/handlers
datadog_api_handler.py	109	31	72%	89, 106–113, 124, 136–152, 161–195, 204, 210, 238
adapta/metrics/providers
datadog_provider.py	43	43	0%	19–147
adapta/ml
__init__.py	1	1	0%	19
_model.py	10	10	0%	17–42
adapta/ml/mlflow
__init__.py	2	2	0%	17–18
_client.py	46	46	0%	19–164
_functions.py	47	47	0%	17–121
adapta/process_communication
_models.py	33	6	82%	90–96
adapta/security/clients
__init__.py	26	12	54%	27–28, 34–35, 41–42, 48–49, 53–54, 58–59
_azure_client.py	73	50	32%	42, 55–65, 75–78, 81, 84–86, 95–153, 156, 159–197
adapta/security/clients/aws
_aws_client.py	38	17	55%	37–40, 47, 57, 63, 75, 100–108, 114–117
_aws_credentials.py	73	30	59%	60–79, 83, 87, 91, 95, 99, 108–112, 116, 120, 124, 128, 132
adapta/security/clients/hashicorp_vault
hashicorp_vault_client.py	31	4	87%	46, 87, 91, 95
kubernetes_client.py	21	5	76%	45–48, 67–68
oidc_client.py	45	20	56%	33–62, 80–83, 92
token_client.py	17	7	59%	42–45, 52–53, 56, 59
adapta/storage/blob
azure_storage_client.py	118	50	58%	71–78, 88, 95–96, 99–105, 127–128, 131–156, 159, 175–188, 196–200, 219, 240–242, 251, 267–271, 281–283, 286–305, 312
local_storage_client.py	54	7	87%	42, 67, 74, 87, 101, 104, 107
s3_storage_client.py	117	77	34%	57, 61, 68–80, 91–96, 117–125, 133–134, 148–165, 181–185, 203–212, 222–243, 254–275
adapta/storage/cache
redis_cache.py	37	37	0%	19–107
adapta/storage/database/v2
azure_sql.py	34	34	0%	21–140
odbc.py	73	73	0%	21–219
snowflake_sql.py	69	69	0%	6–228
trino_sql.py	39	39	0%	21–127
adapta/storage/database/v2/models
__init__.py	1	1	0%	19
_models.py	11	11	0%	20–54
adapta/storage/database/v3
azure_sql.py	32	20	38%	55, 70, 84–122, 132
odbc.py	72	11	85%	94–104, 114, 125–126, 140, 149–155, 182
snowflake_sql.py	77	23	70%	64–78, 93–95, 106–115, 142, 178, 193–196
trino_sql.py	38	38	0%	20–119
adapta/storage/delta_lake/v2
_functions.py	68	45	34%	73–106, 153–168, 210–294
adapta/storage/delta_lake/v3
_functions.py	66	12	82%	72, 156, 161, 163, 165, 221, 231, 241–251, 257, 292
adapta/storage/distributed_object_store/v2/datastax_astra
_models.py	19	9	53%	54–59, 62, 65, 68
astra_client.py	196	140	29%	40–43, 113–137, 143–184, 190–191, 197–198, 201, 211, 223–224, 233, 273–361, 369, 393–520, 530, 541–556, 579–594, 616–632, 656–673
adapta/storage/distributed_object_store/v3/datastax_astra
_model_mappers.py	161	23	86%	64, 130, 136, 138, 142, 148, 152, 167–170, 186–187, 218, 275–285, 370, 411–423, 455
_models.py	45	31	31%	63–69, 72, 75, 78–122, 125
astra_client.py	139	68	51%	35–38, 133–174, 180–181, 187–188, 191, 201, 213–214, 223, 267–353, 361, 373, 384–403, 424–440, 462–482, 507–519
adapta/storage/models
astra.py	35	10	71%	37, 40, 44, 59–61, 64, 70–73
aws.py	37	4	89%	53–55, 63
azure.py	60	17	72%	32, 36–40, 47, 67, 70–71, 74–75, 89–93, 100, 113, 120, 123–124, 127
filter_expression.py	125	5	96%	55, 183–184, 238, 329
hive.py	57	25	56%	37, 41, 44, 92–99, 111, 114–115, 124–172, 175
local.py	21	4	81%	31, 35, 38, 50
adapta/storage/query_enabled_store
_models.py	61	10	84%	76, 138–141, 147–150, 156–157, 163
_qes_astra.py	57	13	77%	65–66, 78, 83–96, 106–109
_qes_delta.py	39	4	90%	33, 60, 72, 80
adapta/storage/secrets
azure_secret_client.py	20	20	0%	19–66
adapta/utils
_common.py	95	15	84%	36–37, 70–83, 94, 122, 142, 162, 245
concurrent_task_runner.py	27	1	96%	109
adapta/utils/data_structures
_functions.py	34	1	97%	134
adapta/utils/decorators
_logging.py	41	1	98%	32
_rate_limit.py	25	1	96%	58
adapta/utils/python_typing
_functions.py	11	3	73%	7–9, 22
tests
test_filtering_api.py	32	2	94%	197–198
test_python_typing_functions.py	11	2	82%	38–39
test_s3_storage_client.py	43	2	95%	34–35
test_utils.py	166	1	99%	371
test_vault_client.py	80	18	78%	33–35, 40–42, 47–51, 56–57, 62–66
TOTAL	4568	1341	71%

Tests	Skipped	Failures	Errors	Time
215	6 💤	0 ❌	0 🔥	1m 3s ⏱️

george-zubrienko · 2024-07-29T07:52:38Z

adapta/storage/models/aws.py

@@ -36,7 +36,7 @@ def to_uri(self) -> str:
        if not self.bucket or not self.path:
            raise ValueError("Bucket and path must be defined")

-        return f"s3://{self.bucket}/{self.path}"
+        return f"s3a://{self.bucket.rstrip('/')}/{self.path}"


use __post_init__ and assert or rstrip in there

same for path

Added __post_init__ with a regex expression to check all valid cases for s3 data paths as suggested in cbf15e9

Since it is a complex regex I suggest looking into the unit tests, I tried to cover most of the corner cases

george-zubrienko · 2024-07-29T07:59:53Z

adapta/storage/models/aws.py

@@ -70,25 +70,30 @@ def from_hdfs_path(cls, hdfs_path: str) -> "S3Path":
        """
        assert hdfs_path.startswith("s3a://"), "HDFS S3 path should start with s3a://"
        uri = urlparse(hdfs_path)
-        parsed_path = uri.path.split("/")
+        parsed_path = uri.path.replace("//", "/").split("/")


change the check to use regex, will simplify assertion here

Included this on the regex check done in __post_init__ done in cbf15e

… Bug-fix-empty-path-segment

george-zubrienko

Needs work

george-zubrienko · 2024-08-28T09:29:51Z

adapta/storage/models/aws.py

+        if not self.bucket:
+            raise ValueError("Bucket must be defined")
+
+        path_regex = r"^(?![0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$)[a-z0-9]([a-z0-9\-]{1,61}[a-z0-9])?(\/(?!.*(\/\/|\\))([^\/].{0,1022}\/?)?)?$"


simplify this regex to just check for //

george-zubrienko · 2024-08-28T09:30:44Z

adapta/storage/models/aws.py

+
+        path_regex = r"^(?![0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$)[a-z0-9]([a-z0-9\-]{1,61}[a-z0-9])?(\/(?!.*(\/\/|\\))([^\/].{0,1022}\/?)?)?$"
+
+        s3_path_without_prefix = f"{self.bucket}/{self.path}"


regex should only be applied to self.path, bucket is checked on line 62, we do not aim to validate bucket name with this PR, out of scope

george-zubrienko · 2024-08-28T09:31:01Z

adapta/storage/models/aws.py

+        match = re.match(path_regex, s3_path_without_prefix)
+
+        if not match:
+            raise ValueError(f"Invalid S3Path provided, must comply with : {path_regex}")


do not put regex in error message, explain in human language :)

…gment

Andre-Lx-Costa added 20 commits July 5, 2024 09:59

Moving callable session from storage S3 to AWSClient and adding a mar…

6aeb241

…ker interface for AccessKeyCredentials in the AWS context.

Adding docstring for IConnectionDetails

d040095

Adding missing S3Path annotation

b943218

Merge branch 'refs/heads/main' into Move-callable-session-from-S3-Sto…

d3909c2

…rage-client

Some more improvements

91015cd

Merge branch 'refs/heads/main' into Move-callable-session-from-S3-Sto…

7e8e55f

…rage-client # Conflicts: # adapta/security/clients/aws/_aws_client.py

Small bugfix after testing with Azure client and rebase fix

2ce1169

Addressing lint

f847a23

Merge branch 'refs/heads/main' into Move-callable-session-from-S3-Sto…

8e68394

…rage-client

Fixing readme file

85afee0

Small fixes

7a17a79

Formating

9a571ff

Bug fix now

4ed076d

Reducing scope in the branch

bb012f8

Missed a file

2fa69e8

Added unit tests

def765b

Removing duplicates

5a459da

Merge branch 'refs/heads/main' into Bug-fix-empty-path-segment

8d6b230

Formatting

f48d1ea

Improving some unittests

2c3d77e

Andre-Lx-Costa marked this pull request as ready for review July 18, 2024 07:55

Andre-Lx-Costa added 4 commits July 22, 2024 08:18

Merge branch 'main' into Bug-fix-empty-path-segment

2283dcb

Merge branch 'main' into Bug-fix-empty-path-segment

01ff833

Merge branch 'main' into Bug-fix-empty-path-segment

bb9b1bb

Merge branch 'main' into Bug-fix-empty-path-segment

73464d7

george-zubrienko reviewed Jul 29, 2024

View reviewed changes

Andre-Lx-Costa added 2 commits July 29, 2024 13:42

Addressing PR comments

cbf15e9

Removing no redundant checks

820f8a0

Andre-Lx-Costa added 8 commits July 29, 2024 13:51

Small improvement to the unitests

8c3bcea

Small improvement to the unitests naming descriptions

3c0746b

Let's not change this

c5770be

Let's not change this

0f71abc

Merge remote-tracking branch 'origin/Bug-fix-empty-path-segment' into…

f4c4610

… Bug-fix-empty-path-segment

Merge branch 'main' into Bug-fix-empty-path-segment

917199b

Merge branch 'main' into Bug-fix-empty-path-segment

3e0f229

Merge branch 'main' into Bug-fix-empty-path-segment

5e19b0e

george-zubrienko requested changes Aug 28, 2024

View reviewed changes

Andre-Lx-Costa added 3 commits October 28, 2024 14:18

Merge remote-tracking branch 'origin/main' into Bug-fix-empty-path-se…

80c9717

…gment

Addressing PR comments

2183be5

Fix Linting

fb9ba51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix empty path segments in Data Path transformations #453

Fix empty path segments in Data Path transformations #453

Andre-Lx-Costa commented Jul 18, 2024

github-actions bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

george-zubrienko Jul 29, 2024

george-zubrienko Jul 29, 2024

Andre-Lx-Costa Jul 29, 2024

george-zubrienko Jul 29, 2024

Andre-Lx-Costa Jul 29, 2024 •

edited

Loading

george-zubrienko left a comment

george-zubrienko Aug 28, 2024

george-zubrienko Aug 28, 2024

george-zubrienko Aug 28, 2024


		path_regex = r"^(?![0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$)[a-z0-9]([a-z0-9\-]{1,61}[a-z0-9])?(\/(?!.*(\/\/\|\\))([^\/].{0,1022}\/?)?)?$"

		s3_path_without_prefix = f"{self.bucket}/{self.path}"

Fix empty path segments in Data Path transformations #453

Are you sure you want to change the base?

Fix empty path segments in Data Path transformations #453

Conversation

Andre-Lx-Costa commented Jul 18, 2024

Scope

Checklist

github-actions bot commented Jul 18, 2024 • edited Loading

github-actions bot commented Jul 18, 2024 • edited Loading

george-zubrienko Jul 29, 2024

Choose a reason for hiding this comment

george-zubrienko Jul 29, 2024

Choose a reason for hiding this comment

Andre-Lx-Costa Jul 29, 2024

Choose a reason for hiding this comment

george-zubrienko Jul 29, 2024

Choose a reason for hiding this comment

Andre-Lx-Costa Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

george-zubrienko left a comment

Choose a reason for hiding this comment

george-zubrienko Aug 28, 2024

Choose a reason for hiding this comment

george-zubrienko Aug 28, 2024

Choose a reason for hiding this comment

george-zubrienko Aug 28, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

Andre-Lx-Costa Jul 29, 2024 •

edited

Loading