fix: CSVImport bugs (AUTH column, case-insensitive validation, username case) + QuerysetEndpoint.find_by_name - #1811
Open
jacalata wants to merge 7 commits into
Open
Conversation
4 tasks
…servation; add find_by_name Fixes for UserItem.CSVImport (issue #1809): - MAX=8 (was 7=AUTH index): 8-column lines with auth type no longer rejected as "too many columns" - create_user_from_line no longer lowercases the whole line before splitting — username case is preserved - _validate_import_line_or_throw normalizes license/admin/publisher to lowercase and auth to canonical form before comparison, so 'Viewer', 'Creator', 'SAML', 'tableauidwithmfa' etc. are all accepted - Add TableauIDWithMFA to valid auth values in validation (was missing) - 5 new tests covering each fix Add QuerysetEndpoint.find_by_name(name) (issue #1810): - Thin wrapper over .filter(name=name) returning a list - Available on all content-item endpoints (workbooks, datasources, views, users, projects, groups) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jacalata
force-pushed
the
jac/csv-import-fixes-and-find-by-name
branch
from
July 28, 2026 01:01
d3376c9 to
948e382
Compare
3 tasks
…rty setter Two related fixes so unmapped auth values fail loudly at CSV parse time rather than producing a UserItem with silently missing auth_setting: - create_user_from_line: raise ValueError instead of silently setting auth to None when the AUTH column value isn't in _auth_canonical(). - _set_values: route auth_setting through the @property_is_enum(Auth) setter rather than writing to _auth_setting directly, so any invalid auth string is rejected at assignment. These two together close bug #5 in #1809 (setter bypass) and the silent- None finding surfaced in an adversarial review of the earlier commits on this branch. Callers who want lenient behavior (skip invalid rows, keep going) can catch the exception in their own iteration loop — that's the model tabcmd uses today via its --complete/--no-complete flag. Once this lands, tabcmd can defer its per-line validation to TSC (see #1809 and #1836). Also tightens test_too_many_columns_raises to expect ValueError only (was accepting either ValueError or AttributeError).
find_by_name was bundled with the CSVImport fixes in earlier commits because it landed in the same working commit. It's orthogonal to the CSV work and closes a different issue (#1810), so it belongs in its own PR. Reverting the 3-line addition here; will land as a separate branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Call out the behavior change explicitly. The old code lowercased the entire CSV line including usernames, display names, fullnames, and emails; the new code preserves case for those fields and only normalizes the fields used for validation comparisons. Callers relying on the previous lowercased output need to know. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small cleanups on UserItem.CSVImport surfaced by an adversarial code review of the earlier bug fixes: - MAX renamed to COLUMN_COUNT and moved out of the ColumnType IntEnum. ColumnType(8) used to return ColumnType.MAX, a fake column mixed in with real column indices. Now the count is a class-level constant. - _auth_canonical() no longer rebuilds its dict on every call. Promoted to _AUTH_CANONICAL class attribute. - _valid_attributes[AUTH] no longer hardcodes the accepted auth values. Derived from _AUTH_CANONICAL.values() instead so there's a single source of truth for what AUTH strings are accepted. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jacalata
enabled auto-merge (squash)
August 6, 2026 21:19
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates UserItem.CSVImport to preserve case for user-provided fields while making validation/parsing of role/admin/publisher/auth fields case-insensitive and stricter, with added regression coverage and a changelog note.
Changes:
- Preserve original casing in
create_user_from_line(notably username/display name/email) and normalize only comparison-relevant fields. - Add canonicalization + validation for the AUTH column (including
TableauIDWithMFA) and fix column-count off-by-one handling. - Add tests covering mixed-case inputs, auth parsing, and error conditions; document the behavior change in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/test_user_model.py | Adds regression tests for mixed-case license/auth values, username case preservation, column count, and invalid auth handling. |
| tableauserverclient/models/user_item.py | Preserves casing for non-enum CSV fields, canonicalizes/validates AUTH, fixes column count logic, and routes auth_setting assignment through the enum-guarded setter. |
| CHANGELOG.md | Documents the behavior change that CSV parsing no longer lowercases the entire line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+311
to
+314
| # Go through the @property_is_enum(Auth) setter rather than writing | ||
| # to _auth_setting directly, so CSV-parsed users can't carry an | ||
| # invalid auth_setting that only fails later at the API call. | ||
| self.auth_setting = auth_setting |
Comment on lines
+536
to
537
| if len(line) > UserItem.CSVImport.COLUMN_COUNT: | ||
| raise AttributeError("Too many attributes in line") |
Copilot review finding on #1811. The prior version of _set_values routed auth_setting through the @property_is_enum(Auth) setter to catch bad values in CSV import. But _set_values is also called by from_xml, _parse_xml, and populate — the server-response paths. If a future Tableau release adds a new Auth enum value that this TSC version doesn't yet know about, response parsing would raise ValueError instead of transparently carrying the new value forward. CSV callers already validate against CSVImport._AUTH_CANONICAL before reaching _set_values (create_user_from_line raises with a clean error message for unknown auth strings), so the enum guard on _set_values was redundant for the CSV path and harmful for the server-parse path. Write directly to _auth_setting instead. Add a regression test that parses a UserItem XML carrying a hypothetical future auth type and asserts it survives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1809.
Motivation
UserItem.CSVImporthad six independent bugs that tabcmd currentlyworks around with per-line validation. Fixing them here so tabcmd can
eventually defer its validation to TSC (see #1836 for the follow-on
file-import method).
Also adds
QuerysetEndpoint.find_by_name-- a shortcut previouslyabsent from the queryset API that tabcmd's user/group lookups will use
once tabcmd bumps its TSC pin.
Behavior change
For users:
ColumnType.AUTH = 7andColumnType.MAX = 7were equal, so any 8-column line was rejected as"too many columns".
MAXis now8(column count, not last index).create_user_from_linepreserves username case.line.strip().lower()ran before splitting, destroying case forLDAP/mixed-case usernames. Only the comparison-relevant fields
(license, admin, publisher, auth) are normalized now.
TableauIDWithMFAadded to auth-column validation allowlist._validate_attribute_valuecompared raw input against lowercase allowlists, so
Viewer,Creator,SAMLwere all rejected. Normalizes each field beforecomparison.
_set_valuesno longer bypasses the auth-setting enum guard.Server-parsed UserItems will now raise on invalid auth values at the
point of parsing (bypasses on
_set_valueswrite directly to theprivate attr for forward-compat; see e7b6c3f).
auth_setting=None. Callers who want lenient behavior can catchthe exception.
Behavior change worth calling out for downstream: if any script was
inadvertently relying on rejected-because-too-many-columns behavior, or
on unknown-auth-values being silently null, those code paths change.
Test plan
test/test_user.py,test_user_model.py: 46 passed includingnew coverage for each of the six bugs
🤖 Generated with Claude Code