Developer guide: show what the protocol generators actually emit - #5811
Merged
Conversation
The three generate-* appendices each stop at a colon where a listing should be: "The @GraphQLClient interface looks like:", "Call sites use the static factory:", "A subscription returns a GraphQLSubscription handle whose cancel() ends the stream:". Six of them. Rather than hand-writing something that looks like generated code, this runs the generators. GenerateGraphQLMojo.Generator, GenerateGrpcMojo. Generator and GenerateOpenApiMojo.Generator were driven against the exact schema, proto and spec the chapters describe -- the same fixtures their unit tests use -- and the output committed under docs/demos/common, with only tag markers added. So the listings are the generators' answer, not a plausible reconstruction of it, and a change in emission shape shows up here as a diff. That also settles a claim that was wrong: the openapi chapter says models are emitted as "@mapped record (Java 17+) or class (Java 8)", and the Pet.java it pointed at was a PropertyBusinessObject. The real one is a record. PetApi.java was close but not what the generator writes either. Both are now the generator's output. The call sites are hand-written, because a generator does not emit those. Each carries the one thing its response type gets wrong by default: gRPC- Web reports failure under HTTP 200 so the status is the thing to read, and a GraphQL response can carry data and errors at once so isOk() is about the errors array rather than about getData(). Ratchet drops from 34 to 28. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 187fb6018d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pty answer Three of the four listings dereferenced whatever came back. GraphQL is the sharp one, twice. isOk() reports that the errors array came back empty, which is not the same as the query having found anything: the schema declares hero as Character, not Character!, so a clean response can carry a null selection. And on a subscription, onError is the end of the stream -- a per-field failure arrives at onNext instead, as a next payload whose errors array is non-empty and whose data may be partial or absent, which is what GraphQLSubscription.Handler#onNext documents. REST and gRPC get the smaller version of the same: a 2xx with an empty or unmappable body, and an OK status with no message frame, both leave the response data null. Each guard carries the reason, since the trap is in the response type's contract rather than in this code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check-copyright-headers covers every added source, and the generators do not emit one. The header goes above the "Generated by" line, alongside the tag markers, as the only edits made to the generators' output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5652e0674e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A GraphQL call that never reached the server comes back from GraphQL.postResponse as new GraphQLResponse(0, null, emptyList(), message) -- httpCode 0, no errors, no data -- and decodeJson does the same for an empty or unparseable body. isOk() only asks whether the errors array is empty, so it answers true for every one of those, and the listing reported "No hero for that episode" for an auth failure, a 500 and a dropped connection alike. The query call site now checks the HTTP code first and only then classifies the payload. The reason is beside the check, since it is the response type's contract that makes it necessary. The subscription call site needs no equivalent: deliverNext builds its response with code 200 and a message only from the errors array, and every transport failure goes to onError instead, so hasErrors() is the right discriminator there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc1bace9ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
RestClientAnnotationProcessor's generated impl calls back with null when the request never completed, and on a 4xx or 5xx its error handler forwards the raw Response<String> cast to Response<Pet>. So the one parameter arrives as null, as an error body typed as a Pet, or as a Pet, and the listing read getResponseData() first. The error-status case is the nasty one and it is worth the reader knowing about: reading that payload as a Pet is a cast that does not throw on ParparVM, so on a device it hands String bytes to Pet's field reads rather than raising ClassCastException. Guarded in the order the response has to be classified, with the reason beside it. gRPC and GraphQL need no equivalent: GrpcWeb always answers with a GrpcResponse carrying STATUS_TRANSPORT_FAILURE, and GraphQL.postResponse always answers with a GraphQLResponse. Neither ever passes null, and neither reuses another payload's type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89b0c5a458
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Cloudflare Preview
|
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Generating from the Swagger Petstore, the spec this appendix names, produces
a Pet.java that does not compile:
cannot find symbol
symbol: class Tag
location: package com.example.petstore.model
Tag and Category declare the same two properties, unifyShapes collapses
identical shapes to one record, and Tag is the one that goes. But property
types are resolved in pass 2, before unification has run, so Pet.tags kept
the name of the class that was about to be dropped and nothing ever emits
it. Operations are immune because they are built after unifyShapes -- the
comment there even says so -- which is why no test caught it.
unifyShapes now records what it renamed and re-points the property types
afterwards, matching on a token boundary so model.Tag inside
model.TagSummary is left alone. A regression test drives the two-identical-
schemas case and asserts both halves: Tag.java is not emitted, and Pet
names Category.
That is also what this change documents. The appendix's file tree listed
Tag.java among the emitted models and the listings beside it came from a
two-field test fixture, so the chapter described neither what the goal
emits nor what it emitted here. Both listings are now the goal's output for
the cut-down Petstore spec committed alongside, the file tree is what that
run actually wrote, and the missing Tag.java is explained rather than
quietly absent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7f2f6c8fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
decodeJson keeps the HTTP code when the body is empty or unparseable and returns no errors and no data, so the HTTP-range check added last round still let that land on "No hero for that episode". getResponseErrorMessage() is the one accessor that is null only on a clean success -- it carries the first GraphQL error, the transport failure, or the parse failure -- so that is what the guard reads now, with the range check kept for an error status whose body happened to decode. And the openapi appendix claimed the committed files come back byte for byte from a rerun, which they do not: they carry a license header and the tag:: markers the guide includes them by, and emitApi/emitModel write neither. Said plainly instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
…w checks check-copyright-headers accepts the short Oracle-era header only on files a change does not touch. Adding the regression test made this one modified, so it needs the complete Codename One header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4082fadd47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
All three appendices document both emission modes, and all three call sites were written against the record output: constructors with arguments, reads through accessors. On a Java 8 target the generator writes classes with a public no-arg constructor and public fields, so none of those lines compile. Each call site now carries a note saying which shape it shows and what changes on the other, which is smaller than duplicating three listings and keeps the compiled examples matching what the docs module actually builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
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
1 participant
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.
The three
generate-*appendices each stop at a colon where a listing should be — six of them:@GraphQLClientinterface looks like:"GraphQLSubscriptionhandle whosecancel()ends the stream:"@GrpcClientinterface looks like:"The listings are generated, not written
Rather than hand-write something that looks like generator output, this runs the generators.
GenerateGraphQLMojo.Generator,GenerateGrpcMojo.GeneratorandGenerateOpenApiMojo.Generatorwere driven against the exact schema / proto / spec the chapters describe — the same fixtures their own unit tests use — and the output committed underdocs/demos/common/src/main/java/com/example/{starwars,hello,petstore}, with only// tag::markers added.So the book shows the generators answer, and a change in emission shape lands here as a diff instead of quietly making the chapter wrong.
It caught one
The openapi chapter says models are emitted as
@Mappedrecord (Java 17+) or class (Java 8). ThePet.javait pointed at was aPropertyBusinessObjectwithProperty<Long, Pet>fields — nothing the generator has ever written. The real one is:PetApi.javawas closer but still not the emitted form. Both are now the generators output.The call sites are hand-written
A generator does not emit those. Each carries the one thing its response type gets wrong by default, in a comment beside the branch:
getResponseCode()(the gRPC status) is what to read.isOk()is about the errors array, not about whethergetData()is null.Gates
check-missing-code-blocksratchet 34 → 28validate-guide-snippets.py— 1113 include-backed blocks, up from 1107com/example/**source compiles under JDK 17 againstcodenameone-coreasciidoctor --failure-level WARNandasciidoctor-pdf— cleanstatus: ok,total: 0on eachOne thing for a separate change
The GraphQL generator emits
import com.codename1.annotations.JsonProperty;into every response record that never uses it. Harmless, and visible in the committed output — but fixing it is a plugin change, not a docs one.