Repository navigation
fix(protobuf): generate valid scalar map fields - #298
Shubham-Padkonde wants to merge 2 commits into
Conversation
Signed-off-by: Shubham Padkonde <shubhampadkonde12@gmail.com>
|
Thank you for this contribution, and welcome to the Accord Project. Apologies for the wait. I have approved the CI runs, which were waiting for approval, so you should see the Build, Verify Codegen and CodeQL results shortly. A maintainer will review the change once they are in. If anything fails, please push a fix to this branch and we will take another look. Drafted by an Accord Project agent, reviewed by a maintainer. |
|
Thanks for approving the workflows. All 27 reported checks are now passing on c174e6f, including the Node 22/24 Linux, macOS and Windows matrix, protobuf verification, and CodeQL. The PR is ready for the maintainer review you mentioned. |
![mira-accord-project[bot]](https://avatars.githubusercontent.com/in/5553948?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for this fix, Shubham, and for the care you took with it. Map fields now use the property name rather than the map declaration name, they no longer carry the invalid optional rule, scalar keys and values resolve through the map's own model file (so imported maps work), and the snapshot changes all follow from the fix. The new protoc check and the narrowed skip reason in cases.js are good additions. One case is still invalid Proto3: a map keyed by DateTime, or by a scalar that extends DateTime. Handling it, or deliberately documenting it, along with a test would make this ready for a maintainer's review.
Nothing blocking; some suggestions below. Findings: 1 should-fix, 1 nit.
- nit · tests ·
test/codegen/fromcto/protobuf/maps.js: The new tests coverStringkeys with scalar andDateTimevalues, but not aDateTimekey or a scalar extendingDateTimeas the key. That is the one key type still invalid in Proto3. A unit test for it, and a matching map in theprotocverification model inprotobuf.validate.test.js, would catch this.
Automated review by Mira, an Accord Project agent, at c174e6f. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.
| type = declaration.getType(); | ||
| } | ||
| } | ||
| return ModelUtil.isPrimitiveType(type) ? this.concertoToProto3PrimitiveType(type) : type; |
There was a problem hiding this comment.
should-fix · correctness
A map whose key is DateTime (or a scalar extending DateTime) still emits map<google.protobuf.Timestamp, V>. Proto3 only allows integral or string types as map keys, never message types, so protoc rejects this. Concerto permits String and DateTime keys, so the case is reachable and sits close to what this PR is fixing. Suggest mapping a DateTime key to string (an ISO-8601 representation) for the key position only, with a short comment explaining why, or otherwise documenting that such maps are unsupported. If you would rather leave it for a follow-up, please say so in the PR description.
Signed-off-by: Shubham Padkonde <shubhampadkonde12@gmail.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
Thank you, Shubham-Padkonde, for the quick follow-up. The new commit addresses the earlier point about DateTime map keys: in key position both DateTime and scalars extending it now become string, while values stay google.protobuf.Timestamp, and the timestamp import is always written. Resolving scalars through the map's own model file means imported maps work too. Using the field name instead of the map declaration name, and dropping optional on map fields, also removes the name clashes and the invalid field rule. The unit tests in maps.js cover local and imported scalar keys, DateTime keys, and message and enum values, and the updated verification skip reason is now accurate. I found nothing further that needs changing. The earlier review thread on the DateTime key can be resolved, as it is now addressed. This looks ready for a maintainer to look at once the full test and verification workflows have run.
No problems found.
Automated review by Mira, an Accord Project agent, at 281b9cd. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.
Related to #249.
Changes
Protobuf map fields now omit the illegal
optionallabel and resolve Concerto scalar keys and values to their underlying Protobuf types. For example, an optional map from anIdentifier extends Stringscalar to anAmount extends Doublescalar now generatesmap<string, double> amounts = 1;.Map fields also use the property name instead of the map declaration name, so two properties using the same map type no longer generate duplicate field names. Field numbers are unchanged. Scalar lookup uses the map's declaring model, including when that map is imported by another namespace.
Added four regression tests using real models and a
protoccompilation regression. Updated the affected Protobuf snapshot and existing map tests.Validation
Field labels (required/optional/repeated) are not allowed on map fieldsand passes after it.npm run buildandnpm run test:esmpass.npm test: 701 passing, 52 pending; lint, dependency/lockfile checks, license checks and JSDoc pass. The command still exits nonzero on the repository-wide branch-coverage threshold (95.91% against 96%).af4fa2568c0af3e501614079f1f35039e88dcfaa, tested separately with the same dependencies and tools: 696 passing, 52 pending, and the same 95.91% branch coverage / threshold failure. The threshold is unchanged in this PR.protoc36.2. The 52 pending cases are existing skips or require unavailable external compilers.Flags
Generated map property names change to match their CTO property names; consumers of generated source/JSON field names should account for that correction. This does not renumber fields.
The full HR verification cases remain skipped for existing empty-enum and other generator limitations. This PR addresses the map-field defects; it does not claim every HR schema now compiles or add support for DateTime map keys.
Author Checklist
mainfrom a fork branch.November 10 review fix
DateTime map keys, including imported scalar aliases, now generate string keys because Proto3 does not permit Timestamp messages as map keys. DateTime values remain google.protobuf.Timestamp. Added direct and imported-scalar regression tests plus protoc validation.
All 703 tests pass, with 52 pending; both new tests fail before the fix. Lint passes. The full command still fails the branch-coverage threshold: 95.92% versus 96%; unchanged c174e6f also fails at 95.91% with 701 tests passing. No thresholds were changed. Published signed commit 281b9cd. Prepared with OpenAI Codex assistance.