fix: correct the defects a pre-release review found in this cycle's work - #1161
Merged
Conversation
A systematic review of everything since v0.14.5 found four shipping defects and several smaller ones, all introduced by this cycle. None had reached a release. The session-config route takes THREE states per field: an absent key leaves the value unchanged, an explicit null clears it to the default, a value sets it. The Go and Java SDKs could emit only two. Go's `*int` with `omitempty` omits a nil pointer rather than writing null; Gson drops nulls by default. So restoring `maxReconnectAttempts` to unlimited — which the DTO says no in-range number can express, and which is the reason the route exists — was unreachable through either. Both now carry explicit `clear*` flags, Go through a MarshalJSON and Java through a serializer registered for that one type on a null-emitting Gson. The shared Gson keeps omitting nulls: applying serializeNulls() globally would turn every unset field of every other body into an explicit null, which this same route reads as "reset to default" — a worse bug than the one it fixes. Three response types dropped fields the API sends: the per-participant group result omitted `message`, the product-send response omitted `timestamp`, and Python's new 503 class was never exported from the package root, so the documented import failed for exactly the class that had been added. The OpenAPI validity pass was added to the export script only. The document is produced in two places, and a running gateway kept serving one that fails schema validation while the committed artifact was clean. Both producers now run the same passes in the same order, held there by a test. Also: the four path parameters this cycle added were the only ones in the document without a schema type, and a structural test now covers that; two Go doc comments and one Java javadoc had been orphaned by insertions; a Python comment had drifted from the class it describes; the JavaScript SessionConfig doc claimed two nullable fields where the DTO has one; every SDK error taxonomy and the design doc's three tables omitted the new 503 class; and the two exclusion lists that the README asserts "have to agree" disagreed in both directions.
Four PRs landed into the same unreleased section, and each conflict was resolved by appending its own `### Fixed` rather than merging into the existing one. The block ended with `Fixed`, `Added`, `Fixed`, `Fixed` — three headings for one type, with `Added` buried in the middle, so the release notes read as four unrelated lists and a reader scanning for fixes would stop at the first block. Keep a Changelog allows one section per change type per release, in a fixed order. The entries are unchanged: they were regrouped mechanically, and every one is present byte-for-byte — the only content difference is that the single `Added` entry now sits above the fixes instead of between them.
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.
A systematic regression review of everything since
v0.14.5— 90 files across three repositories — found four shipping defects and several smaller ones, all introduced by this cycle. None had reached a release. This fixes the ones in core.The session-config route needs three states; two SDKs could send two
The route takes an absent key (leave unchanged), an explicit
null(clear to default) and a value (set it). Its DTO says why the middle one is not decoration:Go declared
*intwithomitempty— a nil pointer is omitted, never written as null. Proven by running it:json.Marshalof the zero value produced{}.Java used
new Gson(), which drops nulls by default.So through either SDK, a capped session could never be returned to unlimited — the one operation the route exists for.
Both now carry explicit
clear*flags: Go through aMarshalJSON, Java through a serializer registered for that one type.Why not just
serializeNulls()in Java: it would turn every unset field of every other request body into an explicit null — which this same route reads as "reset to default". That is a worse bug than the one it fixes, so the shared Gson still omits nulls and only this body type uses the null-emitting one.Worth recording: the first attempt at the Java fix reproduced the original defect one layer down. The custom serializer built the right tree and Gson's writer silently dropped the
JsonNullanyway. The test caught it; reading the code had not.Three response types dropped fields the API sends
message— the same defect class the commit that introduced it set out to fixtimestamp, which the contract marks requiredA running gateway served an invalid document
The schema-validity pass went into
scripts/export-openapi.tsonly. The document is produced in two places — that, andsrc/main.tsfor the live/api/docs. So the committedopenapi.jsonwas clean while a running gateway kept serving a document that fails validation, which is precisely the scenario the original PR described.Both producers now run the same passes in the same order, held there by a structural test. Mutation-checked: removing the pass from
main.tsfails it.Smaller, same cycle
SessionConfigdoc claimed two nullable fields where the DTO has oneVerification
All five SDKs build and test clean.
lint,format:check,openapi:check,check:sdk-routes,check:versions,build,tsc --noEmit, and the full unit suite — 5118 tests — green.chat-media-archive.service.spec.ts, reproduced on the4cb7aa10tree from before any of this cycle's commits (2 failures in 3 runs there). Two consecutive clean runs followed here. It is tracked separately and is not caused by these changes.