fix(parsers): stop coercing free-text fields (single-letter cue names); rc11 - #8
Merged
Merged
Conversation
str_to_value() ran every scalar through int -> float -> strtobool -> Uuid regardless of its key, so free text was silently rewritten on save. Since strtobool accepts the truth abbreviations, a cue named n/N/f/F was persisted as False, y/Y/t/T as True, and any bare digit as an int -- 18 of the 62 alphanumeric single characters, plus yes/no/true/false/on/off. Reported as "can't name a cue with a single letter" (ClickUp 869cqbpxa). There was never a length rule: NameStringType has always been minLength=1, and the frontend has no validator. The failing characters were exactly strtobool's vocabulary, which is why searching for a min-length check found nothing. Separately, a cue named lowercase none/null hit the ['none','null',''] -> None branch, serialised to <name/>, and failed NameStringType's minLength=1 -- a hard XMLSchemaValidationError at save rather than silent corruption. The new short-circuit precedes that branch, fixing both. Adds STRING_TYPED_KEYS and an optional key argument to str_to_value, threaded through all four call sites. Keeping key optional leaves existing single-argument callers unaffected. Only name, description and file_name are reachable today; output_name, parameter_name, icon and color are currently shielded by unrelated bypasses in outputsParser, _normalize_fade_parameters and the GenericDict fallback. They are listed anyway so fixing those bypasses cannot silently reintroduce this bug. 'id' is deliberately excluded -- the Uuid() branch is the only thing producing Uuid objects on parse. Closes ClickUp 869cqbpxa.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
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 ClickUp 869cqbpxa — "Error al querer poner el nombre de una cue de una sola letra".
The bug
There was never a length rule.
NameStringTypehas always beenminLength=1, and the frontend cue-name input has no validator at all.CuemsParser.str_to_value()ran every scalar throughint→float→strtobool→Uuidregardless of its key, so free text was silently rewritten on save. Becausestrtoboolaccepts the truth abbreviations, the failing characters were exactly its vocabulary:n,N,f,FFalsey,Y,t,TTrue0–9a,b,x,Q, …18 of 62 alphanumeric single characters were corrupted, plus
yes/no/true/false/on/off. Reproduced on test2 through the editor's real save path: a cue namednwas written to XML as<name>False</name>.Second failure class, same root cause
A cue named lowercase
none/nullhit the['none','null','']→Nonebranch, serialised to<name/>, and failedminLength=1— a hardXMLSchemaValidationErrorat save time, not silent corruption. The new short-circuit precedes that branch, so both are fixed together.The fix
STRING_TYPED_KEYS+ an optionalkeyargument onstr_to_value, threaded through all four call sites. Keepingkeyoptional leaves existing single-argument callers unaffected.idis deliberately excluded: theUuid()branch insidestr_to_valueis the only thing producingUuidobjects on parse (parsers assign via rawdict.__setitem__and never reach the property setters), so adding it would downgrade every cue/script/media id to a plainstr. Consequence:DmxSceneType.id(script.xsd:403, declaredxs:string) can't be covered by this mechanism — accepted, since DMX scene ids are system-assigned.Scope — narrower than first assumed
Only
name,descriptionandfile_nameare reachable today. An earlier analysis claimedoutput_namewas also affected; that is empirically false.output_name,parameter_name,iconandcolornever reachstr_to_value, for three distinct reasons:outputsParser.parse()constructs output objects directly viaself._class(item)fade_profileParserdivertsparametersto_normalize_fade_parameters()before the scalar branchget_class('ui_properties')misses (the class isUI_properties) and falls back toGenericDictraw passthroughThey're kept in the allowlist as defensive entries so that fixing any of those bypasses can't silently reintroduce this bug. The comment in the source says so explicitly.
Testing
tests/test_name_coercion.py: exhaustive sweep over all 62 alphanumeric single characters per reachable key, the boolean/nullish word set, full XML round-trip, and negative tests pinning thatenabled/autoload/timecode/loopstill coerce andidstill yields aUuid.test_realtime_25fps_jitter, a ±1ms wall-clock tolerance test) — confirmed it fails identically on a clean tree without this change.none/nullno longer error, andenabled 'true'→True/loop '1'→1still correct.Follow-ups (not in this PR)
cue_script.xml; the original text is unrecoverable (n,no,N,offall collapse toFalse) and needs manual renaming.cuems-nodeconfhas the identical bug atNodeXmlBuilders.py:80(calls the inheritedstr_to_valuewithout a key) — tracked as ClickUp 869eag7hv.get_parser_class/get_classcase-sensitivity defects — each needs its own plan.