Skip to content

52 - added new property#73

Open
tomsontom wants to merge 5 commits into
mainfrom
52-typo-in-nativetypesubstitues-should-be-nativetypesubstitutes
Open

52 - added new property#73
tomsontom wants to merge 5 commits into
mainfrom
52-typo-in-nativetypesubstitues-should-be-nativetypesubstitutes

Conversation

@tomsontom

Copy link
Copy Markdown
Member

No description provided.

@tomsontom tomsontom linked an issue Jun 29, 2026 that may be closed by this pull request
@tomsontom

Copy link
Copy Markdown
Member Author

Review

Overview

This PR (branch references "typo in nativeTypeSubstitues") does two things:

  1. Renames the misspelled nativeTypeSubstitues config property to nativeTypeSubstitutes across all four Java generators (java-client-api, java-rest-client-jdk, java-server-jakarta-ws, java-server), keeping the old name as a @deprecated field that's transparently migrated (with a console warning) into the new one.
  2. Extends the substitution model: nativeTypeSubstitutes changes from Record<string, string> (type name → Java FQN) to Record<string, {type, fromJson, toJson}>, letting a native-type substitution declare custom (de)serialization methods instead of assuming a .of(String) factory / .toString() instance method. A new generated _ScalarSupport class centralizes <Scalar>FromJson/<Scalar>ToJson for every scalar (substituted or not), and all JSON (de)serialization call sites across the four generators were switched to go through it instead of calling Type::of/value.toString() directly.

The java-test/ sample projects were regenerated to reflect this (MyRange now exposes static parse/toString(MyRange) instead of instance toString()), and a new scalar-support.test.ts covers the new generator module.

Code quality / correctness

Overall the refactor is well-executed — mechanical rename applied consistently, backward-compat shim is reasonable, and the new _ScalarSupport abstraction is a clean way to decouple JSON marshalling from the domain type. New unit tests for scalar-support.ts are solid.

Found two real inconsistencies, both because the refactor didn't fully complete in a couple of spots:

  1. Bug — stale Type::of calls not migrated to _ScalarSupport (correctness risk)

    • packages/cli/src/java-rest-client-jdk/service.ts (generateParameterContent, scalar branch, ~line 1190) and packages/cli/src/java-server-jakarta-ws/resource.ts (generateParameterContent, scalar branch, ~lines 1267-1275) still emit ${Type}::of for scalar-variant parameters, where Type is computeParameterValueType(...).
    • Everywhere else in this PR, scalar (de)serialization was rerouted through _ScalarSupport::<Scalar>FromJson/ToJson specifically so a substitute type doesn't need a static .of(String) method (the whole point of adding fromJson/toJson config). These two call sites were missed, so a scalar substituted with e.g. fromJson: "parse" (like the MyRange/Range sample itself) would generate Java that calls a non-existent MyRange.of(...) — a compile error — if this code path is ever exercised (grouped/inline parameter data objects, via generateServiceData). It isn't hit by the current Range test spec, so CI won't catch it — worth fixing for consistency and to avoid a landmine for the next scalar substitution added.
  2. Bug — broken log message after property rename (cosmetic, low severity)

    • packages/cli/src/java-server/generator.ts:76: `Using native ${artifactConfig.nativeTypeSubstitutes[t.name]}` — this used to log the substitute type string directly; now nativeTypeSubstitutes[t.name] is an object {type, fromJson, toJson}, so this will print Using native [object Object]. The equivalent code in java-client-api/generator.ts:52 was correctly updated to append .type; this one wasn't.

Other observations (not blocking)

  • Deprecation migration block is duplicated verbatim in all four generator.ts files (java-client-api, java-rest-client-jdk, java-server-jakarta-ws, java-server). Given it's temporary scaffolding for backward compat, a shared helper (e.g. in java-gen-utils.ts) would reduce duplication and the risk of exactly the kind of copy-paste drift that produced bug Add MessagePack support #2 above.
  • Behavior change worth flagging in the PR description: MyRange (and any other substituted scalar using the :: toJson form) no longer overrides Object#toString() — it's now a static toString(MyRange) helper. Any code elsewhere relying on myRange.toString() / implicit string conversion ("" + range, logging, String.valueOf) will silently fall back to Object's default representation. Fine for the sample project, but worth calling out since it's a behavioral change for consumers of substituted scalars generally.
  • .vscode/launch.json gained an unrelated Main launch config for person-java-client — seems like local IDE config that leaked into the commit; harmless but not related to this change.

Test coverage

Good: scalar-support.test.ts covers default methods, substitute methods (simple, ::, and dotted-qualified forms), and the _ScalarSupport aggregate class. No test exercises the generateParameterContent/generateServiceData path directly, which is exactly the path with bug #1 — adding a case there (or extending the ScalarSubstition_Service sample to hit it) would have caught this.

Security

No concerns — this is purely codegen for scalar (de)serialization; no new user input handling or injection surface introduced.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

typo in "nativeTypeSubstitues" should be "nativeTypeSubstitutes"

1 participant