Preserve Workbench business rule payloads for clearer validation errors#8048
Open
acwhite211 wants to merge 9 commits into
Open
Preserve Workbench business rule payloads for clearer validation errors#8048acwhite211 wants to merge 9 commits into
acwhite211 wants to merge 9 commits into
Conversation
📝 WalkthroughWalkthroughThis PR preserves business rule exception payloads through Workbench upload flows and renders them as localized, user-friendly validation messages. Previously, exceptions were stringified and lost structured details; now they flow from backend exception handlers through conversion helpers to frontend rendering with schema-aware labels. ChangesBusiness Rule Exception Payload Preservation
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
emenslin
requested changes
May 19, 2026
Collaborator
emenslin
left a comment
emenslin
left a comment
There was a problem hiding this comment.
- Confirm the message reads like a normal validation error, for example
Collectionobject must have unique catalognumber in collection. - Confirm the message includes conflicting record IDs when provided, for example
Conflicting record IDs: 3347460. - Confirm the tooltip no longer shows the raw Python tuple/dict payload.
Although this looks a lot better I do have one change I think is needed. I think that the error should show either the schema caption set by the user (e.g. CIDA Number instead of catalognumber) or the schema config field name (e.g. catalogNumber instead of catalognumber)
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
specifyweb/backend/workbench/upload/tests/test_upload_results_json.py (1)
41-70: ⚡ Quick winExtend this test to cover JSON round-trip of preserved payload.
This asserts conversion correctness, but not serialization/schema compatibility for the preserved payload. Add a small
UploadResult(...).to_json() -> json.dumps/json.loads -> from_jsonassertion in the same test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/workbench/upload/tests/test_upload_results_json.py` around lines 41 - 70, Extend the testBusinessRuleExceptionPayload to also verify JSON round-trip: create the FailedBusinessRule via to_failed_business_rule(BusinessRuleException(...), info), wrap it in an UploadResult (use the same FailedBusinessRule instance), call UploadResult.to_json() then json.loads and pass that into UploadResult.from_json() (or the project equivalent) and assert the deserialized UploadResult still contains the original FailedBusinessRule payload (compare the preserved payload dict and message). Ensure you use the existing symbols: BusinessRuleException, ReportInfo, to_failed_business_rule, FailedBusinessRule, UploadResult.to_json and UploadResult.from_json for locating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/workbench/upload/upload_result.py`:
- Around line 253-261: The helper is_business_rule_exception_with_payload
currently accepts any dict as the payload (exception.args[1]) which can contain
non-serializable or invalid values; update the logic in
is_business_rule_exception_with_payload (and the similar check around lines
264-268) to validate and/or sanitize the payload shape before treating it as a
business-rule payload: ensure exception.args[1] is a dict whose keys are strings
and whose values are only JSON-serializable scalar types (str, int, float, bool,
None) or plain lists/dicts that recursively satisfy the same constraint, or else
reject it (or replace with a safe fallback like an empty dict or
{"unserializable": true}) so that constructing/encoding FailedBusinessRule will
not fail at runtime. Include references to
is_business_rule_exception_with_payload and the code path that constructs
FailedBusinessRule when applying this validation.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts`:
- Around line 305-336: In resolveBackendBusinessRuleMessage, guard against
missing/empty table by checking the result of getStringPayload(payload, 'table')
(assigned to tableName) and return undefined immediately if it's falsy; this
prevents calling getSchemaTableLabel('') and producing empty localized table
labels. Keep the existing branches that use tableName (fieldNotUnique,
childFieldNotUnique) but only execute them when tableName is non-empty so
callers can fall back to the default message.
- Around line 259-269: The message suffix is hardcoded in English inside
withConflictingRecordIds, causing mixed locales; update withConflictingRecordIds
to fetch a localized prefix from backEndText (e.g., add a conflictingRecordIds
key to the backEndText dictionary) and use that localized string (via
localized/backEndText lookup) instead of the hardcoded "Conflicting record IDs:"
before joining payload.conflicting; ensure IR payload handling and existing
localized(message) wrapping remain unchanged so the full tooltip is entirely
localized.
---
Nitpick comments:
In `@specifyweb/backend/workbench/upload/tests/test_upload_results_json.py`:
- Around line 41-70: Extend the testBusinessRuleExceptionPayload to also verify
JSON round-trip: create the FailedBusinessRule via
to_failed_business_rule(BusinessRuleException(...), info), wrap it in an
UploadResult (use the same FailedBusinessRule instance), call
UploadResult.to_json() then json.loads and pass that into
UploadResult.from_json() (or the project equivalent) and assert the deserialized
UploadResult still contains the original FailedBusinessRule payload (compare the
preserved payload dict and message). Ensure you use the existing symbols:
BusinessRuleException, ReportInfo, to_failed_business_rule, FailedBusinessRule,
UploadResult.to_json and UploadResult.from_json for locating the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f43e892-a787-4b63-a34a-af5fa0eda045
📒 Files selected for processing (5)
specifyweb/backend/workbench/upload/tests/test_upload_results_json.pyspecifyweb/backend/workbench/upload/treerecord.pyspecifyweb/backend/workbench/upload/upload_result.pyspecifyweb/backend/workbench/upload/upload_table.pyspecifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts
Comment on lines
+253
to
+261
| def is_business_rule_exception_with_payload(exception: Exception) -> bool: | ||
| exception_class = exception.__class__ | ||
| return ( | ||
| exception_class.__module__ == BUSINESS_RULE_EXCEPTION_MODULE | ||
| and exception_class.__name__ == BUSINESS_RULE_EXCEPTION_NAME | ||
| and len(exception.args) >= 2 | ||
| and isinstance(exception.args[0], str) | ||
| and isinstance(exception.args[1], dict) | ||
| ) |
There was a problem hiding this comment.
Validate/sanitize preserved payload values before returning FailedBusinessRule.
Right now any dict in exception.args[1] is accepted. If a business-rule payload includes non-serializable values, upload-result JSON encoding can fail at runtime. Please gate payload contents to the allowed shape (or safely fallback).
Proposed fix
+def _is_payload_value(value: Any) -> bool:
+ if isinstance(value, (str, int, bool)) or value is None:
+ return True
+ if isinstance(value, list):
+ return all(isinstance(v, (str, int)) for v in value)
+ if isinstance(value, dict):
+ return all(
+ isinstance(k, str) and (isinstance(v, (str, int, bool)) or v is None)
+ for k, v in value.items()
+ )
+ return False
+
+def _is_business_rule_payload(payload: dict[Any, Any]) -> bool:
+ return all(isinstance(k, str) and _is_payload_value(v) for k, v in payload.items())
+
def is_business_rule_exception_with_payload(exception: Exception) -> bool:
exception_class = exception.__class__
return (
exception_class.__module__ == BUSINESS_RULE_EXCEPTION_MODULE
and exception_class.__name__ == BUSINESS_RULE_EXCEPTION_NAME
and len(exception.args) >= 2
and isinstance(exception.args[0], str)
and isinstance(exception.args[1], dict)
+ and _is_business_rule_payload(exception.args[1])
)Also applies to: 264-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specifyweb/backend/workbench/upload/upload_result.py` around lines 253 - 261,
The helper is_business_rule_exception_with_payload currently accepts any dict as
the payload (exception.args[1]) which can contain non-serializable or invalid
values; update the logic in is_business_rule_exception_with_payload (and the
similar check around lines 264-268) to validate and/or sanitize the payload
shape before treating it as a business-rule payload: ensure exception.args[1] is
a dict whose keys are strings and whose values are only JSON-serializable scalar
types (str, int, float, bool, None) or plain lists/dicts that recursively
satisfy the same constraint, or else reject it (or replace with a safe fallback
like an empty dict or {"unserializable": true}) so that constructing/encoding
FailedBusinessRule will not fail at runtime. Include references to
is_business_rule_exception_with_payload and the code path that constructs
FailedBusinessRule when applying this validation.
Comment on lines
+259
to
+269
| function withConflictingRecordIds( | ||
| message: LocalizedString, | ||
| payload: IR<unknown> | ||
| ): LocalizedString { | ||
| const conflicting = payload.conflicting; | ||
| return Array.isArray(conflicting) && conflicting.length > 0 | ||
| ? localized( | ||
| `${message} (Conflicting record IDs: ${conflicting.join(', ')})` | ||
| ) | ||
| : message; | ||
| } |
There was a problem hiding this comment.
Hardcoded English string defeats localization.
The "Conflicting record IDs:" prefix is hardcoded in English while the surrounding message is localized via backEndText. Non-English users will see a mixed-locale tooltip, which is exactly the kind of UX issue this PR is trying to clean up. Please route this string through backEndText (e.g., a new conflictingRecordIds entry) so it gets translated alongside the rule message.
🌐 Suggested shape
- ? localized(
- `${message} (Conflicting record IDs: ${conflicting.join(', ')})`
- )
+ ? localized(
+ `${message} (${backEndText.conflictingRecordIds({
+ ids: conflicting.join(', '),
+ })})`
+ )
: message;(Add a corresponding conflictingRecordIds key to the backEndText localization dictionary.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts` around
lines 259 - 269, The message suffix is hardcoded in English inside
withConflictingRecordIds, causing mixed locales; update withConflictingRecordIds
to fetch a localized prefix from backEndText (e.g., add a conflictingRecordIds
key to the backEndText dictionary) and use that localized string (via
localized/backEndText lookup) instead of the hardcoded "Conflicting record IDs:"
before joining payload.conflicting; ensure IR payload handling and existing
localized(message) wrapping remain unchanged so the full tooltip is entirely
localized.
Comment on lines
+305
to
+336
| function resolveBackendBusinessRuleMessage( | ||
| payload: IR<unknown> | ||
| ): LocalizedString | undefined { | ||
| const tableName = getStringPayload(payload, 'table'); | ||
| if (payload.localizationKey === 'fieldNotUnique') | ||
| return withConflictingRecordIds( | ||
| backEndText.fieldNotUnique({ | ||
| tableName: getSchemaTableLabel(tableName), | ||
| fieldName: getSchemaFieldLabels( | ||
| tableName, | ||
| getStringPayload(payload, 'fieldName') | ||
| ), | ||
| }), | ||
| payload | ||
| ); | ||
| else if (payload.localizationKey === 'childFieldNotUnique') | ||
| return withConflictingRecordIds( | ||
| backEndText.childFieldNotUnique({ | ||
| tableName: getSchemaTableLabel(tableName), | ||
| fieldName: getSchemaFieldLabels( | ||
| tableName, | ||
| getStringPayload(payload, 'fieldName') | ||
| ), | ||
| parentField: getSchemaFieldLabels( | ||
| tableName, | ||
| getStringPayload(payload, 'parentField') | ||
| ), | ||
| }), | ||
| payload | ||
| ); | ||
| else return undefined; | ||
| } |
There was a problem hiding this comment.
Guard against missing table in payload.
If payload.table is absent or non-string, getStringPayload returns '', and getSchemaTableLabel('') falls back to localized('') — producing a message like "must have unique catalognumber in collection" with an empty table label. Consider returning undefined from resolveBackendBusinessRuleMessage when tableName is empty so the caller falls through to a saner default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specifyweb/frontend/js_src/lib/components/WorkBench/resultsParser.ts` around
lines 305 - 336, In resolveBackendBusinessRuleMessage, guard against
missing/empty table by checking the result of getStringPayload(payload, 'table')
(assigned to tableName) and return undefined immediately if it's falsy; this
prevents calling getSchemaTableLabel('') and producing empty localized table
labels. Keep the existing branches that use tableName (fieldNotUnique,
childFieldNotUnique) but only execute them when tableName is non-empty so
callers can fall back to the default message.
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
3 participants
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.
Fixes #8045
Improve Workbench handling of back-end business rule validation errors. The back-end now preserves structured
BusinessRuleExceptionpayloads inFailedBusinessRuleupload results instead of stringifying and discarding them. The Workbench front-end uses those payloads to show uniqueness-rule failures and appends the conflicting record IDs when available. This makes duplicate catalog number errors clearer and avoids exposing raw Python exception tuples in validation tooltips.Checklist
self-explanatory (or properly documented)
Testing instructions
Collectionobject must have unique catalognumber in collection.Conflicting record IDs: 3347460.Summary by CodeRabbit
Bug Fixes
Improvements