feat!: regenerate the SDK from the enhanced generator - #131
Conversation
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
d3e4357 to
caa8a10
Compare
mridang
changed the title
8f71464 to
fa58f11
Compare
Regenerate the client from openapi-generator-plus with modernized templates, authenticators ported to the new interfaces, and house tooling aligned to the generator's output. BREAKING CHANGE: new generated API surface, a raised minimum runtime, and updated dependencies; not source-compatible with the prior release.
fa58f11 to
0020a93
Compare
Both were referenced nowhere: Version held a stale hardcoded VERSION constant and StringUtil reimplemented String.join.
Reorganize .openapi-generator-ignore into the shared section convention and sort entries alphabetically within each section. No kept paths added or dropped.
Move getPrivateKeyFromString into WebTokenAuthenticator and buildHostname into OpenId, NoAuthAuthenticator and PersonalAccessTokenAuthenticator as private helpers, then delete the shared utils package. Other language SDKs inline these too.
Drop lefthook from devbox, gitignore devbox.d, and trim the keep-list: remove the already-gitignored .env and .idea entries and normalize directory globs from dir/** to dir/.
Regeneration syncs the stale Client references in SKILLS.md to the generated Zitadel facade.
The CMD points jshell at target/lib/* but mvn install never copied dependencies there, so the REPL failed with 'target/lib/* not found'. Add a dependency:copy-dependencies step to the build.
Override toString on token/secret-holding authenticators so secrets and cached tokens are masked as *** instead of leaking into logs, matching the Python, PHP and Ruby SDKs.
Regenerated against the generator that now redacts the bearer token in BearerAuthenticator and ZitadelAccessTokenAuthenticator toString(), so the masking is template-driven and survives regeneration instead of being reverted. Keep-list the bespoke AuthenticatorRedactionTest so the prune step preserves it.
Regenerated against the generator that redacts the bearer token in BearerAuthenticator and ZitadelAccessTokenAuthenticator toString(). Replace the aggregate AuthenticatorRedactionTest with one redaction test per bespoke authenticator, each in its own test file (mirroring the Python, PHP, Ruby and Node SDKs); keep-list them so the prune step preserves them.
aymenmehri
left a comment
aymenmehri
left a comment
There was a problem hiding this comment.
Not a maintainer — just a user of this SDK who went through the diff because it has been sitting for a while. Filtering out the 1377 generated model/ and api/ files leaves ~97 reviewable ones, so this is feedback on those plus the build config. Hope it is useful; happy to be wrong on any of it.
What clearly improves
- Test coverage goes from 18 to 215
@Testmethods. That alone is worth a lot on a client SDK. - Generated models shrink roughly 4x —
UserServiceAddHumanUserRequestdrops from 608 to 147 lines — with serde centralized inObjectSerializer. This is the outcome #44 was after, even though it landed via a serializer rather than theZitadelModelbase class the issue sketched. Worth saying so explicitly when closing it, since the mechanism differs from what was proposed. - The typed exception hierarchy (
errors/BadRequestException,UnauthorizedException,NotFoundException, …) is a real ergonomics win over catchingApiExceptionand switching on an int. - The redirect hardening in
DefaultApiClientis genuinely good: refusing non-HTTP(S) redirect schemes, strippingAuthorizationon an HTTPS→HTTP downgrade, and refusing to replay a request body across that downgrade. Same for preserving the transport cause viaApiException(String, Throwable).
Blocking, in my view
1. The PR silently requires Java 25
pom.xml sets <release>25</release>, up from 11 on main, and .github/workflows/integration.yml collapses the matrix from [ '11', '17', '21' ] to [ '25' ]. Neither the PR description nor the README mentions this.
Concretely, on Temurin 21.0.11 the build does not start:
[ERROR] error: invalid flag: -Xlint:-dangling-doc-comments
[ERROR] Failed to execute goal ...maven-compiler-plugin:3.14.0:compile
For an application this would be a maintainer's call. For a client library it means every consumer must be on Java 25 to use the SDK at all — <release>25</release> puts class file major version 69 in the published jar. Java 11, 17 and 21 are where nearly all deployed workloads sit, and the compatibility matrix no longer proves the SDK works on any of them.
If the Java 25 syntax in use is what forces this, most of it degrades cheaply: @java.io.Serial is Java 14+ and removable, switch expressions are Java 14+. A <release>17</release> or 21 baseline looks reachable. If Java 25 is a deliberate product decision, it needs to be in the PR title/body, the README, and the release notes, because it is a far bigger breaking change than the API rename that feat! currently signals.
2. Closes #113 does not hold
#113's first task is "Remove usage of any v2beta/ endpoints". The branch still issues 143 calls to v2beta service paths, byte-identical to main:
// src/main/java/com/zitadel/api/BetaOrganizationServiceApi.java
String path = "/zitadel.org.v2beta.OrganizationService/CreateOrganization";git grep -c v2beta returns the same 143 hits on both branches, and the ten Beta*ServiceApi classes are all still there. Since V5 removes those endpoints, merging this as-is would break the SDK against precisely the release #113 exists to support. This reads like it needs its own PR, and #113 should stay open.
3. Closes #83 is addressed only incidentally
BaseApi.throwApiException builds the message as:
String message = "API returned status code " + code;So the root cause is still not in the message — "Error 400" merely became "API returned status code 400". The reporter's body content does surface, but only because ApiException.getMessage() was overridden to dump the whole object:
return "ApiException{" + "statusCode=" + statusCode + ", message='" + super.getMessage()
+ "', responseHeaders=" + responseHeaders + ", responseBody='" + responseBody + '\'' + '}';Two problems with relying on that:
- It is a
toString()wearinggetMessage()'s clothes. Every log line and every stack trace header now carries the full header map and full body inline, which is rough on anyone aggregating logs. - It inlines every response header into the exception message. That sits awkwardly next to the three commits in this PR that redact secrets in authenticator
toString()— the same reasoning applies here, and response headers are not guaranteed to be free of sensitive values.
What #83 actually asked for was "Error 400: invalid OrganizationNameQuery.Name: value length must be between 1 and 200 runes, inclusive" — pull message out of the parsed errorBody in throwApiException and append it, then leave getMessage() alone and keep the dump in toString().
4. Minor: the README keeps a stale requirement
The rewritten README still says "Ensure you have Java 8 or higher installed" (line 39), which was already wrong against <release>11</release> and is very wrong against 25. I opened #136 / #137 against main for that line; whichever lands second should carry the fix, otherwise this PR reintroduces it.
Summary
The transport, error model and test story here are a solid step up, and I would happily use this SDK. The Java 25 baseline is the one I would not merge without an explicit decision, and I think #113 and #83 should stay open rather than be closed by this PR.
2 participants
Regenerates the SDK from the enhanced openapi-generator-plus, with bespoke authenticators ported to the new interfaces and full unit + integration suites passing locally.
Closes #44
Closes #113
Closes #83