Fix Java map code generation#267
Conversation
Signed-off-by: Aayush Tiwari <aayushtiwari1001@gmail.com>
apoorv7g
left a comment
apoorv7g
left a comment
There was a problem hiding this comment.
Further Review Required from @mttrbrts and if possible @DianaLease and/or @ekarademir .
There was a problem hiding this comment.
@Aayush10016 Thank you for fixing this issue. Good approach. I wanted to know if there is a workaround to fix this without any breaking changes in the javavisitor.js. I believe this might be a breaking change, @mttrbrts can you please review this when you have, overall it looks good to me.
mttrbrts
left a comment
mttrbrts
left a comment
There was a problem hiding this comment.
Thanks for the tag @apoorv7g, I would argue that previously code generation for Java where a model contains Scalars and Maps was totally broken so this is a bug fix, not a breaking change.
There's probably a more elegant way to support scalar constraints in Java, but let's fix the bug first, and we can tighten the codegen another time.
| protobuf: 'Empty enum produces invalid Protobuf (enum must have >= 1 item) and scalar map keys (e.g. SSN) are not valid Proto3 map key types', | ||
| graphql: 'map types emit invalid GraphQL SDL (key/value field syntax)', | ||
| rust: 'map declarations reference scalar key/value types (e.g. SSN, Time) that the Rust visitor does not emit, so cargo check fails', | ||
| java: 'map declarations reference scalar key/value types (e.g. SSN, Time) that the Java visitor does not emit, so javac fails', |
There was a problem hiding this comment.
Pull request overview
This PR fixes Java code generation for Concerto map declarations so the generated output compiles when map keys/values involve scalar aliases (e.g., SSN -> String) and when map member types (e.g., enums) must be imported even though the map declaration itself is not emitted as a Java class.
Changes:
- Generate
Map<K, V>consistently for map fields and their getters/setters (eliminating prior type mismatches). - Unwrap scalar aliases used as map keys/values when generating Java generic arguments.
- Avoid importing Concerto scalar/map declarations (which are not emitted as Java types), while adding imports for required non-scalar map member types.
- Re-enable the Java compile verification case by removing the Java skip reason for the affected fixture.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/codegen/fromcto/java/javavisitor.js |
Updates Java generation to compute map field/getter/setter types as Map<K,V>, unwrap scalar aliases for map members, and refine import emission. |
test/codegen/fromcto/java/javavisitor.js |
Adds unit tests to assert map accessor signatures use resolved Map<K,V> and that scalar aliases in map members are unwrapped. |
test/codegen/__snapshots__/codegen.js.snap |
Updates expected generated Java to remove invalid imports (scalar/map decls), add required enum imports, and reflect corrected map types in fields/accessors. |
test/verification/cases.js |
Removes the Java skip note for the HR integration verification case now that javac should succeed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes generated Java for Concerto map declarations that use scalar aliases or imported map member types.
The Java visitor now:
Map<K, V>consistently for map fields, getters, and settersSSN->Stringhr_integrationJava compile verification case that was previously skipped for this bugValidation
npm.cmd cinpm.cmd run mocha -- test/codegen/fromcto/java/javavisitor.jsnpm.cmd run test:verify -- --grep "generated Java"npm.cmd run mocha -- test/codegen/codegen.jsnpm.cmd run test:verifynpm.cmd run lintnpm.cmd test -- --grep JavaVisitorwas also attempted, but the repository's global coverage gate still runs for filtered tests, so the focusedmochacommand above is the valid JavaVisitor test signal.Fixes #265.