Skip to content

Conversation

1yam
Copy link
Member

@1yam 1yam commented Oct 2, 2025

fix: #236

and fix an issue where crn wouldn't get fetch on the crn list due to filter

Also include the fix from this other PR spotted by @RezaRahemtola (#236):

When deleting an instance, the behavior for the ports-forwarding aggregate is different between the CLI and the UI:

The problem is that the types in the SDK (and thus in the CLI) don't expect an item hash in the ports-forwarding aggregate to be null (as you can see, I just added the possibility in this PR).

This means that if a user deletes an instance in the UI, then the CLI won't function properly anymore, as it will throw Pydantic parsing errors when fetching the ports, for example when deleting an instance:
image

Unfortunately the fix is probably not as simple as adding this line:

  • Adding the possibility of null ports may (well is according to the tests) break some stuff in the SDK / CLI so it should be done carefully
  • Updating the UI to avoid setting null values is also out of the question imo, as it won't solve the issue for existing users (and after a quick look at the UI, it's using a generic part of the code to delete the ports so it would need a bit more than a single line change too)

@github-actions github-actions bot added the BLUE This PR is simple and straightforward. label Oct 2, 2025
Copy link

github-actions bot commented Oct 2, 2025

Summary:
This PR introduces a change to the AllForwarders type definition in the types.py file. The original type was RootModel[Dict[ItemHash, Ports]], and it has been updated to RootModel[Dict[ItemHash, Optional[Ports]]]. This change does not affect the functionality of the code but rather adds an optional type to the dictionary values, which is a common practice to handle cases where certain keys might not have associated Ports objects.

Highlight:

diff --git a/src/aleph/sdk/types.py b/src/aleph/sdk/types.py
index e76aedbc..b839a14b 100644
--- a/src/aleph/sdk/types.py
+++ b/src/aleph/sdk/types.py
@@ -309,7 +309,7 @@ class Ports(BaseModel):
     ports: Dict[int, PortFlags]
 
 
-AllForwarders = RootModel[Dict[ItemHash, Ports]]
+AllForwarders = RootModel[Dict[ItemHash, Optional[Ports]]]
 
 
 class DictLikeModel(BaseModel):

Explanation:
The change made in this PR is a straightforward type modification, adding an optional type to the dictionary values in the AllForwarders type definition. This does not introduce any new functionality or modify existing behavior. It is a refinement that improves type safety and ensures that the dictionary can handle cases where Ports might not be present for certain keys. This change is unlikely to introduce bugs and does not require a deep understanding of the codebase. Therefore, it is categorized as 'BLUE'.

@1yam 1yam changed the title fix: allow None type for ports when they are not set / removed by front Fix: filter on crn list & port-forwarding Oct 2, 2025
@1yam 1yam requested a review from nesitor October 2, 2025 11:28
Comment on lines 82 to 83
required_tokens: Decimal
payment_type: str
Copy link
Member

@nesitor nesitor Oct 6, 2025

Choose a reason for hiding this comment

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

Suggested change
required_tokens: Decimal
payment_type: str
required_tokens: Decimal
cost: Optional[str]
payment_type: str

I think that the cost field remains to stay here, as it's part of the response.

@1yam 1yam requested a review from nesitor October 7, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLUE This PR is simple and straightforward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants