Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import unittest
from jsonschema import validate, Draft7Validator # type: ignore

from specifyweb.backend.businessrules.exceptions import BusinessRuleException

from ..upload_result import *
from ..upload_results_schema import schema

Expand Down Expand Up @@ -36,6 +38,37 @@ def testFailedBusinessRule(self, failedBusinessRule: FailedBusinessRule):
j = json.dumps(failedBusinessRule.to_json())
self.assertEqual(failedBusinessRule, FailedBusinessRule.from_json(json.loads(j)))

def testBusinessRuleExceptionPayload(self):
info = ReportInfo(
tableName="Collectionobject",
columns=["catalogNumber"],
treeInfo=None,
)
payload = {
"localizationKey": "childFieldNotUnique",
"table": "Collectionobject",
"fieldName": "catalognumber",
"fieldData": {"catalognumber": "0037481"},
"parentField": "collection",
"parentData": {"collection": "Collection object (360449)"},
"conflicting": [3347460],
}

self.assertEqual(
to_failed_business_rule(
BusinessRuleException(
"Collectionobject must have unique catalognumber in collection",
payload,
),
info,
),
FailedBusinessRule(
"Collectionobject must have unique catalognumber in collection",
payload,
info,
),
)

@given(noMatch=infer)
def testNoMatch(self, noMatch: NoMatch):
j = json.dumps(noMatch.to_json())
Expand Down
3 changes: 2 additions & 1 deletion specifyweb/backend/workbench/upload/treerecord.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
FailedBusinessRule,
ReportInfo,
TreeInfo,
to_failed_business_rule,
)
from .uploadable import (
Row,
Expand Down Expand Up @@ -954,7 +955,7 @@ def _upload(
obj = self._do_insert(model, **new_attrs)
except (BusinessRuleException, IntegrityError) as e:
return UploadResult(
FailedBusinessRule(str(e), {}, info), parent_result, {}
to_failed_business_rule(e, info), parent_result, {}
)

result = UploadResult(Uploaded(obj.id, info, []), parent_result, {})
Expand Down
32 changes: 31 additions & 1 deletion specifyweb/backend/workbench/upload/upload_result.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@
from .parsing import WorkBenchParseFailure

Failure = Literal["Failure"]
BUSINESS_RULE_EXCEPTION_MODULE = "specifyweb.backend.businessrules.exceptions"
BUSINESS_RULE_EXCEPTION_NAME = "BusinessRuleException"
BusinessRulePayloadValue = (
str
| int
| bool
| None
| list[str]
| list[int]
| dict[str, str | int | bool | None]
)
BusinessRulePayload = dict[str, BusinessRulePayloadValue]


class TreeInfo(NamedTuple):
Expand Down Expand Up @@ -215,7 +227,7 @@ def from_json(json: dict) -> "Deleted":

class FailedBusinessRule(NamedTuple):
message: str
payload: dict[str, str | int | list[str] | list[int]]
payload: BusinessRulePayload
info: ReportInfo

def get_id(self) -> Failure:
Expand All @@ -238,6 +250,24 @@ def from_json(json: dict) -> "FailedBusinessRule":
)


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)
)
Comment on lines +253 to +261
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.



def to_failed_business_rule(exception: Exception, info: ReportInfo) -> FailedBusinessRule:
if is_business_rule_exception_with_payload(exception):
return FailedBusinessRule(exception.args[0], exception.args[1], info)

return FailedBusinessRule(str(exception), {}, info)


class NoMatch(NamedTuple):
info: ReportInfo

Expand Down
7 changes: 4 additions & 3 deletions specifyweb/backend/workbench/upload/upload_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
PicklistAddition,
ParseFailures,
PropagatedFailure,
to_failed_business_rule,
)
from .uploadable import (
NULL_RECORD,
Expand Down Expand Up @@ -760,7 +761,7 @@ def _do_upload(
picklist_additions = self._do_picklist_additions()
except (BusinessRuleException, IntegrityError) as e:
return UploadResult(
FailedBusinessRule(str(e), {}, info), to_one_results, {}
to_failed_business_rule(e, info), to_one_results, {}
)

record = Uploaded(uploaded.id, info, picklist_additions)
Expand Down Expand Up @@ -865,7 +866,7 @@ def delete_row(self, parent_obj=None) -> UploadResult:
reference_record.delete()
result = Deleted(self.current_id, info)
except (BusinessRuleException, IntegrityError) as e:
result = FailedBusinessRule(str(e), {}, info)
result = to_failed_business_rule(e, info)

to_one_deleted: dict[str, UploadResult] = {
key: value.delete_row()
Expand Down Expand Up @@ -1066,7 +1067,7 @@ def _do_upload(
picklist_additions = self._do_picklist_additions()
except (BusinessRuleException, IntegrityError) as e:
return UploadResult(
FailedBusinessRule(str(e), {}, info), to_one_results, {}
to_failed_business_rule(e, info), to_one_results, {}
)

record: Updated | NoChange = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
formatDisjunction,
} from '../Atoms/Internationalization';
import { getField } from '../DataModel/helpers';
import { tables } from '../DataModel/tables';
import { getTable, tables } from '../DataModel/tables';
import type { Tables } from '../DataModel/types';

/*
Expand Down Expand Up @@ -256,14 +256,96 @@ export function resolveBackendParsingMessage(
else return undefined;
}

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;
}
Comment on lines +259 to +269
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


function getStringPayload(payload: IR<unknown>, key: string): string {
const value = payload[key];
return typeof value === 'string' ? value : '';
}

function getSchemaTableLabel(tableName: string): LocalizedString {
return getTable(tableName)?.label ?? localized(tableName);
}

function getSchemaFieldLabel(
tableName: string,
fieldName: string
): LocalizedString {
const lookupFieldName = fieldName.split('__').join('.');
return (
getTable(tableName)?.getField(lookupFieldName)?.label ??
localized(fieldName)
);
}

function getSchemaFieldLabels(
tableName: string,
fieldNames: string
): LocalizedString {
const labels = fieldNames
.split(',')
.map((fieldName) => fieldName.trim())
.filter((fieldName) => fieldName.length > 0)
.map((fieldName) => getSchemaFieldLabel(tableName, fieldName));
return labels.length === 0
? localized(fieldNames)
: formatConjunction(labels);
}

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;
}
Comment on lines +305 to +336
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


/** Back-end sends a validation key. Front-end translates it */
export function resolveValidationMessage(
key: string,
payload: IR<unknown>
): LocalizedString {
const baseParsedMessage = resolveBackendParsingMessage(key, payload);
const businessRuleMessage = resolveBackendBusinessRuleMessage(payload);
if (baseParsedMessage !== undefined) {
return baseParsedMessage;
} else if (businessRuleMessage !== undefined) {
return businessRuleMessage;
} else if (key === 'failedParsingPickList')
return backEndText.failedParsingPickList({
value: `"${payload.value as string}"`,
Expand Down
Loading