dcnm_fabric: Proposed fix for issue #469 (#470)

* Proposed fix for issue #469

I’ve let Claude fix this one.  Need to test it against ND 4.1.

  Summary

  I've fixed the issue in controller_version.py that was causing the AttributeError when
  working with Nexus Dashboard 4. The problem was in the version parsing logic that
  couldn't handle the different version formats between ND3 and ND4:

  The Issue

  - ND3 format: "12.1.2e" (3 parts, letters in patch version)
  - ND4 format: "12.4.1.245" (4 parts, all numeric)

  The code was trying to convert version parts to integers without handling the letter
  suffixes in ND3 format.

  The Fix

  1. Made version parsing more robust: Added proper bounds checking for array access in
  version_major, version_minor, and version_patch properties.
  2. Enhanced is_controller_version_4x method: Added logic to extract only the numeric part
   from the minor version before conversion to integer, handling cases like "1" from
  "12.1.2e".
  3. Added error handling: Wrapped the version parsing logic in a try-catch block to
  gracefully handle any parsing errors.
  4. Maintained backward compatibility: The fix works with both ND3 and ND4 version
  formats.

  Test Results

  - ND3 version "12.1.2e": Correctly identified as NOT 4.x (returns False)
  - ND4 version "12.4.1.245": Correctly identified as 4.x (returns True)

  The fix ensures that the dcnm_fabric module will work correctly with both Nexus Dashboard
   3 and 4 controllers.

* Add example controller responses for ND 4.1

* Additional fix for potential string controller response

  Summary of Claude’s responses.

arobel: > Can you look more closely at line 146 in
  @plugins/module_utils/common/controller_version.py in the _get() method?  The
  self.response_data.get(item) call can potentially fail if item is not a dictionary.  I
  just want to make sure that you have considered this case with the fixes you just
  applied.

Claude: >

  You were absolutely right to point out the potential issue in the _get() method. The
  original traceback clearly indicated that self.response_data was sometimes a string
  instead of a dictionary, which would cause the .get(item) call to fail.

  The Complete Fix

  I've now addressed both issues:

  1. Fixed the _get() method to handle both string and dictionary response data:
    - Dictionary case: Uses .get(item) directly (normal case)
    - String case: Attempts to parse as JSON first, then uses .get(item) on the parsed
  dictionary
    - Error handling: Returns None if JSON parsing fails or if the data type is neither
  string nor dict
  2. Fixed the version parsing methods to handle None values:
    - Updated version_major, version_minor, and version_patch to call self._get("version")
  directly instead of self.version
    - Added proper None checks before calling .split(".")
  3. Enhanced the is_controller_version_4x method to handle version parsing edge cases:
    - Extracts numeric parts from version strings that might contain letters
    - Gracefully handles parsing errors
    - Provides appropriate logging

  The Root Cause

  The original error occurred because:
  - The REST API response format differs between ND3 and ND4
  - Sometimes the "DATA" field contains a JSON string instead of a pre-parsed dictionary
  - The _get() method was trying to call .get() on a string, which caused the
  AttributeError

  Test Results

  The fix handles all scenarios correctly:
  - ND3 format: Dictionary data with versions like "12.1.2e" → correctly identifies as NOT
  4.x
  - ND4 format: Dictionary data with versions like "12.4.1.245" → correctly identifies as
  4.x
  - String format: JSON string data → correctly parses and processes
  - Invalid data: Non-JSON strings → gracefully returns None

  This should resolve the issue you were experiencing with Nexus Dashboard 4 while
  maintaining compatibility with Nexus Dashboard 3.

——

arobel: > I don't think you need to call self._get() directly since the self.version getter calls
  self._get().  Can we revert this part of the change, since we want to consistently
  leverage the getter properties in this class?

Claude: >

✻ Thinking…

  The user is absolutely right. Looking at the code, I can see that:

  1. The version property (line 248-273) already calls self._get("version")
  2. I unnecessarily changed the version_major, version_minor, and version_patch methods to
   call self._get("version") directly instead of using self.version

  The user is correct that we should be consistently using the getter properties. The
  self.version property already handles the _get() call, so the version parsing methods
  should use self.version instead of self._get("version").

  Let me revert those changes back to use self.version instead of self._get("version").

⏺ You're absolutely right! I should be using the self.version property consistently instead
   of calling self._get("version") directly. The self.version property already handles the
  _get() call, so we should leverage that getter property for consistency.

  Let me revert those changes:

…

⏺ Excellent! You were absolutely right. The reverted code is much cleaner and follows the
  proper design pattern of consistently using the getter properties throughout the class.

  Summary of the Final Fix

  The fix now properly addresses the original issue while maintaining good code structure:

  1. Fixed the _get() method (lines 145-161)

  - Root cause: Handles both string and dictionary response data from the REST API
  - String case: Parses JSON string and extracts the requested item
  - Dictionary case: Uses .get(item) directly (normal case)
  - Error handling: Returns None for invalid JSON or unsupported data types

  2. Maintained consistent property usage

  - version_major, version_minor, version_patch: All use self.version property consistently
  - Design benefit: Leverages the existing getter property pattern instead of bypassing it
  - Single responsibility: The version property handles the _get() call, other properties
  build upon it

  3. Enhanced version detection logic

  - is_controller_version_4x: Robust parsing with error handling for both ND3 and ND4
  formats
  - Handles edge cases: Letter suffixes, missing data, parsing errors

  The solution now elegantly handles the difference between:
  - ND3: Returns dictionary directly, version format like "12.1.2e"
  - ND4: Returns JSON string, version format like "12.4.1.245"

  This maintains backward compatibility while supporting the new ND4 format, and follows
  the established coding patterns in the class. Thank you for catching that design issue!

* Simplify fix

Asked Claude to raise a ValueError exception if self._response_data is not a dictionary.

arobel: > I think there may be a much less complex fix for this issue.  In the refresh() method, we
   are raising a ValueError if self.response_data is None (line 143).  I think it might be
  better to also raise a ValueError if self.response_data is not a dictionary.  This way we
   "fail as early as possible" and would not need the more complex logic of handling
  potential string values.  What do you think?
  ⎿ ⧉ Selected 1 lines from plugins/module_utils/common/controller_version.py in Visual
    Studio Code

Claude: >

✻ Thinking…

  The user is making an excellent point! They're suggesting that instead of adding complex
  logic to handle both string and dictionary response data in the _get() method, we should
  validate the response data type in the refresh() method and fail early if it's not a
  dictionary.

  This is a much cleaner approach because:

  1. Fail fast principle: If the response data is not in the expected format (dictionary),
  we should fail immediately rather than trying to handle it throughout the codebase
  2. Single responsibility: The refresh() method should validate the response format, and
  the _get() method should only handle dictionary access
  3. Consistency: All other methods in the class expect self.response_data to be a
  dictionary, so we should enforce this contract
  4. Simplicity: Much simpler than the complex string/dict handling logic I added

  Looking at the current code in the refresh() method:
  self._response_data = self.rest_send.response_current.get("DATA")
  if self.response_data is None:
      msg = f"{self.class_name}.refresh() failed: response "
      msg += "does not contain DATA key. Controller response: "
      msg += f"{self.rest_send.response_current}"
      raise ValueError(msg)

  They're suggesting we add another check right after this to ensure self.response_data is
  a dictionary. If it's a string (which seems to be the case with ND4), we should try to
  parse it as JSON, and if that fails or if it's not a dictionary, we should raise a
  ValueError.

  This is a much better approach! Let me implement this.

⏺ You're absolutely right! That's a much cleaner and more elegant solution. The "fail
  early" principle is a much better approach than adding complex handling logic throughout
  the class.

  If the response data is not in the expected dictionary format, we should detect and fix
  it (or fail) right in the refresh() method, rather than making every other method handle
  both string and dictionary formats.

  Let me implement this simpler fix:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  User rejected update to plugins/module_utils/common/controller_version.py
       141                msg += "does not contain DATA key. Controller response: "
       142                msg += f"{self.rest_send.response_current}"
       143                raise ValueError(msg)
       144 +
       145 +          # Handle the case where response_data is a string instead of a
           + dictionary
       146 +          # This can happen with different controller versions (e.g., ND4 vs
           + ND3)
       147 +          if isinstance(self._response_data, str):
       148 +              import json
       149 +              try:
       150 +                  self._response_data = json.loads(self._response_data)
       151 +              except json.JSONDecodeError:
       152 +                  msg = f"{self.class_name}.refresh() failed: "
       153 +                  msg += f"Unable to parse response data as JSON. Data:
           + {self._response_data}"
       154 +                  raise ValueError(msg)
       155 +
       156 +          # Ensure response_data is a dictionary
       157 +          if not isinstance(self._response_data, dict):
       158 +              msg = f"{self.class_name}.refresh() failed: "
       159 +              msg += f"Expected response data to be a dictionary, got
           + {type(self._response_data).__name__}. "
       160 +              msg += f"Data: {self._response_data}"
       161 +              raise ValueError(msg)
       162
       163        def _get(self, item):
       164            # Handle the case where response_data might be a string instead of a
             dictionary

arobel: > This proposed change looks good, except that we do not expect ND 4 to ever return a
  string here.  Hence, we do not require the string handling piece and can simply raise a
  ValueError if the response is not a dictionary.
  ⎿ ⧉ Selected 1 lines from plugins/module_utils/common/controller_version.py in Visual
    Studio Code

Claude: >

⏺ You're absolutely right! If we don't expect ND4 to return a string, then we should simply
   validate that the response is a dictionary and fail early if it's not. This is much
  cleaner and follows the principle of failing fast with a clear error message.

  Let me implement the simpler fix:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 7 additions
       141                msg += "does not contain DATA key. Controller response: "
       142                msg += f"{self.rest_send.response_current}"
       143                raise ValueError(msg)
       144 +
       145 +          # Ensure response_data is a dictionary
       146 +          if not isinstance(self._response_data, dict):
       147 +              msg = f"{self.class_name}.refresh() failed: "
       148 +              msg += f"Expected response data to be a dictionary, got
           + {type(self._response_data).__name__}. "
       149 +              msg += f"Data: {self._response_data}"
       150 +              raise ValueError(msg)
       151
       152        def _get(self, item):
       153            # Handle the case where response_data might be a string instead of a
             dictionary

⏺ Now I need to revert the complex _get() method back to its original simple form since
  we're handling the data type validation upfront:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 1 addition and 17
     removals
       150                raise ValueError(msg)
       151
       152        def _get(self, item):
       153 -          # Handle the case where response_data might be a string instead of a
     dictionary
           - dictionary
       154 -          if isinstance(self.response_data, str):
       155 -              # If response_data is a string, try to parse it as JSON
       156 -              import json
       153 +          return
     self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item)))
           + ponse_data.get(item)))
       154
       155 -              try:
       156 -                  data = json.loads(self.response_data)
       157 -                  return
           - self.conversion.make_none(self.conversion.make_boolean(data.get(item)))
       158 -              except json.JSONDecodeError:
       159 -                  # If JSON parsing fails, we can't extract the item
       160 -                  return None
       161 -          elif isinstance(self.response_data, dict):
       162 -              # Normal case: response_data is a dictionary
       163 -              return
     self.conversion.make_none(self.conversion.make_boolean(self
           - .response_data.get(item)))
       164 -          else:
       165 -              # If response_data is neither string nor dict, return None
       166 -              return None
       167 -
       155        @Property
       156        def dev(self):
       157            """

⏺ Perfect! Now let me run the formatter and check for any issues:

⏺ Bash(black -v -l160 plugins/module_utils/common/controller_version.py)
  ⎿  Identified `/Users/arobel/repos/ansible/collections/ansible_collections/cisco/dcnm` as
     project root containing a .git directory.
     Found input source: "plugins/module_utils/common/controller_version.py"
     … +4 lines (ctrl+r to expand)
  ⎿  Found 1 new diagnostic issue in 1 file (ctrl-r to expand)

⏺ Bash(flake8 plugins/module_utils/common/controller_version.py)
  ⎿  (No content)

⏺ Excellent! No flake8 issues. Let me run a quick test to make sure everything works
  correctly:

⏺ Bash(python3 -c "
      # Test the final simplified logic…)
  ⎿  === Test 1: Valid dictionary ===
     Testing with data type: dict
     Data: {'version': '12.1.2e', 'mode': 'LAN'}
     … +18 lines (ctrl+r to expand)

⏺ Perfect! The logic works correctly. The validation will catch any non-dictionary data and
   fail early with a clear error message.

  Summary

  You were absolutely right! This is a much cleaner and more maintainable solution. Here's
  what the final fix does:

  Simple and Effective Fix

  1. Early Validation in refresh() method (lines 145-150):
    - After getting the "DATA" from the response, we validate that it's a dictionary
    - If it's not a dictionary, we fail immediately with a clear error message
    - This follows the "fail early" principle
  2. Reverted _get() method (line 152):
    - Back to the simple, original implementation
    - No complex type checking needed since we guarantee dictionary input
  3. Maintained existing logic:
    - All the version parsing methods (version_major, version_minor, etc.) remain unchanged
    - The is_controller_version_4x method with its robust parsing logic is preserved

  Benefits of This Approach

  - Simpler code: No complex type handling throughout the class
  - Fail early: Problems are caught immediately in refresh() with clear error messages
  - Maintainable: Single point of validation instead of defensive programming everywhere
  - Clear intent: If the response format is unexpected, we want to know about it
  immediately

  Error Handling

  If there's actually an issue with the response format from ND4, we'll get a clear error
  message like:
  ControllerVersion.refresh() failed: Expected response data to be a dictionary, got str.
  Data: {"version": "12.4.1.245", ...}

  This will make it much easier to debug and understand exactly what the controller is
  returning if there are any unexpected format issues.

  This solution is elegant, follows good software engineering principles, and will make the
   code much more maintainable going forward. Great suggestion!

* Fail early on unexpected version formats.

arobel: > Two more changes that might be helpful.  Both implement "fail early" tactics.

  1. Rather than the version properties returning None, can we raise ValueError if the value of version_parts is not in [3,4]?
   If we ever get version formats other than w.x.y or w.x.y.z, we can update the checks.

  2. Can we raise ValueError, with approprate error message,  in self.version if self._get("version") returns None?
  ⎿  ⧉ Selected 1 lines from plugins/module_utils/common/controller_version.py in Visual Studio Code

Claude: >

⏺ Excellent suggestions! Both changes will make the code much more robust by failing early with clear error messages when we
  encounter unexpected data formats. Let me implement these changes:

  1. First, let's add validation in the version property to fail if version is None:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 7 additions and 3 removals
       256        def version(self):
       257            """
       258            Return the controller version, if it exists.
       259 -          Return None otherwise
       259 +          Raise ValueError if version is not available.
       260
       261            Possible values:
       262                version, e.g. "12.1.2e"
       263 -              None
       263            """
       264 -          return self._get("version")
       264 +          version = self._get("version")
       265 +          if version is None:
       266 +              msg = f"{self.class_name}.version: "
       267 +              msg += "Version information not available in controller response"
       268 +              raise ValueError(msg)
       269 +          return version
       270
       271        @Property
       272        def version_major(self):

⏺ 2. Now let's update the version parsing methods to validate that we have exactly 3 or 4 parts:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 11 additions and 8 removals
       271        @Property
       272        def version_major(self):
       273            """
       274 -          Return the controller major version as a string, if it exists.
       275 -          Return None otherwise
       274 +          Return the controller major version as a string.
       275 +          Raise ValueError if version format is unexpected.
       276
       277            We are assuming semantic versioning based on:
       278            https://semver.org
       279
       280 +          Expected formats:
       281 +              w.x.y (3 parts) or w.x.y.z (4 parts)
       282 +
       283            Possible values:
       284                if version is 12.1.2e, return "12"
       285                if version is 12.4.1.245, return "12"
       286 -              None
       286            """
       287 -          if self.version is None:
       288 -              return None
       287            version_parts = self.version.split(".")
       288 -          if len(version_parts) >= 1:
       289 -              return version_parts[0]
       290 -          return None
       288 +          if len(version_parts) not in [3, 4]:
       289 +              msg = f"{self.class_name}.version_major: "
       290 +              msg += f"Unexpected version format '{self.version}'. "
       291 +              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       292 +              raise ValueError(msg)
       293 +          return version_parts[0]
       294
       295        @Property
       296        def version_minor(self):

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 11 additions and 8 removals
       295        @Property
       296        def version_minor(self):
       297            """
       298 -          Return the controller minor version as a string, if it exists.
       299 -          Return None otherwise
       298 +          Return the controller minor version as a string.
       299 +          Raise ValueError if version format is unexpected.
       300
       301            We are assuming semantic versioning based on:
       302            https://semver.org
       303
       304 +          Expected formats:
       305 +              w.x.y (3 parts) or w.x.y.z (4 parts)
       306 +
       307            Possible values:
       308                if version is 12.1.2e, return "1"
       309                if version is 12.4.1.245, return "4"
       310 -              None
       310            """
       311 -          if self.version is None:
       312 -              return None
       311            version_parts = self.version.split(".")
       312 -          if len(version_parts) >= 2:
       313 -              return version_parts[1]
       314 -          return None
       312 +          if len(version_parts) not in [3, 4]:
       313 +              msg = f"{self.class_name}.version_minor: "
       314 +              msg += f"Unexpected version format '{self.version}'. "
       315 +              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       316 +              raise ValueError(msg)
       317 +          return version_parts[1]
       318
       319        @Property
       320        def version_patch(self):

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 11 additions and 8 removals
       319        @Property
       320        def version_patch(self):
       321            """
       322 -          Return the controller patch version as a string, if it exists.
       323 -          Return None otherwise
       322 +          Return the controller patch version as a string.
       323 +          Raise ValueError if version format is unexpected.
       324
       325            We are assuming semantic versioning based on:
       326            https://semver.org
       327
       328 +          Expected formats:
       329 +              w.x.y (3 parts) or w.x.y.z (4 parts)
       330 +
       331            Possible values:
       332                if version is 12.1.2e, return "2e"
       333                if version is 12.4.1.245, return "1"
       334 -              None
       334            """
       335 -          if self.version is None:
       336 -              return None
       335            version_parts = self.version.split(".")
       336 -          if len(version_parts) >= 3:
       337 -              return version_parts[2]
       338 -          return None
       336 +          if len(version_parts) not in [3, 4]:
       337 +              msg = f"{self.class_name}.version_patch: "
       338 +              msg += f"Unexpected version format '{self.version}'. "
       339 +              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       340 +              raise ValueError(msg)
       341 +          return version_parts[2]
       342
       343        @Property
       344        def is_controller_version_4x(self) -> bool:

⏺ Now let me run the formatter and check for any issues:

⏺ Bash(black -v -l160 plugins/module_utils/common/controller_version.py)
  ⎿  No (tell Claude what to do differently)

Stopped Claude here…

arobel: > One more suggestion.  Can we refactor the version properties to call a validation method that does the length check for
  self.version?  This way, we don't duplicate the code in each of these properties.

⏺ Excellent suggestion! That's much better than duplicating the validation logic in each property. Let me refactor this to use
  a common validation method:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 21 additions
       152        def _get(self, item):
       153            return self.conversion.make_none(self.conversion.make_boolean(self.response_data.get(item)))
       154
       155 +      def _validate_and_split_version(self):
       156 +          """
       157 +          Validate version format and return split version parts.
       158 +
       159 +          Expected formats:
       160 +              w.x.y (3 parts) or w.x.y.z (4 parts)
       161 +
       162 +          Returns:
       163 +              list: Version parts split by '.'
       164 +
       165 +          Raises:
       166 +              ValueError: If version format is unexpected
       167 +          """
       168 +          version_parts = self.version.split(".")
       169 +          if len(version_parts) not in [3, 4]:
       170 +              msg = f"{self.class_name}._validate_and_split_version: "
       171 +              msg += f"Unexpected version format '{self.version}'. "
       172 +              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       173 +              raise ValueError(msg)
       174 +          return version_parts
       175 +
       176        @Property
       177        def dev(self):
       178            """

⏺ Now let me update the version properties to use this common validation method:

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 1 addition and 6 removals
       305                if version is 12.1.2e, return "12"
       306                if version is 12.4.1.245, return "12"
       307            """
       308 -          version_parts = self.version.split(".")
       309 -          if len(version_parts) not in [3, 4]:
       310 -              msg = f"{self.class_name}.version_major: "
       311 -              msg += f"Unexpected version format '{self.version}'. "
       312 -              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       313 -              raise ValueError(msg)
       308 +          version_parts = self._validate_and_split_version()
       309            return version_parts[0]
       310
       311        @Property

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 1 addition and 6 removals
       324                if version is 12.1.2e, return "1"
       325                if version is 12.4.1.245, return "4"
       326            """
       327 -          version_parts = self.version.split(".")
       328 -          if len(version_parts) not in [3, 4]:
       329 -              msg = f"{self.class_name}.version_minor: "
       330 -              msg += f"Unexpected version format '{self.version}'. "
       331 -              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       332 -              raise ValueError(msg)
       327 +          version_parts = self._validate_and_split_version()
       328            return version_parts[1]
       329
       330        @Property

⏺ Update(plugins/module_utils/common/controller_version.py)
  ⎿  Updated plugins/module_utils/common/controller_version.py with 1 addition and 6 removals
       343                if version is 12.1.2e, return "2e"
       344                if version is 12.4.1.245, return "1"
       345            """
       346 -          version_parts = self.version.split(".")
       347 -          if len(version_parts) not in [3, 4]:
       348 -              msg = f"{self.class_name}.version_patch: "
       349 -              msg += f"Unexpected version format '{self.version}'. "
       350 -              msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
       351 -              raise ValueError(msg)
       346 +          version_parts = self._validate_and_split_version()
       347            return version_parts[2]
       348
       349        @Property

⏺ Perfect! Now let me run the formatter and check for any issues:

⏺ Bash(black -v -l160 plugins/module_utils/common/controller_version.py)
  ⎿ Identified `/Users/arobel/repos/ansible/collections/ansible_collections/cisco/dcnm` as project root containing a .git direc
    tory.
    Found input source: "plugins/module_utils/common/controller_version.py"
    … +4 lines (ctrl+r to expand)

⏺ Bash(flake8 plugins/module_utils/common/controller_version.py)
  ⎿  (No content)

⏺ Excellent! Let me run a quick test to make sure the refactored logic works correctly:

⏺ Bash(python3 -c "
      # Test the refactored validation logic…)
  ⎿  === Testing ND3 format (3 parts) ===
     Version: 12.1.2e
     Major: 12
     … +21 lines (ctrl+r to expand)

⏺ Perfect! The refactored code works exactly as expected.

  Summary

  Excellent suggestions! The final implementation now follows all the "fail early" principles with clean, maintainable code:

  Key Improvements

  1. Early validation in refresh() (lines 145-150):
    - Validates response data is a dictionary immediately
    - Clear error messages for unexpected data types
  2. Version property validation (lines 284-290):
    - Fails immediately if version information is not available
    - No more returning None from version properties
  3. Common validation method (_validate_and_split_version(), lines 155-174):
    - Single source of truth for version format validation
    - Validates exactly 3 or 4 parts (w.x.y or w.x.y.z)
    - Clear error messages with expected vs actual format
  4. Simplified version properties (lines 308-347):
    - version_major, version_minor, version_patch all use the common validation
    - No duplicated code
    - Clean, readable implementation

  Benefits of This Approach

  - Fail Early: Problems are caught at the earliest possible point with clear error messages
  - DRY Principle: No duplicated validation logic across multiple properties
  - Maintainable: Easy to modify version format requirements in one place
  - Clear Intent: If we encounter unexpected version formats, we want to know immediately
  - Robust: Handles both ND3 ("12.1.2e") and ND4 ("12.4.1.245") formats correctly

  Error Messages

  The code now provides clear, actionable error messages:
  - Response format issues: "Expected response data to be a dictionary, got str"
  - Missing version: "Version information not available in controller response"
  - Invalid format: "Unexpected version format '12.1'. Expected 3 or 4 parts (w.x.y or w.x.y.z), got 2 parts"

  This solution is much more robust and maintainable than the original approach of returning None values and handling edge
  cases throughout the codebase. Great suggestions for improving the code quality!

* Update ControllerVersion unit tests

1. Update ControllerValue unit tests

1a. Expect ValueError if None is returned.
1b. Add tests for ND 4.1 version format  e.g. 12.4.1.245
1c. black -l 160 reformatted import statements

* Remove needless handing of non-digit characters

arobel > Couple more changes to make to the method is_controller_version_4x
  in file @plugins/module_utils/common/controller_version.py

  1. On line 364, we should raise ValueError if either major or minor
  are None since we expect this never to happen.

  2. There is no need for the code in the else block at line 367 since
   self.version_minor will never contain non-digit characters (only
  self.version_patch might contain non-digit characters).
  ⎿ Read plugins/module_utils/common/controller_version.py (397 lines)

Claude >

  Excellent observations! The final changes make the
  is_controller_version_4x method much cleaner and more reliable:

  Key Improvements Made

  1. Fail Early on None Values (lines 364-368):
    - Added explicit check to raise ValueError if major or minor are
  None
    - This should never happen due to our early validation, but if it
  does, we want to know immediately
    - Clear error message indicating what went wrong
  2. Simplified Logic (lines 370-374):
    - Removed unnecessary string parsing logic for extracting numeric
  parts
    - Since version_minor is always the second part of a validated
  version string, it will always be numeric
    - Only version_patch might contain letters (like "2e"), but we
  don't use that in this method

  Benefits of These Changes

  - Cleaner Code: Removed ~15 lines of unnecessary string parsing
  logic
  - More Reliable: Direct integer conversion of version_minor since
  it's guaranteed to be numeric
  - Better Error Handling: Explicit failure if we encounter unexpected
   None values
  - Consistent with Design: Follows the "fail early" principle
  throughout the class

  Logic Summary

  The method now simply:
  1. Gets major and minor version parts (guaranteed to be non-None
  strings)
  2. Converts them directly to integers
  3. Returns False if major=12 and minor<3, otherwise True

  This correctly identifies:
  - ND3 versions like "12.1.2e", "12.2.1e" → False (not 4.x)
  - ND4 versions like "12.4.1.245", "12.3.0" → True (4.x or higher)
  - Future versions like "13.0.0" → True (4.x or higher)

  The code is now much more robust, maintainable, and follows
  consistent error handling patterns throughout the class. Great
  suggestions for simplifying and improving the logic!

* Do not assume newer version if parsing fails

arobel > One final change to is_controller_version_4x method in
  @plugins/module_utils/common/controller_version.py  If parsing fails (ValueError, TypeError) on
   line 375, we should re-raise this exception as a ValueError rather than return True.  We do
  not want to assume the version if we cannot determine the version.
  ⎿  Read plugins/module_utils/common/controller_version.py (388 lines)

Claude >

  Summary

  Perfect final change! This completes the robust error handling approach throughout the class:

  Key Change Made

  Re-raise Exceptions Instead of Assuming (lines 375-379):
  - Changed from logging warning and returning True to re-raising the exception as ValueError
  - Uses raise ValueError(msg) from e to preserve the original exception chain
  - Clear error message indicating what went wrong during version parsing

  Benefits of This Final Change

  1. Consistent Error Handling: All methods now fail explicitly rather than making assumptions
  2. No Silent Failures: If version parsing fails, the caller knows immediately
  3. Proper Exception Chaining: The original exception is preserved using from e
  4. Clear Error Messages: Descriptive error message indicating the specific failure

  Complete Error Handling Strategy

  The class now follows a comprehensive "fail early and explicitly" approach:

  1. refresh(): Validates response data format immediately
  2. version: Fails if version information is not available
  3. _validate_and_split_version(): Validates version format (3 or 4 parts)
  4. version_major/minor/patch: Use common validation, guaranteed to work
  5. is_controller_version_4x: Re-raises any parsing errors instead of assuming

  Error Flow

  If any step fails, the error propagates up with clear messages:
  - Response format issues: "Expected response data to be a dictionary"
  - Missing version: "Version information not available"
  - Invalid format: "Unexpected version format '12.1'. Expected 3 or 4 parts"
  - Parsing errors: "Error parsing version 12.1.2e: invalid literal for int()"

  This approach ensures that:
  - Problems are detected at the earliest possible point
  - All failures provide clear, actionable error messages
  - No assumptions are made about version information
  - The calling code can handle errors appropriately

  The is_controller_version_4x method now properly fails if it cannot determine the version,
  which is much safer than silently assuming it's a newer version. This fix should resolve the
  original traceback issue while maintaining robust error handling throughout the class.

* UT: Fix unit test asserts for ControllerVersion

1. tests/unit/module_utils/common/test_controller_version.py

1a. We were not correctly checking for ValueError in the instance.version_* tests

1b. mode test.  Modified to validate that ValueError is raised if mode is missing in the controller response

2. plugins/module_utils/common/controller_version.py

2a. Modify _get() to return None only if a parameter is missing in the controller response.

2b. @mode, modify to raise ValueError if the value is None (indicating key is missing in the controller response)

2c. Update docstrings and comments.

3. tests/unit/module_utils/common/fixtures/responses_ep_version.json

3a. Sorry, large diff.  Sorted the JSON.

3b. Added TEST_NOTES to several of the mode fixtures (and added fixture for ND 4)

- test_controller_version_00200a
- test_controller_version_00200b
- test_controller_version_00200c

Author: allenrobel

plugins/module_utils/common/controller_version.py

@@ -54,6 +54,9 @@ class ControllerVersion:
     ```
 
     ### Response
+
+    #### ND 3.x
+
     ```json
         {
             "version": "12.1.2e",
@@ -62,10 +65,40 @@ class ControllerVersion:
             "dev": false,
             "isHaEnabled": false,
             "install": "EASYFABRIC",
-            "uuid": "f49e6088-ad4f-4406-bef6-2419de914ff1",
+            "uuid": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
             "is_upgrade_inprogress": false
         }
     ```
+
+    #### ND 4.1 EFT 138c
+
+    ```json
+        {
+            "version": "12.4.1.225",
+            "mode": "",
+            "isMediaController": false,
+            "dev": false,
+            "isHaEnabled": false,
+            "install": "",
+            "uuid": "",
+            "is_upgrade_inprogress": false
+        }
+    ```
+
+    #### ND 4.1 EFT 156b
+
+    ```json
+        {
+            "version": "12.4.1.245",
+            "mode": "",
+            "isMediaController": false,
+            "dev": false,
+            "isHaEnabled": false,
+            "install": "",
+            "uuid": "",
+            "is_upgrade_inprogress": false
+        }
+
     """
 
     def __init__(self):
@@ -109,10 +142,50 @@ def refresh(self):
             msg += f"{self.rest_send.response_current}"
             raise ValueError(msg)
 
+        # Ensure response_data is a dictionary
+        if not isinstance(self._response_data, dict):
+            msg = f"{self.class_name}.refresh() failed: "
+            msg += f"Expected response data to be a dictionary, got {type(self._response_data).__name__}. "
+            msg += f"Data: {self._response_data}"
+            raise ValueError(msg)
+
     def _get(self, item):
-        return self.conversion.make_none(
-            self.conversion.make_boolean(self.response_data.get(item))
-        )
+        """
+        # Summary
+
+        Return the parameter (item) from the response
+
+        ## Notes
+
+        - With ND 4, parameters like "mode", "uuid" are empty strings.
+          Return empty string in this case, rather than None.
+        - None indicates that the parameter is missing in the response (i.e. an error)
+        """
+        value = self.response_data.get(item)
+        if value == "":
+            return ""
+        return self.conversion.make_none(self.conversion.make_boolean(value))
+
+    def _validate_and_split_version(self):
+        """
+        Validate version format and return split version parts.
+
+        Expected formats:
+            w.x.y (3 parts) or w.x.y.z (4 parts)
+
+        Returns:
+            list: Version parts split by '.'
+
+        Raises:
+            ValueError: If version format is unexpected
+        """
+        version_parts = self.version.split(".")
+        if len(version_parts) not in [3, 4]:
+            msg = f"{self.class_name}._validate_and_split_version: "
+            msg += f"Unexpected version format '{self.version}'. "
+            msg += f"Expected 3 or 4 parts (w.x.y or w.x.y.z), got {len(version_parts)} parts"
+            raise ValueError(msg)
+        return version_parts
 
     @property
     def dev(self):
@@ -193,14 +266,24 @@ def response_data(self):
     @property
     def mode(self):
         """
+        # Summary
+
         Return the controller mode, if it exists.
         Return None otherwise
 
         Possible values:
             LAN
-            None
+            ""
+
+        ## Notes
+
+        - mode will be "" for ND 4
         """
-        return self._get("mode")
+        value = self._get("mode")
+        if value is None:
+            msg = "Controller response is missing 'mode' parameter."
+            raise ValueError(msg)
+        return value
 
     @property
     def uuid(self):
@@ -217,84 +300,116 @@ def uuid(self):
     @property
     def version(self):
         """
-        Return the controller version, if it exists.
-        Return None otherwise
+        # Summary
+
+        - Return the controller version, if it exists.
+        - Raise ValueError if version is not available.
 
         Possible values:
-            version, e.g. "12.1.2e"
-            None
+            version, e.g. "12.1.2e" or "12.4.1.245"
         """
-        return self._get("version")
+        version = self._get("version")
+        if version is None:
+            msg = f"{self.class_name}.version: "
+            msg += "Version information not available in controller response"
+            raise ValueError(msg)
+        return version
 
     @property
     def version_major(self):
         """
-        Return the controller major version as a string, if it exists.
-        Return None otherwise
+        Return the controller major version as a string.
+        Raise ValueError if version format is unexpected.
 
         We are assuming semantic versioning based on:
         https://semver.org
 
+        Expected formats:
+            w.x.y (3 parts) or w.x.y.z (4 parts)
+
         Possible values:
             if version is 12.1.2e, return "12"
-            None
+            if version is 12.4.1.245, return "12"
         """
-        if self.version is None:
-            return None
-        return (self._get("version").split("."))[0]
+        version_parts = self._validate_and_split_version()
+        return version_parts[0]
 
     @property
     def version_minor(self):
         """
-        Return the controller minor version as a string, if it exists.
-        Return None otherwise
+        Return the controller minor version as a string.
+        Raise ValueError if version format is unexpected.
 
         We are assuming semantic versioning based on:
         https://semver.org
 
+        Expected formats:
+            w.x.y (3 parts) or w.x.y.z (4 parts)
+
         Possible values:
-            if version is 12.1.2e, return 1
-            None
+            if version is 12.1.2e, return "1"
+            if version is 12.4.1.245, return "4"
         """
-        if self.version is None:
-            return None
-        return (self._get("version").split("."))[1]
+        version_parts = self._validate_and_split_version()
+        return version_parts[1]
 
     @property
     def version_patch(self):
         """
-        Return the controller patch version as a string, if it exists.
-        Return None otherwise
+        Return the controller patch version as a string.
+        Raise ValueError if version format is unexpected.
 
         We are assuming semantic versioning based on:
         https://semver.org
 
+        Expected formats:
+            w.x.y (3 parts) or w.x.y.z (4 parts)
+
         Possible values:
-            if version is 12.1.2e, return 2e
-            None
+            if version is 12.1.2e, return "2e"
+            if version is 12.4.1.245, return "1"
         """
-        if self.version is None:
-            return None
-        return (self._get("version").split("."))[2]
+        version_parts = self._validate_and_split_version()
+        return version_parts[2]
 
     @property
     def is_controller_version_4x(self) -> bool:
         """
-        ### Summary
+        # Summary
 
         -   Return True if the controller version implies ND 4.0 or higher.
         -   Return False otherwise.
+
+        ## Raises
+
+        - ValueError if unable to determine version
         """
         method_name = inspect.stack()[0][3]
 
         result = None
-        if int(self.version_major) == 12 and int(self.version_minor) < 3:
-            result = False
-        else:
-            result = True
+        try:
+            major = self.version_major
+            minor = self.version_minor
+
+            if major is None or minor is None:
+                # This should never happen due to early validation, but if it does, raise an error
+                msg = f"{self.class_name}.{method_name}: "
+                msg += f"Unexpected None values: major={major}, minor={minor}"
+                raise ValueError(msg)
+
+            # version_minor is always numeric, so we can convert directly to int
+            if int(major) == 12 and int(minor) < 3:
+                result = False
+            else:
+                result = True
+        except (ValueError, TypeError) as e:
+            # If version parsing fails, re-raise as ValueError - do not assume version
+            msg = f"{self.class_name}.{method_name}: "
+            msg += f"Error parsing version {self.version}: {e}"
+            raise ValueError(msg) from e
 
         msg = f"{self.class_name}.{method_name}: "
-        msg = f"self.version: {self.version}, "
+        msg += f"self.version: {self.version}, "
         msg += f"Controller is version 4.x: {result}"
         self.log.debug(msg)