Skip to content

fix: _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info#5049

Open
Copilot wants to merge 6 commits into
mainfrom
copilot/fix-allowed-surface-ids-call
Open

fix: _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info#5049
Copilot wants to merge 6 commits into
mainfrom
copilot/fix-allowed-surface-ids-call

Conversation

Copilot AI commented Apr 2, 2026

Copy link
Copy Markdown
Contributor

_AllowedSurfaceIDs.__call__ silently swallowed KeyError/IndexError and returned None, causing a confusing downstream TypeError: argument of type 'NoneType' is not iterable in _SurfaceIds.validate.

Context

When a surface entry in _get_surfaces_info() is missing the surface_id key or has an empty surface_id list, the method caught both exceptions and fell off the end, returning None. Callers like _SurfaceIds.validate then failed with a TypeError that gave no indication of the actual root cause.

Change Summary

  • _AllowedSurfaceIDs.__call__: replaced the silent catch-all with a per-surface loop that raises a descriptive LookupError naming the offending surface and whether the failure was a missing key or empty index:
# Before
try:
    return [info["surface_id"][0] for _, info in surface_info.items()]
except (KeyError, IndexError):
    pass  # silently returns None

# After
for surface_name, info in surface_info.items():
    try:
        surface_ids.append(info["surface_id"][0])
    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}'.")
return surface_ids
  • Tests: added three unit tests covering the KeyError path, the IndexError (empty list) path, and propagation through _SurfaceIds.validate.

Rationale

Raising a LookupError with the surface name pinpoints the bad data immediately, rather than silently producing None and letting an unrelated TypeError surface elsewhere.

Impact

_AllowedSurfaceIDs.__call__ and any callers that previously received None (e.g., _SurfaceIds.validate, _get_surface_ids) will now get a clear LookupError instead of a silent None or a misleading TypeError.

@codacy-production

codacy-production Bot commented Apr 2, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes. Give us feedback

…ndexError

Agent-Logs-Url: https://github.com/ansys/pyfluent/sessions/068476b6-21c5-4c0a-a683-3096b157ad4f

Co-authored-by: Gobot1234 <50501825+Gobot1234@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix silent failures in _AllowedSurfaceIDs.__call__ method Fix _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info Apr 2, 2026
Copilot AI requested a review from Gobot1234 April 2, 2026 13:27
@Gobot1234 Gobot1234 marked this pull request as ready for review April 2, 2026 13:32
Copilot AI review requested due to automatic review settings April 2, 2026 13:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR improves error reporting in the field data surface-ID lookup path by ensuring malformed surface metadata triggers a descriptive exception at the source, rather than returning None and failing later with an unrelated TypeError.

Changes:

  • Updated _AllowedSurfaceIDs.__call__ to iterate per-surface and raise a descriptive LookupError on missing surface_id or empty surface_id list.
  • Added unit tests covering both malformed-surface-info cases and verifying _SurfaceIds.validate propagates the LookupError.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/ansys/fluent/core/field_data_interfaces.py Replace silent exception swallow/implicit None return with explicit per-surface LookupErrors containing the surface name and failure mode.
tests/test_field_data.py Add regression tests for missing surface_id key, empty surface_id list, and propagation through _SurfaceIds.validate.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Gobot1234 Gobot1234 enabled auto-merge (squash) April 8, 2026 10:36
@github-actions github-actions Bot added the bug Issue, problem or error in PyFluent label Apr 8, 2026
@Gobot1234 Gobot1234 changed the title Fix _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info fix: _AllowedSurfaceIDs.__call__ silently returning None on malformed surface info Apr 8, 2026
@Gobot1234 Gobot1234 closed this Apr 8, 2026
auto-merge was automatically disabled April 8, 2026 15:59

Pull request was closed

@Gobot1234 Gobot1234 reopened this Apr 8, 2026
@Gobot1234 Gobot1234 enabled auto-merge (squash) April 21, 2026 12:43
Copilot AI review requested due to automatic review settings April 23, 2026 10:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ansys/fluent/core/field_data_interfaces.py
Comment thread src/ansys/fluent/core/field_data_interfaces.py
surface_ids = []
for surface_name, info in surface_info.items():
try:
surface_ids.append(info["surface_id"][0])

@mkundu1 mkundu1 Apr 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If a user faces the below KeyError and IndexError, they will not know how to fix that error by reading the error-message. The key and the index are hardcoded values in the code, those are not supplied by the user.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

@mkundu1 mkundu1 Apr 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue, problem or error in PyFluent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential data state from _AllowedSurfaceIDs.__call__ returning None

6 participants