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.

Sign up for GitHub

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: Make asset classes fields dataclass fields #863

Merged
merged 7 commits into from
Feb 17, 2025
Merged

Fix: Make asset classes fields dataclass fields #863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
81e8cfb
Make asset classes files data fields
Feb 13, 2025
47e3e49
Fix test definitions_test.py
Feb 13, 2025
7533078
Fix version field typing in IPv4 and IPv6 assets
Feb 14, 2025
74ef100
Remove __init__ if redundant
Feb 14, 2025
73b887c
Merge branch 'main' into fix/asset-class-use-non-data-field
nassermohamedit0 Feb 14, 2025
3ecdc50
Add __post_init__ method to initialize ip version if None
Feb 17, 2025
4e3f442
Add __post_init__ method to initialize ip version if None
Feb 17, 2025
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
18 changes: 5 additions & 13 deletions src/ostorlab/assets/agent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,11 @@
class Agent(asset.Asset):
"""Agent asset."""

def __init__(
self,
key: str,
version: Optional[str] = None,
git_location: Optional[str] = None,
docker_location: Optional[str] = None,
yaml_file_location: Optional[str] = None,
):
self.key = key
self.version = version
self.git_location = git_location
self.docker_location = docker_location
self.yaml_file_location = yaml_file_location
key: str
Copy link

Choose a reason for hiding this comment

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

Add a docstring to describe the purpose of the key field. This will improve code readability and maintainability. For example: 'key: str # Agent key.' Also, apply the same principle to the 'name', 'version', 'docker_image', and 'description' fields by adding specific docstrings to each.

Suggested change
key: str
key: str # Agent key.

version: Optional[str] = None
git_location: Optional[str] = None
docker_location: Optional[str] = None
yaml_file_location: Optional[str] = None

Check warning on line 18 in src/ostorlab/assets/agent.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/agent.py#L14-L18 

Added lines #L14 - L18 were not covered by tests

def __str__(self) -> str:
if self.version is not None:
Expand Down
12 changes: 3 additions & 9 deletions src/ostorlab/assets/android_aab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,9 @@
class AndroidAab(asset.Asset):
"""Android .AAB target asset."""

def __init__(
self,
content: Optional[bytes] = None,
path: Optional[str] = None,
content_url: Optional[str] = None,
):
self.content = content
self.path = path
self.content_url = content_url
content: Optional[bytes] = None
Copy link

Choose a reason for hiding this comment

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

Add a docstring to explain the purpose of the content field. Consider using an empty bytes object (b"") as the default value instead of None, which might simplify handling cases where AAB content is expected. If None is retained, ensure all code accessing content includes a check for None to prevent potential errors. Provide clear guidance on how the None value will be handled, versus an empty bytes object, in different parts of the system using this asset. For example, if an empty AAB is a valid case, then b'' is more appropriate than None .

Suggested change
content: Optional[bytes] = None
content: bytes | None = b""
"""The content of the AAB file."""

path: Optional[str] = None
content_url: Optional[str] = None

Check warning on line 16 in src/ostorlab/assets/android_aab.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/android_aab.py#L14-L16 

Added lines #L14 - L16 were not covered by tests

def __str__(self) -> str:
str_representation = "Android AAB"
Expand Down
12 changes: 3 additions & 9 deletions src/ostorlab/assets/android_apk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,9 @@
class AndroidApk(asset.Asset):
"""Android .APK target asset."""

def __init__(
self,
content: Optional[bytes] = None,
path: Optional[str] = None,
content_url: Optional[str] = None,
):
self.content = content
self.path = path
self.content_url = content_url
content: Optional[bytes] = None
Copy link

Choose a reason for hiding this comment

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

Consider adding a docstring to explain the purpose of the content field. Suggestion: """The APK file content as bytes.""" Also, for better flexibility with large files, consider using a file-like object (e.g., io.BytesIO) instead of bytes. This would allow reading the APK content in chunks, reducing memory usage. Example: content: typing.BinaryIO and update the constructor accordingly to accept and handle file-like objects. If you switch to file-like object, make sure to update any code that reads this content, to read from the stream instead of assuming it's all in memory. Update the docstring accordingly if a file-like object is used. Suggestion: """The APK file content as a binary stream."""

Suggested change
content: Optional[bytes] = None
content: bytes = dataclasses.field(init=False, repr=False)
"""The APK file content as bytes."""

path: Optional[str] = None
content_url: Optional[str] = None

Check warning on line 16 in src/ostorlab/assets/android_apk.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/android_apk.py#L14-L16 

Added lines #L14 - L16 were not covered by tests

def __str__(self) -> str:
str_representation = "Android APK"
Expand Down
3 changes: 1 addition & 2 deletions src/ostorlab/assets/android_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
class AndroidStore(asset.Asset):
"""Android store package target asset."""

def __init__(self, package_name: str):
self.package_name = package_name
package_name: str

Check warning on line 13 in src/ostorlab/assets/android_store.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/android_store.py#L13 

Added line #L13 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

Add a docstring to describe the purpose of 'package_name' and its usage to improve code readability and maintainability. For example: package_name: str = dataclasses.field(metadata={'description': 'The package name of the Android application.'}). Place this docstring within the AndroidStore dataclass definition, ideally directly above the package_name field declaration.

Suggested change
package_name: str
package_name: str = dataclasses.field(metadata={'description': 'The package name of the Android application.'})


def __str__(self) -> str:
return f"Android Store: ({self.package_name})"
Expand Down
15 changes: 4 additions & 11 deletions src/ostorlab/assets/api_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,10 @@
class ApiSchema(asset.Asset):
"""API Schema target asset."""

def __init__(
self,
endpoint_url: str,
content: Optional[bytes] = None,
content_url: Optional[str] = None,
schema_type: Optional[str] = None,
):
self.content = content
self.endpoint_url = endpoint_url
self.schema_type = schema_type
self.content_url = content_url
endpoint_url: str
Copy link

Choose a reason for hiding this comment

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

Add a docstring to the name field to describe its purpose. This will improve code clarity and maintainability. For example: name: str = Field(description='The name of the API schema.')

Suggested change
endpoint_url: str
name: str = Field(description='The name of the API schema.')

content: Optional[bytes] = None
content_url: Optional[str] = None
schema_type: Optional[str] = None

Check warning on line 20 in src/ostorlab/assets/api_schema.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/api_schema.py#L17-L20 

Added lines #L17 - L20 were not covered by tests

def __str__(self) -> str:
str_representation = CLASS_NAME
Expand Down
3 changes: 1 addition & 2 deletions src/ostorlab/assets/domain_name.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@
class DomainName(asset.Asset):
"""Domain Name target asset per RFC 1034 and 1035."""

def __init__(self, name: str):
self.name = name
name: str
Copy link

Choose a reason for hiding this comment

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

Consider adding a default value for the name field or using typing.Optional[str] if the field can be None. This enhances code clarity and prevents potential errors if the value is not provided during object instantiation. For example: name: str = "" or name: typing.Optional[str] = None depending on your specific use case. Only apply this fix if the field is intended to be optional. If the field is required, do not apply the suggested fix. Also, this applies to other dataclasses in the project if they lack default values and might be None during runtime.

Suggested change
name: str
name: str = ""


def __str__(self) -> str:
return self.name

Check warning on line 16 in src/ostorlab/assets/domain_name.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/domain_name.py#L13-L16 

Added lines #L13 - L16 were not covered by tests

@property
def proto_field(self) -> str:
Expand Down
12 changes: 3 additions & 9 deletions src/ostorlab/assets/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,9 @@
class File(asset.Asset):
"""File target asset."""

def __init__(
self,
content: Optional[bytes] = None,
path: Optional[str] = None,
content_url: Optional[str] = None,
):
self.content = content
self.path = path
self.content_url = content_url
content: Optional[bytes] = None
Copy link

Choose a reason for hiding this comment

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

For consistency, either add default values for path and content_url as well, or remove the default value from content to ensure uniform class initialization.

Suggested change
content: Optional[bytes] = None

path: Optional[str] = None
content_url: Optional[str] = None

def __str__(self) -> str:
str_representation = "File"
Expand Down
12 changes: 3 additions & 9 deletions src/ostorlab/assets/ios_ipa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,9 @@
class IOSIpa(asset.Asset):
"""IOS .IPA target asset."""

def __init__(
self,
content: Optional[bytes] = None,
path: Optional[str] = None,
content_url: Optional[str] = None,
):
self.content = content
self.path = path
self.content_url = content_url
content: Optional[bytes] = None
Copy link

Choose a reason for hiding this comment

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

Add a docstring to explain the purpose of the content field. This improves readability and maintainability. For example: """The content of the IPA file, if available."""

Suggested change
content: Optional[bytes] = None
content: Optional[bytes] = None
"""The content of the IPA file, if available."""

path: Optional[str] = None
content_url: Optional[str] = None

Check warning on line 16 in src/ostorlab/assets/ios_ipa.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ios_ipa.py#L14-L16 

Added lines #L14 - L16 were not covered by tests

def __str__(self) -> str:
str_representation = "iOS"
Expand Down
3 changes: 1 addition & 2 deletions src/ostorlab/assets/ios_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
class IOSStore(asset.Asset):
"""Ios bundle target asset."""

def __init__(self, bundle_id: str):
self.bundle_id = bundle_id
bundle_id: str

Check warning on line 13 in src/ostorlab/assets/ios_store.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ios_store.py#L13 

Added line #L13 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

Remove the comment. It's redundant since the code change itself should be clear enough. If the fix is complex, a more detailed explanation of the fix or a link to the issue it resolves would be more helpful than a simple confirmation that it's correct.

Suggested change
bundle_id: str


def __str__(self) -> str:
return f"iOS Store ({self.bundle_id})"
Expand Down
3 changes: 1 addition & 2 deletions src/ostorlab/assets/ios_testflight.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
class IOSTestflight(asset.Asset):
"""iOS testflight target asset."""

def __init__(self, application_url: str):
self.application_url = application_url
application_url: str

Check warning on line 13 in src/ostorlab/assets/ios_testflight.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ios_testflight.py#L13 

Added line #L13 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

Add a comment to describe the purpose of the application_url field. This improves code readability and maintainability by providing context. For example: application_url: str # The URL of the iOS Testflight application.

Suggested change
application_url: str
application_url: str # The URL of the iOS Testflight application.


def __str__(self) -> str:
return f"iOS Testflight ({self.application_url})"
Expand Down
13 changes: 7 additions & 6 deletions src/ostorlab/assets/ip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@
class IP(asset.Asset):
"""IP Address target asset."""

def __init__(
self, host: str, version: Optional[int] = None, mask: Optional[str] = None
):
self.host = host
self.version = version or ipaddress.ip_address(self.host).version
self.mask = mask
host: str
version: Optional[int] = None
mask: Optional[str] = None

Check warning on line 17 in src/ostorlab/assets/ip.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ip.py#L15-L17 

Added lines #L15 - L17 were not covered by tests

def __post_init__(self) -> None:

Check warning on line 19 in src/ostorlab/assets/ip.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ip.py#L19 

Added line #L19 was not covered by tests
if self.version is None:
self.version = ipaddress.ip_address(self.host).version

def __str__(self) -> str:
return f"{self.host}/{self.mask}"
9 changes: 3 additions & 6 deletions src/ostorlab/assets/ipv4.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,9 @@
class IPv4(asset.Asset):
"""IPv4 Address target asset."""

def __init__(
self, host: str, version: Optional[int] = 4, mask: Optional[str] = None
):
self.host = host
self.version = version
self.mask = mask
host: str
version: int = 4
mask: Optional[str] = None

Check warning on line 16 in src/ostorlab/assets/ipv4.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ipv4.py#L14-L16 

Added lines #L14 - L16 were not covered by tests

def __str__(self) -> str:
return f"{self.host}/{self.mask}"
Expand Down
9 changes: 3 additions & 6 deletions src/ostorlab/assets/ipv6.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,9 @@
class IPv6(asset.Asset):
"""IPv6 Address target asset."""

def __init__(
self, host: str, version: Optional[int] = 6, mask: Optional[str] = None
):
self.host = host
self.version = version
self.mask = mask
host: str
version: int = 6
mask: Optional[str] = None

Check warning on line 16 in src/ostorlab/assets/ipv6.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/ipv6.py#L14-L16 

Added lines #L14 - L16 were not covered by tests

def __str__(self) -> str:
return f"{self.host}/{self.mask}"
Expand Down
5 changes: 2 additions & 3 deletions src/ostorlab/assets/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@
class Link(asset.Asset):
"""Agent asset."""

def __init__(self, url: str, method: str):
self.url = url
self.method = method
url: str
method: str

Check warning on line 14 in src/ostorlab/assets/link.py

View check run for this annotation

Codecov / codecov/patch

src/ostorlab/assets/link.py#L13-L14 

Added lines #L13 - L14 were not covered by tests

def __str__(self) -> str:
return f"Link {self.url} with method {self.method}"
Expand Down
6 changes: 3 additions & 3 deletions tests/runtimes/definitions_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,13 +407,13 @@ def testAssetGroupDefinitionFromYaml_whenYamlIsValid_returnsValidAssetGroupDefin
content=b"content", path="/tests/files/fake_app.aab"
),
android_aab_asset.AndroidAab(
content=b"content", path="/tests/files/fake_app_2.apk"
content=b"content", path="/tests/files/fake_app_2.aab"
),
android_apk_asset.AndroidApk(
content=b"content", path="/tests/files/fake_app.aab"
content=b"content", path="/tests/files/fake_app.apk"
),
android_apk_asset.AndroidApk(
content=b"content", path="/tests/files/fake_app_2.aab"
content=b"content", path="/tests/files/fake_app_2.apk"
),
ios_ipa_asset.IOSIpa(content=b"content", path="/files/fake_app.ipa"),
ios_ipa_asset.IOSIpa(content_url="https://cia.sketchy.com/secret_files.ipa"),
Expand Down