fix(pretty_export): stringify related objects when joining list values - #1112
fix(pretty_export): stringify related objects when joining list values#1112Sanjays2402 wants to merge 7 commits into
Conversation
_base_export_cell joined a relationship's formatted value with ",".join(formatted_value). The default formatter returns the related model instances themselves for an InstrumentedList, so exporting a column_list containing a to-many relationship raised TypeError: sequence item 0: expected str instance, Address found. Each item is now coerced with str() before joining, matching how the non-list relationship branch renders its value. Closes smithyhq#1111
There was a problem hiding this comment.
Pull request overview
Fixes PrettyExport._base_export_cell to correctly export to-many relationship values by coercing each formatted item to str before joining, preventing TypeError when the default formatter returns related model instances (e.g., SQLAlchemy InstrumentedList of Address objects).
Changes:
- Update relationship list rendering to
",".join(str(item) for item in formatted_value)to avoid joining non-strings. - Add a regression test covering exporting a
column_listthat includes a to-many relationship.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
sqladmin/pretty_export.py |
Coerces relationship list items to strings before joining to prevent export-time TypeError. |
tests/test_pretty_export.py |
Adds regression test verifying relationship list export no longer errors and returns a joined string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert len(values) == 3 | ||
| assert values[0] == 1 | ||
| assert values[1] == "John Doe" | ||
| assert values[2] == ",".join(str(address) for address in user.addresses) |
|
One more thing. This PR considers relationships rendered as QuerySelectField (scalar — uselist=False, a plain many-to-one). It does not fix relationships rendered as QuerySelectMultipleField. This PR works by keeping errors that fallback_form.validate() already reported. It depends on WTForms actually reporting one. For a collection relationship, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sqladmin/pretty_export.py:33
- The iterable-join decision is based on
formatted_value, not the underlying relationshipvalue. This can change behavior for to-one relationships if a custom formatter returns an iterable (e.g., dict/list/tuple): it will now be comma-joined instead of being passed through. Also, joining sets without sorting yields nondeterministic output order. Consider joining only when the relationshipvalueis a collection, and sort set values for stable exports.
if isinstance(formatted_value, str):
cell_value = formatted_value
elif isinstance(formatted_value, Iterable):
cell_value = ",".join(str(item) for item in formatted_value)
else:
| cell_value = ",".join(formatted_value) | ||
| if isinstance(formatted_value, str): | ||
| cell_value = formatted_value | ||
| elif isinstance(formatted_value, Iterable): |
There was a problem hiding this comment.
(list, set, tuple, frozenset)
Lets do this instead of Iterable. Apparently, formatted_value can be dict as well:
class Order(Base):
items = relationship("Item", collection_class=attribute_keyed_dict("sku"))There was a problem hiding this comment.
Even, do like this:
if isinstance(value, bool):
cell_value = "TRUE" if value else "FALSE"
elif isinstance(formatted_value, str):
cell_value = formatted_value
elif isinstance(formatted_value, (set, frozenset)):
cell_value = ",".join(sorted(str(item) for item in formatted_value))
elif isinstance(formatted_value, (list, tuple)):
cell_value = ",".join(str(item) for item in formatted_value)
else:
cell_value = formatted_valueBcuz, set and frozenset are unodered. and each export might give different set of items.
There was a problem hiding this comment.
Applied your ladder in d4b0691, with the explicit types instead of Iterable.
if isinstance(formatted_value, str):
cell_value = formatted_value
elif isinstance(formatted_value, (set, frozenset)):
cell_value = ",".join(sorted(str(item) for item in formatted_value))
elif isinstance(formatted_value, (list, tuple)):
cell_value = ",".join(str(item) for item in formatted_value)
else:
cell_value = formatted_valueI kept the bool check in the non-relationship branch where it already was rather than hoisting it above the relationship branch - a bool can't be a relationship value, so moving it only changes which branch a non-relationship bool takes, and it currently takes that one.
Two tests added: test_get_export_row_values_with_set_relationship_is_sorted (three tags inserted as vip/beta/alpha, asserts alpha,beta,vip, so it pins the ordering rather than just the membership) and test_get_export_row_values_with_dict_relationship using collection_class=attribute_keyed_dict("sku"). Both fail on the previous commit - the dict one used to come out as sku-1 because Iterable caught the dict and joined its keys.
One thing to decide: with the explicit tuple, an attribute_keyed_dict relationship now falls through to else and the raw dict reaches the writer, so the CSV cell is the dict repr. That is what your snippet does and it is at least not silently wrong the way key-joining was, but if you'd rather it render as ",".join(sorted(str(v) for v in formatted_value.values())) say so and I'll add the branch plus flip the test.
Also changed the _base_export_cell return annotation from str to Any - it was never accurate, the else arms return whatever the formatter gave (int, date, None), and now a dict too.
| elif isinstance(formatted_value, Iterable): | ||
| cell_value = ",".join(str(item) for item in formatted_value) | ||
| else: | ||
| cell_value = formatted_value |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Keyed collections (attribute_keyed_dict) aren't list/tuple/set/str, so they
fall through here and reach the CSV as a Python repr:
1,"Main St,Second St","alpha,vip","{'sku-1': <__main__.Item object at 0x7f55d58e03d0>, ...}"
Two runs of the same export over the same rows:
run 1: ...<__main__.Item object at 0x7f55d58e03d0>...
run 2: ...<__main__.Item object at 0x7fe63fae0390>...
Which is the opposite of the comment three lines up — sets became reproducible in
the same commit that left mappings not reproducible. It also leaks the module path
and ignores __str__ on the related model, which every other branch honours.
9a3e5a6 had this right via Iterable (it produced sku-1,sku-2). Narrowing away
from Iterable was a sound instinct, but could we name dict rather than let it
fall through?
elif isinstance(formatted_value, dict):
cell_value = ",".join(str(item) for item in formatted_value.values())
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_pretty_export.py:190
- This assertion assumes the
addressesrelationship is loaded in a deterministic order ("Main St" then "Second St"). Without anorder_byon the relationship, most databases do not guarantee row ordering, so this test can be flaky depending on the backend/query plan. Since the intent is to verify stringification/joining, make the assertion order-insensitive (or addorder_byto the relationship).
assert len(values) == 3
assert values[0] == 1
assert values[1] == "John Doe"
assert values[2] == "Main St,Second St"
| elif isinstance(formatted_value, Iterable): | ||
| cell_value = ",".join(str(item) for item in formatted_value) | ||
| else: | ||
| cell_value = formatted_value |
There was a problem hiding this comment.
Keyed collections (attribute_keyed_dict) aren't list/tuple/set/str, so they
fall through here and reach the CSV as a Python repr:
1,"Main St,Second St","alpha,vip","{'sku-1': <__main__.Item object at 0x7f55d58e03d0>, ...}"
Two runs of the same export over the same rows:
run 1: ...<__main__.Item object at 0x7f55d58e03d0>...
run 2: ...<__main__.Item object at 0x7fe63fae0390>...
Which is the opposite of the comment three lines up — sets became reproducible in
the same commit that left mappings not reproducible. It also leaks the module path
and ignores __str__ on the related model, which every other branch honours.
9a3e5a6 had this right via Iterable (it produced sku-1,sku-2). Narrowing away
from Iterable was a sound instinct, but could we name dict rather than let it
fall through?
elif isinstance(formatted_value, dict):
cell_value = ",".join(str(item) for item in formatted_value.values())
mmzeynalli
left a comment
There was a problem hiding this comment.
Perfect. Almost ready! We just need one more test)
| else: | ||
| if isinstance(value, bool): | ||
| cell_value = "TRUE" if value else "FALSE" |
There was a problem hiding this comment.
Test missing for this line.
What still lands here is a to-one relationship, and on a related model without
__str__ that exports as the same thing this commit just fixed for mappings:
column_export_list = ["id", "author", "publisher"]
Author has no __str__ -> <__main__.Author object at 0x7f...>
Publisher has __str__ -> P
CSV LINE -> 1,<__main__.Author object at 0x7f...>,P
Identical on current main, so not something this PR caused — but it's the same bug class,
so we might fix it in this PR
|
Thanks @mmzeynalli — that last test is in (ed1f827). The to-one fall-through now honours Added |
Closes #1111
PrettyExport._base_export_celljoined a relationship's formatted value with",".join(formatted_value), but for anInstrumentedListthe default formatter returns the related model instances themselves, so exporting acolumn_listcontaining a to-many relationship raisedTypeError: sequence item 0: expected str instance, Address found. Each item is now coerced withstr()before joining, matching how the non-list relationship branch renders its value.Regression test added in
tests/test_pretty_export.pybeside the existing_get_export_row_valuestests; it fails with the reported TypeError without the fix and passes with it.This change was prepared with AI assistance; the regression test was run locally and fails without the fix.