[tabcmd] fix: preserve case of customSubscriptionEmail and customSubscriptionFooter (#1849) - #1851
Open
jacalata wants to merge 2 commits into
Open
[tabcmd] fix: preserve case of customSubscriptionEmail and customSubscriptionFooter (#1849)#1851jacalata wants to merge 2 commits into
jacalata wants to merge 2 commits into
Conversation
…ooter (#1849) RequestFactory.Site.update_req and create_req called `str(value).lower()` on customSubscriptionEmail and customSubscriptionFooter when serializing to XML, silently mangling caller intent: site.custom_subscription_email = "Sales@Company.com" site.custom_subscription_footer = "Sent by Tableau -- Confidential." server.sites.update(site) # server received: customSubscriptionEmail="sales@company.com" # customSubscriptionFooter="sent by tableau -- confidential." Impact: - Email: cosmetic only (SMTP treats mailboxes case-insensitively in practice), but recipients see the wrong-case "from" address. - Footer: functionally broken. The footer is displayed verbatim in outgoing subscription emails, so lowercasing removes company-name casing, sentence capitalization, brand terms, etc. Anyone setting a customer-facing footer via TSC got a mangled result with no workaround short of the web UI. Drop the .lower() on both string values in both code paths. The paired *Enabled boolean attributes still get .lower()d (they need to serialize as "true"/"false"), just the string values are now sent verbatim. Discovered while implementing tabcmd editsite parity (tableau/tabcmd#437, tabcmd PR #452). Fixes #1849.
3 tasks
jacalata
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes a serialization bug in the Tableau Server Client request factory where two site attributes (customSubscriptionEmail, customSubscriptionFooter) were being lowercased when generating XML, which broke case-sensitive footer display and altered caller intent.
Changes:
- Stop lowercasing
customSubscriptionEmailandcustomSubscriptionFooterwhen serializing site create/update requests. - Add regression tests to ensure both fields preserve exact casing in the generated request XML.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tableauserverclient/server/request_factory.py |
Removes unintended .lower() calls so these site string fields serialize verbatim. |
test/test_site.py |
Adds regression tests validating case preservation for create/update request bodies. |
Suppressed comments (1)
test/test_site.py:235
- Similarly here, asserting on substrings in the serialized XML text is brittle. Parsing the XML and checking the
<site>attributes directly will make the test more robust and matches the existing style in this file.
xml_bytes = RequestFactory.Site.create_req(site)
xml_text = xml_bytes.decode("utf-8")
assert 'customSubscriptionEmail="Support@Company.com"' in xml_text, xml_text
assert 'customSubscriptionFooter="COMPANY, Inc. -- All Rights Reserved."' in xml_text, xml_text
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+216
to
+222
| xml_bytes = RequestFactory.Site.update_req(site) | ||
| xml_text = xml_bytes.decode("utf-8") | ||
|
|
||
| assert 'customSubscriptionEmail="Sales@Company.com"' in xml_text, xml_text | ||
| assert ( | ||
| 'customSubscriptionFooter="Sent by Tableau -- Confidential. See https://Example.com/Legal"' in xml_text | ||
| ), xml_text |
Contributor
Author
There was a problem hiding this comment.
Fixed in abb41b2. Refactored both tests to parse with defusedxml and assert on the parsed site element attributes, matching the pattern in test_encoding_attr_capture (line 361). Still catches the load-bearing regression (.lower() applied to caller input) but survives serialization-format changes like quoting or entity escaping.
Copilot noted that substring-matching the serialized XML is brittle: attribute quoting (single vs double), whitespace, and entity escaping can vary without changing meaning. Parse the request with defusedxml (matching the existing test_encoding_attr_capture style at line 361) and assert on the parsed attribute value instead. The load-bearing check is still "the value is not lowercased", which is what site_elem.attrib["customSubscriptionEmail"] == "Sales@Company.com" verifies. Test now survives serialization-format changes and still fails if the .lower() regression comes back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 #1849.
Motivation
RequestFactory.Site.update_reqandcreate_reqcalledstr(value).lower()on
customSubscriptionEmailandcustomSubscriptionFooterwhen serializing.Email was cosmetic (SMTP is case-insensitive in practice) but footer was
functionally broken -- the footer text is displayed verbatim in outgoing
subscription emails, so lowercasing removed company-name casing, sentence
capitalization, brand terms. Any TSC caller setting a customer-facing footer
got a mangled result with no workaround short of the web UI.
Discovered while implementing tabcmd editsite parity
(tableau/tabcmd#437, tableau/tabcmd#452).
Behavior change
For users:
customSubscriptionEmailandcustomSubscriptionFooternowserialize verbatim rather than lowercased. If any downstream test or script
asserts against the lowercased-value bug, it will need updating. The paired
*Enabledboolean attributes still get.lower()'d (they need to serializeas
"true"/"false").Verification against server: Read the server-side write chain to confirm
the server does not itself lowercase these fields.
RestApiSiteParamsBuilder.setCustomEmail/setCustomEmailFooterpass thevalues through verbatim into
SiteParams.withCustom*and thence toSite.setCustomReplyToEmail(JPA columncustom_subscription_email). NotoLowerCase()anywhere in the write chain.SiteParamsValidator.java:170validates the email is well-formed; does not modify case. Footer has no
validation. Cross-checked against tabcmd Classic, which sends both strings
verbatim as multipart form parts.
Test plan
test/test_site.py-- one forupdate_req,one for
create_req, asserting exact case survives to XML request bodytest_site.pysuite: 28 passed🤖 Generated with Claude Code