fix: preserve GitHub label objects in create-issue response - #41
Open
jonsuguiyama wants to merge 1 commit into
Open
fix: preserve GitHub label objects in create-issue response#41jonsuguiyama wants to merge 1 commit into
jonsuguiyama wants to merge 1 commit into
Conversation
The response schema declared labels as an array of strings while the GitHub REST API returns them as objects. fast-json-stringify coerces each object to the string "[object Object]" rather than failing, so the endpoint answers 200 with corrupted label data. Restores the anyOf(string, object) shape the schema had before hasadna#23, and adds a serialization test that drives the route through fastify.inject. The existing unit tests call the controller with a plain mock reply, so they never run the serializer and cannot observe this.
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.
🤖 This PR was written with the help of an AI coding agent (Claude), reviewed and submitted by @jonsuguiyama.
Summary
POST /issues/create(and the deprecatedPOST /create-issue) answer HTTP 200 with the issue's labels serialized as["[object Object]"]instead of the label data GitHub actually returned.The issue is still created correctly on GitHub. Only the response body is affected, which is why this has gone unnoticed.
Root cause
githubIssueModelinsrc/schemas/issues.schema.jsdeclares:The GitHub REST API returns
labelsas an array of objects, not strings:fast-json-stringifydoes not reject this mismatch. For a declaredstringit coerces the value, so each label object becomes the literal string"[object Object]". The request itself succeeded, so the endpoint returns 200 and the coerced value reaches the client.Impact
To be upfront: nothing is visibly broken today.
open-bus-map-searchnever readslabelsfrom this response, so no user currently sees the effect. I would still argue this is worth fixing, for three reasons.1. The published contract is wrong, and it is generated. The live OpenAPI document at
/docs/jsondeclares:That document is what generates
@hasadna/open-bus-api-client, where the type reads:So a TypeScript consumer is told labels are strings, and at runtime they are strings, just useless ones. The declared type and the runtime value agree with each other while both are wrong, which means the compiler can never surface it. Whoever first tries to display a label has to trace back through the frontend, the generated client and the backend schema to find out why they got
"[object Object]".2. It is data loss rather than a formatting quirk. The name, color and id are discarded during serialization. Nothing downstream can recover them, because the information is already gone when the response leaves the server.
3. The failure mode generalizes.
fast-json-stringifycoerces instead of throwing for every declared property, so any future type drift in this model will degrade just as quietly, and the current test setup cannot observe it. That is the part I think matters most: the response contract of this endpoint is effectively unverified today. The test added here checks the serialized response as a whole, not onlylabels.Where it came from
Before #23 the schema matched GitHub's own spec:
#23 ("feat(issues): add debug, mask email, simplefiy schema") reduced that file from 108 lines to 17 and kept only the
stringbranch of theanyOf. GitHub returns the object branch, so the branch that was dropped is the one that actually occurs.The same PR added the debug fixture, which returns
labels: ['REPORTED-BY-USER']. That fixture is string shaped, so it agrees with the narrowed schema. The real GitHub response is the only path that disagrees with it.Why the existing tests do not catch this
Two reasons, both structural rather than accidental:
tests/issues.test.jscallscreateIssue(request, reply)directly, passing the mock reply fromtest.utils.js. That mock recordssend()arguments on a plain object and never runs Fastify's serializer, so no assertion in the file can observe what a client actually receives.The two assertions that mention
labelscheck data that is string shaped by construction. Line 91 checks the payload sent to GitHub (correctly strings). Line 210 checks the debug fixture.createMockGitHubResponsedoes not includelabelsat all, so no test ever feeds a realistic GitHub response through the endpoint.Fix
src/schemas/issues.schema.js: restore theanyOf(string, object)shape the schema had before feat(issues): add debug, mask email, simplefiy schema #23. Both forms stay valid, so anything relying on string labels keeps working.tests/issues.serialization.test.js(new): drives the route throughfastify.inject, so the response passes through the real serializer instead of a mock. Covers both the object form and the string form.Test plan
npm test(eslint, prettier, mocha, c8), 54 passing and clean.mainasAssertionError: expected [ '[object Object]' ] to deeply equal [ { color: 'ededed', …(6) } ], and passes with the schema fix./docs/json.src/routes/issues.routes.js, which did not appear in the coverage report at all before, since nothing exercised the route. Total statement coverage goes from 87.41% to 90.33%.Notes
@NoamGaash