Skip to content

[adr] Java CDDL generator proposal#17692

Open
pujagani wants to merge 2 commits into
SeleniumHQ:trunkfrom
pujagani:adr-java-cddl
Open

[adr] Java CDDL generator proposal#17692
pujagani wants to merge 2 commits into
SeleniumHQ:trunkfrom
pujagani:adr-java-cddl

Conversation

@pujagani

Copy link
Copy Markdown
Contributor

🔗 Related Issues

💥 What does this PR do?

Adds ADR recording the decision to generate Java WebDriver BiDi bindings from the CDDL spec.

🔧 Implementation Notes

The three cross-binding decisions recorded here : factory construction, subscription cancellation handles, and unified modules. These have user-visible consequences that needed to be settled before implementation begins. Python and JavaScript already generate from the same CDDL source. This record closes the loop on the Java binding's approach and captures the rejected alternatives (static utility pattern, raw listener IDs, split command/event classes) so they don't resurface in review.

Full generator design proposal: https://gist.github.com/pujagani/0a62197fd7c9b6d120e3a658fd7381d0

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude
    • What was generated: The spec and the ADR
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

The @beta annotation on all existing hand-written BiDi classes ships in the next release, giving users one release of warning before the cutover PR removes them.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

@pujagani pujagani marked this pull request as ready for review June 19, 2026 13:05
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add ADR for generating Java WebDriver BiDi bindings from CDDL
📝 Documentation 🕐 10-20 Minutes

Grey Divider

Description

• Document decision to generate Java BiDi bindings from shared CDDL-derived JSON artifacts.
• Standardize Java BiDi API shape: factory-created modules, unified domains, cancellable
 subscriptions.
• Record rejected alternatives and outline migration/breaking changes under @Beta.
Diagram

graph TD
  A["CDDL BiDi spec"] --> B["JS pipeline"] --> C[("bidi-ast.json / bidi-model.json")] --> D["Java generator"] --> E["Generated Java BiDi"]
  E --> F["Factory modules"] --> G["Unified domains"] --> H["Subscription handles"]

  subgraph Legend
    direction LR
    _db[(Artifact)] ~~~ _proc[Process] ~~~ _out[Output]
  end
Loading
High-Level Assessment

The PR’s approach is appropriate for an ADR: it clearly records the key cross-binding API-shape decisions (generation from CDDL-derived JSON, unified domain modules, factory construction, and cancellable subscription handles) and captures the main rejected alternatives to prevent re-litigating them during implementation.

Files changed (1) +204 / -0

Documentation (1) +204 / -0
17692-java-bidi-cddl-generator.mdAdd ADR proposing Java BiDi generation from CDDL-derived JSON model +204/-0

Add ADR proposing Java BiDi generation from CDDL-derived JSON model

• Introduces an architecture decision record to move Java WebDriver BiDi bindings from hand-written code to generated sources driven by CDDL-derived JSON artifacts. Defines API-shape standards (factory creation with HasBiDi checks, unified command/event domain modules, and AutoCloseable subscription cancellation handles) and documents migration consequences and rejected alternatives.

docs/decisions/17692-java-bidi-cddl-generator.md

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 11 rules

Grey Divider


Informational

1. Unnumbered ADR title 🐞 Bug ⚙ Maintainability
Description
The new decision record’s top-level heading omits the decision number, diverging from the
repository’s ADR template convention and making cross-referencing inconsistent. This is a
documentation hygiene issue (not a functional/code defect).
Code

docs/decisions/17692-java-bidi-cddl-generator.md[1]

+# Java WebDriver BiDi CDDL generator proposal
Evidence
The ADR template explicitly shows decision records should have a numbered heading, while the newly
added ADR uses an unnumbered heading.

docs/decisions/0000-template.md[1-8]
docs/decisions/17692-java-bidi-cddl-generator.md[1-5]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR template in `docs/decisions/0000-template.md` indicates decision records should have a numbered H1 heading (`# NNNN. ...`). The new ADR’s H1 lacks the number, which reduces consistency when people cite decisions.

## Issue Context
- File is already numbered in its filename (`17692-java-bidi-cddl-generator.md`), but the H1 header does not include `17692.`.

## Fix Focus Areas
- docs/decisions/17692-java-bidi-cddl-generator.md[1-1]

### Suggested change
Update line 1 to something like:
- `# 17692. Java WebDriver BiDi CDDL generator proposal`

(Or whatever numbering convention you want to standardize on, but keep it consistent with the template.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@diemol diemol added the A-needs decision TLC needs to discuss and agree label Jun 19, 2026
@titusfortner

Copy link
Copy Markdown
Member

I think the manifest is a bad idea and easily avoidable. We don't need to store orchestration logic in the generated module classes. Keep the generated low-level in its own namespace and let a hand-written, checked-in coordination layer import it rather than be injected into it. If the generated code lands in a new namespace, we don't need to ensure parity with existing code. The low level things should all be private API, and any high level implementations can move to the new implementation. The CDDL spec should be the oracle, not the existing implementation.

@titusfortner

Copy link
Copy Markdown
Member

The Type Generation section covers the common cases well (enums from string-literal unions, POJOs from named groups, param builders), but a handful of CDDL shapes don't fit those patterns and tend to fail silently and need to be accounted for.

  • Inline string-literal unions on a field. The plan generates enums from named string-literal unions, but some fields have the union inline (e.g. cacheBehavior, orientation, Initiator.type, ClientWindowInfo.state). These should still become enums — easy to special-case only the single-literal form and let multi-literal fields fall back to String, which drops the allowed values.
  • Commands whose params are a union, not a record. A few commands don't have a flat parameter object: either a variant group nested inside the params (continueWithAuth, setClientWindowState, setGeolocationOverride, handleRequestDevicePrompt) or params that are a top-level union (session.unsubscribe). A POJO/builder pass that expects named members walks right past these and emits a command with missing or empty parameters.
  • Cross-field rules the builder can't express. Some commands have constraints beyond per-field types: a field required only for a given discriminator value (continueWithAuth: credentials required when action=provideCredentials) and exactly-one-of (session.unsubscribe: events xor subscriptions). Worth deciding up front whether these are enforced by overloads, sealed types, or a runtime check — a plain fluent builder allows the invalid combinations.
  • Enum wire values must come from the spec, not the constant name. Some values are hyphenated (powered-off, subscribe-to-notifications) and can't be reconstructed from a SCREAMING_SNAKE constant by case conversion. The toString() → wire-value mapping needs the literal copied from the AST.
  • Degenerate result unions. Many command results are a union with a single member or just an empty result. Worth collapsing these rather than generating a one-variant type + dispatch for each.
  • Open-map / extension construct (* text => any). Beyond the {* text => T} → Map mapping, CDDL allows arbitrary extra keys on some types. Decide whether the generator drops unknown keys or round-trips them.
  • A coverage assertion in the generator. Given several of the above fail silently, I'd add a check that errors if any command, event, or field in the model isn't emitted. The Phase 8 diff against the existing hand-written classes won't catch these, since a couple of these commands aren't implemented there either.

@titusfortner

Copy link
Copy Markdown
Member

Actually I think this belongs in the bidi-model generation, let me whip up a PR.

@titusfortner

titusfortner commented Jun 21, 2026

Copy link
Copy Markdown
Member

The PR that addresses my previous comments is here: #17700

@titusfortner titusfortner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for proposing this with all the details.

Per our ADR guidance, ADRs aren't meant to cover "single-binding internals," so I've taken what I read as the purpose behind this, reframed it toward what I think is the right approach, and opened an alternate ADR: #17701.

I've described my substantive disagreements there, but a lot of it hinges on how we decide #17670.

I think we should:

  1. Decide #17670
  2. Close this in favor of #17701 and argue the concepts there
  3. Merge #17700 to make the generation easier
  4. The remaining specifics here should fall into an obvious place from there

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

Labels

A-needs decision TLC needs to discuss and agree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants