-
Notifications
You must be signed in to change notification settings - Fork 75
fix: _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info #5049
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
e203cf8
5a01cea
6cea7c4
a0a73fa
d3d2c3e
e1225de
a53e890
968d069
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -568,13 +568,20 @@ def valid_name(self, surface_name: str) -> str: | |
|
|
||
| class _AllowedSurfaceIDs(_AllowedNames): | ||
| def __call__(self, respect_data_valid: bool = True) -> List[int]: | ||
| try: | ||
| return [ | ||
| info["surface_id"][0] | ||
| for _, info in self._field_info._get_surfaces_info().items() | ||
| ] | ||
| except (KeyError, IndexError): | ||
| pass | ||
| surface_info = self._field_info._get_surfaces_info() | ||
| surface_ids = [] | ||
| for surface_name, info in surface_info.items(): | ||
| try: | ||
| surface_ids.append(info["surface_id"][0]) | ||
|
Collaborator
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. If a user faces the below Is there a real scenario where these errors can happen due to some user error? If the errors are due to some implementation logic (either in server or client), then that should be handled in the code without raising user-facing errors.
Collaborator
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. No I don't think there are I'm pretty sure you'd only hit this in an implementation failure. How would you handle this in the code in this case?
Collaborator
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. If we can ignore the missing key/index, we can simply return an empty list. If we cannot ignore the missing key/index, that means an implementation issue. In that case we can do a debug-log for now and return an empty list. Ideally, the implementation should be fixed. @prmukherj Please guide which of the above scenarios is the case here.
Collaborator
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. @Gobot1234, you are right that this issue can happen only due to an implementation problem. You can follow @mkundu1's first suggestion as this was just an extra precautionary check. Let's re-run the tests and check for test failures after this change and decide. |
||
| except KeyError: | ||
| raise LookupError( | ||
| f"Failed to retrieve valid surface_id key for surface '{surface_name}'." | ||
| ) | ||
| except IndexError: | ||
| raise LookupError( | ||
| f"Failed to retrieve valid surface_id index for surface '{surface_name}'." | ||
|
Gobot1234 marked this conversation as resolved.
Outdated
|
||
| ) | ||
| return surface_ids | ||
|
|
||
|
|
||
| class FieldUnavailable(RuntimeError): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.