-
Notifications
You must be signed in to change notification settings - Fork 6
Fix scan #51
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 scan #51
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| from __future__ import annotations | ||
|
|
||
| import ast | ||
| import json | ||
| import logging | ||
| import os | ||
| import sys | ||
| from typing import ( | ||
|
|
@@ -29,12 +31,14 @@ | |
|
|
||
| from datacustomcode.version import get_version | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| DATA_ACCESS_METHODS = ["read_dlo", "read_dmo", "write_to_dlo", "write_to_dmo"] | ||
|
|
||
| DATA_TRANSFORM_CONFIG_TEMPLATE = { | ||
| "sdkVersion": get_version(), | ||
| "entryPoint": "", | ||
| "dataspace": "default", | ||
| "dataspace": "", | ||
| "permissions": { | ||
| "read": {}, | ||
| "write": {}, | ||
|
|
@@ -232,6 +236,40 @@ def dc_config_json_from_file(file_path: str) -> dict[str, Any]: | |
| config = DATA_TRANSFORM_CONFIG_TEMPLATE.copy() | ||
| config["entryPoint"] = file_path.rpartition("/")[-1] | ||
|
|
||
| file_dir = os.path.dirname(file_path) | ||
| config_json_path = os.path.join(file_dir, "config.json") | ||
|
|
||
| if os.path.exists(config_json_path) and os.path.isfile(config_json_path): | ||
| try: | ||
| with open(config_json_path, "r") as f: | ||
| existing_config = json.load(f) | ||
|
|
||
| if "dataspace" in existing_config: | ||
| dataspace_value = existing_config["dataspace"] | ||
| if not dataspace_value or ( | ||
| isinstance(dataspace_value, str) and dataspace_value.strip() == "" | ||
| ): | ||
| logger.error( | ||
|
Contributor
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. Should this be a
Collaborator
Author
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. yup! addressed in eb51f58 |
||
| f"dataspace in {config_json_path} is empty or None." | ||
| f"Updating config file to use dataspace 'default'." | ||
| ) | ||
| config["dataspace"] = "default" | ||
| else: | ||
| config["dataspace"] = dataspace_value | ||
| else: | ||
| raise ValueError( | ||
| f"dataspace must be defined in {config_json_path}. " | ||
| f"Please add a 'dataspace' field to the config.json file." | ||
| ) | ||
|
Contributor
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 diff between missing, null, or empty string is subtle in my mind. It might be simpler to just default to "default" value for all of those cases, instead of raising an exception specifically if it's missing?
Collaborator
Author
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. as per our internal follow-up discussion, I'm planning to go with throwing an error when |
||
| except json.JSONDecodeError as e: | ||
| raise ValueError( | ||
| f"Failed to parse JSON from {config_json_path}: {e}" | ||
| ) from e | ||
| except OSError as e: | ||
| raise OSError(f"Failed to read config file {config_json_path}: {e}") from e | ||
| else: | ||
| config["dataspace"] = "default" | ||
|
|
||
| read: dict[str, list[str]] = {} | ||
| if output.read_dlo: | ||
| read["dlo"] = list(output.read_dlo) | ||
|
|
@@ -244,4 +282,5 @@ def dc_config_json_from_file(file_path: str) -> dict[str, Any]: | |
| write["dmo"] = list(output.write_to_dmo) | ||
|
|
||
| config["permissions"] = {"read": read, "write": write} | ||
|
|
||
| return config | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -358,6 +358,175 @@ def test_dmo_to_dmo_config(self): | |
| finally: | ||
| os.remove(temp_path) | ||
|
|
||
| @patch( | ||
| "datacustomcode.scan.DATA_TRANSFORM_CONFIG_TEMPLATE", | ||
| { | ||
| "sdkVersion": "1.2.3", | ||
| "entryPoint": "", | ||
| "dataspace": "", | ||
| "permissions": { | ||
| "read": {}, | ||
| "write": {}, | ||
| }, | ||
| }, | ||
| ) | ||
| def test_preserves_existing_dataspace(self): | ||
| """Test that existing dataspace value is preserved when config.json exists.""" | ||
| import json | ||
|
|
||
| content = textwrap.dedent( | ||
| """ | ||
| from datacustomcode.client import Client | ||
|
|
||
| client = Client() | ||
| df = client.read_dlo("input_dlo") | ||
| client.write_to_dlo("output_dlo", df, "overwrite") | ||
| """ | ||
| ) | ||
| temp_path = create_test_script(content) | ||
| file_dir = os.path.dirname(temp_path) | ||
| config_path = os.path.join(file_dir, "config.json") | ||
|
|
||
| try: | ||
| # Create an existing config.json with a custom dataspace | ||
| existing_config = { | ||
| "sdkVersion": "1.0.0", | ||
| "entryPoint": "test.py", | ||
| "dataspace": "my_custom_dataspace", | ||
| "permissions": { | ||
| "read": {"dlo": ["old_dlo"]}, | ||
| "write": {"dlo": ["old_output"]}, | ||
| }, | ||
| } | ||
| with open(config_path, "w") as f: | ||
| json.dump(existing_config, f) | ||
|
|
||
| # Generate new config - should preserve dataspace | ||
| result = dc_config_json_from_file(temp_path) | ||
| assert result["dataspace"] == "my_custom_dataspace" | ||
| assert result["permissions"]["read"]["dlo"] == ["input_dlo"] | ||
| assert result["permissions"]["write"]["dlo"] == ["output_dlo"] | ||
| finally: | ||
| os.remove(temp_path) | ||
| if os.path.exists(config_path): | ||
| os.remove(config_path) | ||
|
|
||
| @patch( | ||
| "datacustomcode.scan.DATA_TRANSFORM_CONFIG_TEMPLATE", | ||
| { | ||
| "sdkVersion": "1.2.3", | ||
| "entryPoint": "", | ||
| "dataspace": "", | ||
| "permissions": { | ||
| "read": {}, | ||
| "write": {}, | ||
| }, | ||
| }, | ||
| ) | ||
| def test_rejects_empty_dataspace(self): | ||
|
Contributor
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. Nit: should this be
Collaborator
Author
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. fixed test names in eb51f58 |
||
| """Test that empty dataspace value uses default and logs error.""" | ||
| import json | ||
|
|
||
| content = textwrap.dedent( | ||
| """ | ||
| from datacustomcode.client import Client | ||
|
|
||
| client = Client() | ||
| df = client.read_dlo("input_dlo") | ||
| client.write_to_dlo("output_dlo", df, "overwrite") | ||
| """ | ||
| ) | ||
| temp_path = create_test_script(content) | ||
| file_dir = os.path.dirname(temp_path) | ||
| config_path = os.path.join(file_dir, "config.json") | ||
|
|
||
| try: | ||
| # Create an existing config.json with empty dataspace | ||
| existing_config = { | ||
| "sdkVersion": "1.0.0", | ||
| "entryPoint": "test.py", | ||
| "dataspace": "", | ||
| "permissions": { | ||
| "read": {"dlo": ["old_dlo"]}, | ||
| "write": {"dlo": ["old_output"]}, | ||
| }, | ||
| } | ||
| with open(config_path, "w") as f: | ||
| json.dump(existing_config, f) | ||
|
|
||
| # Should use "default" for empty dataspace (not raise error) | ||
| result = dc_config_json_from_file(temp_path) | ||
| assert result["dataspace"] == "default" | ||
| assert result["permissions"]["read"]["dlo"] == ["input_dlo"] | ||
| assert result["permissions"]["write"]["dlo"] == ["output_dlo"] | ||
| finally: | ||
| os.remove(temp_path) | ||
| if os.path.exists(config_path): | ||
| os.remove(config_path) | ||
|
|
||
| @patch( | ||
| "datacustomcode.scan.DATA_TRANSFORM_CONFIG_TEMPLATE", | ||
| { | ||
| "sdkVersion": "1.2.3", | ||
| "entryPoint": "", | ||
| "dataspace": "", | ||
| "permissions": { | ||
| "read": {}, | ||
| "write": {}, | ||
| }, | ||
| }, | ||
| ) | ||
| def test_rejects_missing_dataspace(self): | ||
|
Contributor
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 got confused by this test name- looks like we're actually testing This also makes me think: do we want a test to cover the missing dataspace case? Currently, the PR is a raising a ValueError, but I'm suggesting perhaps we simplify that.
Collaborator
Author
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. fixed test names in eb51f58 |
||
| """Test missing config.json uses default dataspace.""" | ||
| content = textwrap.dedent( | ||
| """ | ||
| from datacustomcode.client import Client | ||
|
|
||
| client = Client() | ||
| df = client.read_dlo("input_dlo") | ||
| client.write_to_dlo("output_dlo", df, "overwrite") | ||
| """ | ||
| ) | ||
| temp_path = create_test_script(content) | ||
|
|
||
| try: | ||
| # No existing config.json - should use "default" dataspace | ||
| result = dc_config_json_from_file(temp_path) | ||
| assert result["dataspace"] == "default" | ||
| assert result["permissions"]["read"]["dlo"] == ["input_dlo"] | ||
| assert result["permissions"]["write"]["dlo"] == ["output_dlo"] | ||
| finally: | ||
| os.remove(temp_path) | ||
|
|
||
| def test_raises_error_on_invalid_json(self): | ||
| """Test that invalid JSON in config.json raises an error.""" | ||
|
|
||
| content = textwrap.dedent( | ||
| """ | ||
| from datacustomcode.client import Client | ||
|
|
||
| client = Client() | ||
| df = client.read_dlo("input_dlo") | ||
| client.write_to_dlo("output_dlo", df, "overwrite") | ||
| """ | ||
| ) | ||
| temp_path = create_test_script(content) | ||
| file_dir = os.path.dirname(temp_path) | ||
| config_path = os.path.join(file_dir, "config.json") | ||
|
|
||
| try: | ||
| # Create an invalid JSON file | ||
| with open(config_path, "w") as f: | ||
| f.write("{ invalid json }") | ||
|
|
||
| # Should raise ValueError for invalid JSON | ||
| with pytest.raises(ValueError, match="Failed to parse JSON"): | ||
| dc_config_json_from_file(temp_path) | ||
| finally: | ||
| os.remove(temp_path) | ||
| if os.path.exists(config_path): | ||
| os.remove(config_path) | ||
|
|
||
|
|
||
| class TestDataAccessLayerCalls: | ||
| """Tests for the DataAccessLayerCalls class directly.""" | ||
|
|
||
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.
deployalready runszip, doesn't it?Uh oh!
There was an error while loading. Please reload this page.
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.
yes, it does. this line is optional.
update: addressed in eb51f58