Fix: Support iteration and indexing for ModelSimple list responses#109
Fix: Support iteration and indexing for ModelSimple list responses#109Xyerophyte wants to merge 3 commits into
Conversation
- Add __iter__, __len__, and enhanced __getitem__ to ModelSimple - Fixes ApiAttributeError when iterating DomainsResponse and similar models - Users can now use for loops, list comprehensions, indexing, and len() - Add comprehensive test coverage in test_model_simple_sequence.py - Update CHANGELOG.md for v12.0.0 Fixes issue where DomainsResponse._data_store had to be accessed directly to iterate over domain lists. Now DomainsResponse and other ModelSimple list-based responses behave like native Python sequences.
There was a problem hiding this comment.
Pull request overview
This PR updates the Python client’s ModelSimple base class to better support list-style response models (e.g., DomainsResponse) by implementing sequence-like behavior, and adds tests + changelog entry to validate and document the change.
Changes:
- Extend
ModelSimplewith sequence protocol support (__getitem__for int/slice, plus__iter__and__len__) backed by the underlyingvalue. - Add pytest coverage for iteration, indexing, slicing, and
len()onDomainsResponse. - Document the behavior in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
fastly/model_utils.py |
Adds sequence semantics to ModelSimple to make list-wrapping response models iterable/indexable. |
tests/test_model_simple_sequence.py |
Introduces tests verifying the new iteration/indexing/slicing/len behavior. |
CHANGELOG.md |
Notes the ModelSimple sequence behavior change under bug fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise ApiAttributeError( | ||
| "{0} has no attribute '{1}'".format( | ||
| type(self).__name__, name), | ||
| [e for e in [self._path_to_item, name] if e] | ||
| ) |
There was a problem hiding this comment.
In the new int/slice error path construction, the list comprehension uses if e, which will drop index 0 (because 0 is falsy). That can lose useful context for callers when model[0] fails. Consider filtering with is not None (or similar) so index 0 is preserved in the path.
| try: | ||
| return v[name] | ||
| except Exception: | ||
| # normalize to attribute error expected by callers | ||
| raise ApiAttributeError( | ||
| "{0} has no attribute '{1}'".format( | ||
| type(self).__name__, name), | ||
| [e for e in [self._path_to_item, name] if e] | ||
| ) | ||
|
|
There was a problem hiding this comment.
__getitem__ returns v[name] for int/slice names without verifying that value is actually a list/tuple. This is inconsistent with __iter__/__len__, which treat non-(list, tuple) value as a single item; e.g., len(model)==1 but model[0] can still raise. Consider limiting numeric indexing to list/tuple values, or explicitly handling index 0/slices for the single-item case so the sequence semantics stay consistent.
| try: | |
| return v[name] | |
| except Exception: | |
| # normalize to attribute error expected by callers | |
| raise ApiAttributeError( | |
| "{0} has no attribute '{1}'".format( | |
| type(self).__name__, name), | |
| [e for e in [self._path_to_item, name] if e] | |
| ) | |
| if isinstance(v, (list, tuple)): | |
| try: | |
| return v[name] | |
| except Exception: | |
| # normalize to attribute error expected by callers | |
| raise ApiAttributeError( | |
| "{0} has no attribute '{1}'".format( | |
| type(self).__name__, name), | |
| [e for e in [self._path_to_item, name] if e] | |
| ) | |
| if isinstance(name, slice): | |
| return [v][name] | |
| if name in (0, -1): | |
| return v | |
| raise ApiAttributeError( | |
| "{0} has no attribute '{1}'".format( | |
| type(self).__name__, name), | |
| [e for e in [self._path_to_item, name] if e] | |
| ) |
| def __len__(self): | ||
| """Return length of the underlying sequence if present, otherwise 0.""" | ||
| try: | ||
| v = self.__dict__['_data_store'].get('value') | ||
| except Exception: |
There was a problem hiding this comment.
Adding __len__ changes truthiness for ModelSimple instances (Python will use __len__ for bool(model) when __bool__ is not defined). For list-based responses this means empty responses become falsy, which can be a backward-incompatible behavioral change. If that’s not intended, consider defining __bool__ to preserve prior truthiness, or documenting this as a behavior change.
| # If underlying value is a sequence (list/tuple), return its length. | ||
| if isinstance(v, (list, tuple)): | ||
| return len(v) | ||
| # For dict or other single objects, treat as single item. | ||
| return 1 |
There was a problem hiding this comment.
The __len__ docstring says “otherwise 0”, but the implementation returns 1 for non-(list, tuple) values (treating them as a single item). Update the docstring to match the actual behavior.
| @@ -0,0 +1,38 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
pytest is imported but not used in this test file. Removing the unused import keeps the test module clean and avoids unused-import lint noise (if/when linting is enabled).
| import pytest |
| **Bug fixes:** | ||
|
|
||
| - fix(dictionary): Correct example dictionary name to use valid characters | ||
| - fix(model): Make `ModelSimple` behave like a sequence when `value` is a list/tuple so generated response models (e.g. `DomainsResponse`) can be iterated, indexed and sized. |
There was a problem hiding this comment.
The new changelog entry is indented with an extra leading space ( - fix(model): ...), which will render as a nested list item in Markdown. Align it with the other bullets under Bug fixes: (no leading space before -).
| - fix(model): Make `ModelSimple` behave like a sequence when `value` is a list/tuple so generated response models (e.g. `DomainsResponse`) can be iterated, indexed and sized. | |
| - fix(model): Make `ModelSimple` behave like a sequence when `value` is a list/tuple so generated response models (e.g. `DomainsResponse`) can be iterated, indexed and sized. |
Problem
The
DomainsResponseclass (and otherModelSimple-based list response models) could not be iterated, causingApiAttributeErrorwhen attempting common operations:Users had to access the internal
_data_storedirectly, breaking the abstraction.Root Cause
DomainsResponseinherits fromModelSimpleand stores a list in itsvalueattribute. When Python's iteration protocol calls__getitem__(0), the existing implementation only accepted string attribute names, not integer indices.Solution
Enhanced the
ModelSimpleclass infastly/model_utils.pywith sequence protocol support:__getitem__- Now accepts integers and slices, delegating to the underlyingvaluelist/tuple__iter__- Returns an iterator over the sequence invalue__len__- Returns the length of the sequence invalueThe implementation intelligently handles:
What Now Works
Testing
tests/test_model_simple_sequence.pyDocumentation
CHANGELOG.md(v12.0.0 bug fixes section)Impact
ModelSimpleinstances that wrap sequences