Skip to content

fix: guard attrLookup lookup against unmapped attribute types - #124

Open
CptOrange16 wants to merge 1 commit into
cedadev:masterfrom
CptOrange16:bugfix/chkattribute-list-type-keyerror
Open

CptOrange16 wants to merge 1 commit into
cedadev:masterfrom
CptOrange16:bugfix/chkattribute-list-type-keyerror

Conversation

@CptOrange16

Copy link
Copy Markdown

Problem

Related to issue #123

chkAttribute crashes with an uncaught KeyError when a standard CF
attribute (e.g. comment, flag_meanings) has a value that netCDF4
reads back as a Python list rather than a single string or numeric
value.

Root cause

In the type-detection block, if a value doesn't match any of the
expected types (string, numeric, ndarray, None), attrType is left
as the raw Python type object (e.g. <class 'list'>) instead of being
mapped to a reportable type code:

else:
    self._add_info("Invalid Type for attribute: %s %s" % (attribute, attrType))
    # attrType stays as <class 'list'>

That unmapped value then reaches:

attrLookup = {"D": "Data Variable", "N": "Numeric", "S": "String"}
...
attrLookup[attrType])   # KeyError: <class 'list'>

which crashes the entire checker run instead of reporting a normal
validation error.

Fix

Use .get(attrType, str(attrType)) instead of a direct [attrType]
lookup, so an unrecognized type degrades to a readable error message
(e.g. got '<class 'list'>' type) instead of raising.

Test added

tests/test_chkattribute_list_type_crash.py builds a minimal
in-memory netCDF file with a list-valued comment attribute and
asserts:

  1. checker.checker(...) completes without raising.
  2. An ERROR is recorded against the offending attribute.

Verified: fails with the original KeyError on master, passes after
this patch.

Scope note

A list-valued flag_meanings attribute triggers a separate crash
further downstream, in chkFlags -> extendedBlankSeparatedList
(which also assumes a string and calls re.match directly on the
value). Same class of bug, different function — deliberately left out
of this PR to keep it focused. Happy to open a follow-up if useful.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants