Iceberg REST Phase 1 [1/3]: Read-only contract and codegen - #689
Conversation
Vendor the upstream Apache Iceberg REST OpenAPI spec (v1.10) and wire Polaris-aligned codegen into the services/tables Gradle build: - spec/iceberg-rest-catalog-open-api.yaml: upstream spec as source of truth - Gradle tasks: setUpOpenApiCliForIcebergRest, validateIcebergRestOpenApiSpec, generateIcebergRestOpenApiServer with importMappings/typeMappings to map spec schemas to real Iceberg library types (no model generation) - Post-processing for Iceberg 1.10-only types mapped to Object (compatibility with our 1.5.2 fork) - compileJava depends on codegen; check depends on spec validation - jackson-databind-nullable dependency for generated code
- Strip non-upstream comments (CODE_COPIED_TO_POLARIS, version marker) so the vendored spec is byte-for-byte identical to apache-iceberg-1.10.0 - Add verifyIcebergRestSpecSync Gradle task that downloads the upstream spec from the pinned tag and fails if it differs from the vendored copy - Wire verifyIcebergRestSpecSync into the check task so CI catches drift
Generate the supported profile deterministically and resolve the generator through Gradle so unsupported APIs cannot leak into the server surface.
Drop the full upstream vendored YAML and Python profile generator from the build surface so codegen depends on one constant Phase 1 contract file.
Add a maintainer script and Gradle task that fetch an Iceberg tag, rebuild the Phase 1 allowlisted profile, and refresh the pinned checksum without making the build depend on network or Python regeneration.
Check in the complete Apache Iceberg 1.11 REST contract with x-openhouse-support markers, assert every operation is annotated, codegen only supported ops, and drop the upgrade script.
Drop unsupported markers and the YAML checksum pin so upgrades are a plain merge plus x-openhouse-support: supported where needed.
Keep services/tables/build.gradle lean and simplify filtering by retaining full OpenAPI components while still codegening only supported operations.
|
Gradle LOC cleanup: Iceberg REST OpenAPI build logic now lives in |
mkuchenbecker
left a comment
mkuchenbecker
left a comment
There was a problem hiding this comment.
This is part of a coordinated review of the #689/#690/#691 stack (architecture, Iceberg REST spec conformance, and testing passes). It was generated based on the review skills at https://github.com/mkuchenbecker/code-review-skills — linked so the criteria being judged are inspectable: arch-review (module topology, contracts, and the error channel), testing-review (testing as falsification: tests derived from claims, suites judged by diffing the tests that should exist against those that do), plus an Iceberg REST spec conformance pass against the vendored YAML.
Verdict for this PR: the contract-first shape is right, and the vendored YAML is byte-identical to upstream apache-iceberg-1.11.0 apart from the header comment and the four x-openhouse-support annotation lines, and the SHA-256 recorded in its header matches the upstream file. Two things need attention: the mechanism's central safety claim ("marking an operation supported fails compilation until the facade implements it") is not enforced by the build, and nothing verifies the vendored spec stays faithful to upstream after future merges. One structural follow-up on the convention plugin.
3 inline comments: 1 fix-before-merge, 1 fix-before-enabling, 1 follow-up.
Generated by Claude Code
| '--global-property', | ||
| 'apis,models=CatalogConfig:ListTablesResponse,apiTests=false,apiDocs=false,modelTests=false,modelDocs=false,supportingFiles=', | ||
| '--additional-properties', | ||
| 'interfaceOnly=true,useTags=true,skipDefaultInterface=false,hideGenerationTimestamp=true,useSpringBoot3=false,openApiNullable=false' |
There was a problem hiding this comment.
Fix before merge — the documented codegen guardrail does not exist. The spec header (lines 26–31) and docs/iceberg-rest-catalog.md both state that marking an operation x-openhouse-support: supported without implementing it "fails compilation". With skipDefaultInterface=false on this line, every generated operation is a Java default method returning an empty HTTP 501 — so it compiles, is added to SUPPORTED_ENDPOINTS, and is advertised as supported by GET /v1/config; clients that trust the advertisement call it and get a bodyless 501. Second hole: an operation under a new spec tag generates an interface no class implements, so no route is registered at all. Fix: set skipDefaultInterface=true so generated methods are abstract (restoring the documented compile failure), add a startup assertion that every SUPPORTED_ENDPOINTS entry has a registered Spring route (covers the new-tag hole), and align the two prose claims.
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| # | ||
|
|
There was a problem hiding this comment.
Fix before enabling — the recorded upstream SHA is a comment nobody checks. The header records the SHA-256 of the upstream Iceberg 1.11.0 file, and it is correct today: stripping the header and annotation lines yields a file whose hash matches the recorded value. But no build task recomputes it, so a conflict resolution during the next spec upgrade can silently diverge the vendored contract while the header keeps asserting fidelity. Fix: a ~10-line task wired into check — strip the header comment and the x-openhouse-support lines, hash, compare against the recorded value, and fail with the upstream URL in the message. No network access needed.
| * Codegens Spring interfaces for supported ops only and emits | ||
| * IcebergRestOpenHouseSupport.SUPPORTED_ENDPOINTS for GET /v1/config. | ||
| */ | ||
| def icebergRestSpec = rootProject.file('spec/iceberg-rest-catalog-open-api.yaml') |
There was a problem hiding this comment.
Follow-up — the shared convention plugin hardcodes its single consumer. The spec path here, plus the Java package and support-class names below, embed one project's identity in a shared buildSrc plugin, and generated sources are attached by mutating compileJava.source rather than registering a source directory. Lift these into plugin extension properties with the current values as defaults, and register the generated directories via sourceSets.main.java.srcDir(...) so the task dependency flows through builtBy.
|
@cbb330 , Is there a design doc articulating the points of integration for IRC, known compatibility issues & path to resolution? any internal docs and sharing internally within team channel is good. |
3 participants
Summary
Part 1 of 3 for a read-only Iceberg REST Catalog facade.
Checks in the full Apache Iceberg 1.11.0 REST OpenAPI. OpenHouse support is opt-in via
x-openhouse-support: supportedon operations (missing = unsupported). The build codegens only supported operations; marking support or changing a supported signature fails compilation until the facade implements it. No YAML checksum pin.Changes
Internal API Changes
x-openhouse-support: supportedmarkers.IcebergRestOpenHouseSupport.SUPPORTED_ENDPOINTSfrom supported ops only.Upgrade model
Merge newer upstream YAML → add
x-openhouse-support: supportedwhere needed → compile. Unsupported ops need no annotation, so merges stay quiet.Testing Done
Runtime tests are in #690 / #691.
Additional Information
Merge in order: