Repository navigation
fix(API): make translation content and key description nullable #STRINGS-3652 - #1325
Mikko Karkee (Varpuspaavi) wants to merge 5 commits into
Conversation
…le #STRINGS-3652
Plain string fields generate Go structs with omitempty, so the CLI dropped
{"content":""} before sending and clearing a translation silently no-op'd.
Marking them nullable generates *NullableString, which preserves an empty
string on the wire.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
API changelog (oasdiff)Doc-only edits (descriptions, examples) do not appear here. |
…n #STRINGS-3652
The CLI and Go client still drop {"content":null}; only document the
empty string as the way to clear content.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Same omitempty issue as translation update: an empty description was dropped client-side, so clearing a key description silently no-op'd. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Efe Bikmaz (bikmazefe)
left a comment
There was a problem hiding this comment.
Two review notes on the nullable change.
| description: Translation content | ||
| description: Translation content. Send an empty string to clear the translation's content; omit the field to leave it unchanged. | ||
| type: string | ||
| nullable: true |
There was a problem hiding this comment.
nullable: true advertises null as a valid value, but the description tells callers to send an empty string, and the PR notes {"content":null} is still dropped by the CLI. The spec, the prose and the CLI behaviour disagree: generated clients will accept null, while the CLI silently ignores it. Consider either documenting the null semantics (and the CLI limitation) or rewording the description so it doesn't imply null is unsupported.
There was a problem hiding this comment.
Thanks, fair point. Checking the API code showed null clears content and description, so the descriptions now say an empty string or null clears the value, and that the CLI drops null (use "" there). I also removed nullable from branch and plural_suffix: branch is only a selector (null is ignored) and plural_suffix is NOT NULL in the DB. Pushed in 4a610f9. Note the null behavior comes from reading the API code, not from running requests.
| branch: | ||
| description: specify the branch to use | ||
| type: string | ||
| nullable: true |
There was a problem hiding this comment.
This changes the generated Go model type for branch, content and plural_suffix (and description in keys update) from string to *NullableString. That is source-breaking for typed phrase-go consumers, e.g. body.Content = "x" stops compiling, and other typed clients (Java, Swift, TypeScript) change field types too. oasdiff only reports it as info. Could the PR description call this out and say what release/version bump it needs?
There was a problem hiding this comment.
Confirmed: I generated the Go client from main and from this branch. body.Content = "x" no longer compiles (string -> *NullableString); the scope is now only content and description after dropping branch and plural_suffix. The PR body has a "Client impact" section with the migration snippet and the release-note question. Version bump (fix(API) vs fix(API)!:) is still open for the release owners; other typed clients were not inspected.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…STRINGS-3652 branch is ignored when null and plural_suffix is NOT NULL in the DB, so neither should be advertised as nullable. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Update after review, Efe Bikmaz (@bikmazefe):
Open: the version bump. Title is Re-review would be appreciated. |
Problem
STRINGS-3652:
phrase translations update --data '{"content":""}'returns success but leaves the translation unchanged. The same payload sent directly to the API clears it.contentontranslation/update(anddescriptiononkey/update) were plaintype: string, so the generated Go model usesstringwithjson:",omitempty". The CLI unmarshals--datainto that struct and re-marshals it, andomitemptydrops"", so the key never reaches the API.Change
paths/translations/update.yaml: markcontentasnullable: true; document that an empty string ornullclears it and omitting it leaves it unchanged. The CLI dropsnull, so the description tells CLI users to send an empty string.paths/keys/update.yaml: markdescriptionasnullable: true(sameomitemptybug), same wording.doc/compiled.json: regenerated (make lint).Generated Go model now uses
*NullableStringfor these two fields (same pattern asrepo_syncname).clients/is gitignored and regenerated on release.Why not
branchandplural_suffixAn earlier revision also marked these nullable. Checking the API code showed that was wrong:
branchis a branch selector read fromparams[:branch], not a translation attribute.null,""and omitted all fall back to the main project, soomitemptydropping""is harmless.plural_suffixisNOT NULL DEFAULT ""in the DB.nullon a pluralized key gives a 422; on a non-plural key it probably violates the NOT NULL constraint (not confirmed by running it).""is the safe "no suffix" value, and dropping it rarely matters.API behavior of
null(from reading the API code, not from running requests)content:nullclears it (stored as nil;""is stored as an empty string; both are treated as blank). Omitted leaves it unchanged.description:nullclears it (NULL;""stores""). Omitted leaves it unchanged.Verification
Generated the Go client with the repo's generator config and ran a marshal round-trip on
TranslationUpdateParameters:{"content":""}{"content":""}{"content":"x"}{"content":"x"}{"content":"null"}(the string){"content":"null"}{}{}{"content":null}{}make lintpasses. Other language generators were not run.Client impact (source-breaking for typed clients)
nullable: truechanges the generated field type fortranslation/updatecontentandkey/updatedescription:string->*NullableString. Existing code such asbody.Content = "x"no longer compiles; callers must switch to the nullable wrapper type:mainand from this branch and compiling against the branch model.null(previously impossible withstring).NewNullableString(nil), a zero-value&NullableString{}and aNullableStringafterUnset()all marshal as"content":null, becauseMarshalJSONignoresisSet. Sincenullclears these fields, a forgotten or zero-valued wrapper clears the field instead of omitting it.""is now sent (the point of this fix); before,""was dropped byomitempty.info(request-property-became-nullable) because the wire contract is backwards compatible: any previously valid request is still valid.Release note: the PR title is
fix(API), which release-please treats as a patch bump. Because of the source-breaking Go/typed-client change, the release owners should decide whether to mark it as breaking (fix(API)!:) for a major bump, or accept a patch bump with a changelog note. This is a call for the reviewers.Known limitation (out of scope)
{"content":null}through the CLI is still dropped:encoding/jsonsets a*NullableStringtonilon JSONnullwithout callingUnmarshalJSON. Fixing it needs a Go model template change affecting all nullable fields, so it is deliberately not done here. Direct API calls withnullclear the field.Follow-up
Once released,
phrase-clineeds aphrase-godependency bump to pick this up.🤖 Generated with Claude Code