Skip to content

Commit 290cc58

Browse files
committed
feat(protobuf): structural import comparison and complete import test coverage
- Add structural message comparison for cross-import compatibility: areMessagesStructurallyCompatible compares message types by field numbers and wire types instead of fully-qualified names, matching Confluent behavior - Rewrite protobuf diff tests 37-43 with proper reference setup (register reference schemas as subjects, use references for registration and compat checks) - Add step definitions for schema registration and compatibility checking with references - Replace all io.confluent package references with com.axonops in tests - All 1292 BDD scenarios pass, zero @pending-impl remaining
1 parent 55573ed commit 290cc58

File tree

6 files changed

+398
-129
lines changed

6 files changed

+398
-129
lines changed

SESSION_RESUME.md

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66

77
---
88

9-
## Current State — ALL NON-PENDING BDD TESTS PASS
9+
## Current State — ALL BDD TESTS PASS (ZERO PENDING)
1010

11-
**1298 passing BDD scenarios** (0 failures) across 67+ feature files.
12-
- **1298 passing** (memory backend, in-process)
13-
- **7 tagged `@pending-impl`** (Protobuf import resolution, excluded from CI)
11+
**1305 passing BDD scenarios** (0 failures, 0 pending) across 67+ feature files.
12+
- **1292 passing** (in-process mode, excludes `@operational`)
13+
- **0 tagged `@pending-impl`** — all previously pending protobuf import tests now pass
1414

1515
### Phase Progress
1616

@@ -21,7 +21,7 @@
2121
| Phase 3: Protobuf Diff Tests | COMPLETE | 43 | Section 31 data-driven |
2222
| Phase 4-5: JSON Schema Diff | COMPLETE | 251 | Sections 27-29 data-driven |
2323
| Phase 6: JSON Schema Validation | COMPLETE | 40 | Section 30 reader/writer pairs |
24-
| Phase 7: Feature Implementation | COMPLETE || All checkers enhanced, aliases, fingerprint fix |
24+
| Phase 7: Feature Implementation | COMPLETE || All checkers enhanced, aliases, fingerprint fix, structural import comparison |
2525
| Phase 8: Feature BDD Tests | NOT STARTED || Tests for Phase 7 features |
2626

2727
### Key Fixes (All Sessions)
@@ -34,12 +34,8 @@
3434
6. **Avro fingerprint**: Include `default` in field fingerprint so schemas differing only in defaults are treated as distinct (fixes BACKWARD_TRANSITIVE/FULL_TRANSITIVE dedup bypass)
3535
7. **Avro aliases**: Field and record alias matching for backward-compatible renaming
3636
8. **JSON Schema test corrections**: Verified against Confluent — adding property to open content model IS incompatible (PROPERTY_ADDED_TO_OPEN_CONTENT_MODEL); closed model allows new properties
37-
38-
### Remaining 7 `@pending-impl` Scenarios
39-
40-
| Category | Count | Details |
41-
|----------|-------|---------|
42-
| Protobuf imports | 7 | Proto import resolution (`import "google.proto"`, custom imports) — fails at parse: `file not found: google.proto` |
37+
9. **Protobuf import tests**: Rewrote diff tests 37-43 with proper reference setup. All 7 pass.
38+
10. **Protobuf structural import comparison**: Message-typed fields now compared by structure (field numbers + wire types) instead of fully-qualified name. This handles cross-import compatibility where the same message structure appears under different package names, matching Confluent's behavior.
4339

4440
### Checker Enhancements
4541

@@ -51,11 +47,12 @@
5147
- 13 new check categories matching Confluent's JsonSchemaDiff
5248
- $ref resolution, composition, dependencies, constraints, open/closed model
5349

54-
**Protobuf** (`internal/compatibility/protobuf/checker.go`, ~660 lines):
50+
**Protobuf** (`internal/compatibility/protobuf/checker.go`, ~700 lines):
5551
- Required field removal detection (proto2)
5652
- Field-to-oneof: FIELD_MOVED_TO_EXISTING_ONEOF = incompatible
5753
- Real vs synthetic oneof distinction (proto3 optional)
5854
- Optional→repeated: only compatible for string/bytes/message
55+
- Structural message comparison for cross-import compatibility (`areMessagesStructurallyCompatible`)
5956

6057
**Avro Parser** (`internal/schema/avro/parser.go`):
6158
- Field fingerprint now includes `default` values to prevent dedup bypass
@@ -82,10 +79,10 @@ Source document: `CONFLUENT-bdd-test-scenarios.md` (~555 scenarios across 35 sec
8279

8380
```bash
8481
# In-process (fast, memory backend)
85-
BDD_BACKEND=memory go test -tags bdd -v -count=1 -timeout 15m ./tests/bdd/...
82+
go test -tags bdd -v -count=1 -timeout 10m ./tests/bdd/...
8683

87-
# Including pending-impl scenarios (to see what fails)
88-
BDD_BACKEND=memory BDD_TAGS="~@operational" go test -tags bdd -v -count=1 -timeout 15m ./tests/bdd/...
84+
# Docker mode (memory backend, includes @operational)
85+
BDD_BACKEND=memory go test -tags bdd -v -count=1 -timeout 15m ./tests/bdd/...
8986

9087
# Against Confluent
9188
podman compose -f tests/bdd/docker-compose.confluent.yml up -d --wait

internal/compatibility/protobuf/checker.go

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,14 +350,23 @@ func (c *Checker) areTypesCompatible(newField, oldField protoreflect.FieldDescri
350350
if newKind == oldKind {
351351
// Same kind - check message/enum types
352352
if newKind == protoreflect.MessageKind {
353-
return newField.Message().FullName() == oldField.Message().FullName()
353+
// Compare message types structurally (by field numbers and types) rather
354+
// than by fully-qualified name. Protobuf wire format encodes field numbers
355+
// and wire types, not type names. This handles cross-import compatibility
356+
// where the same message structure appears under different package names.
357+
return c.areMessagesStructurallyCompatible(newField.Message(), oldField.Message(), nil)
354358
}
355359
if newKind == protoreflect.EnumKind {
356360
return newField.Enum().FullName() == oldField.Enum().FullName()
357361
}
358362
return true
359363
}
360364

365+
return c.areKindsWireCompatible(newKind, oldKind)
366+
}
367+
368+
// areKindsWireCompatible checks if two field kinds are wire-compatible.
369+
func (c *Checker) areKindsWireCompatible(newKind, oldKind protoreflect.Kind) bool {
361370
// Wire-type compatible groups per official protobuf specification:
362371
// - Varint (wire type 0): int32, uint32, int64, uint64, bool
363372
// - Zigzag varint (wire type 0): sint32, sint64
@@ -414,6 +423,64 @@ func (c *Checker) areTypesCompatible(newField, oldField protoreflect.FieldDescri
414423
return false
415424
}
416425

426+
// areMessagesStructurallyCompatible checks if two message descriptors have
427+
// compatible field structures. For each field in the old message, the new
428+
// message must have a field with the same number and wire-compatible type.
429+
// This handles cross-import compatibility where the same message structure
430+
// may appear under different package names (Confluent behavior).
431+
func (c *Checker) areMessagesStructurallyCompatible(newMsg, oldMsg protoreflect.MessageDescriptor, visited map[messagePair]bool) bool {
432+
// Fast path: same message descriptor
433+
if newMsg.FullName() == oldMsg.FullName() && newMsg.ParentFile().Path() == oldMsg.ParentFile().Path() {
434+
return true
435+
}
436+
437+
// Recursion guard for self-referencing message types
438+
key := messagePair{newMsg.FullName(), oldMsg.FullName()}
439+
if visited == nil {
440+
visited = make(map[messagePair]bool)
441+
}
442+
if visited[key] {
443+
return true // Assume compatible for recursive types to break cycle
444+
}
445+
visited[key] = true
446+
447+
// Check each field in the old message has a compatible counterpart
448+
for i := 0; i < oldMsg.Fields().Len(); i++ {
449+
oldField := oldMsg.Fields().Get(i)
450+
newField := newMsg.Fields().ByNumber(oldField.Number())
451+
if newField == nil {
452+
// Field absent in new message — wire-safe (readers ignore unknown fields)
453+
continue
454+
}
455+
456+
oldKind := oldField.Kind()
457+
newKind := newField.Kind()
458+
459+
if oldKind != newKind {
460+
if !c.areKindsWireCompatible(newKind, oldKind) {
461+
return false
462+
}
463+
continue
464+
}
465+
466+
// Same kind — check deeper for message/enum types
467+
if oldKind == protoreflect.MessageKind {
468+
if !c.areMessagesStructurallyCompatible(newField.Message(), oldField.Message(), visited) {
469+
return false
470+
}
471+
}
472+
// For enums with same kind, wire format is always varint — compatible
473+
}
474+
475+
return true
476+
}
477+
478+
// messagePair is a key for tracking visited message pairs during structural comparison.
479+
type messagePair struct {
480+
newName protoreflect.FullName
481+
oldName protoreflect.FullName
482+
}
483+
417484
// checkNestedMessages checks compatibility of nested messages.
418485
func (c *Checker) checkNestedMessages(newMsg, oldMsg protoreflect.MessageDescriptor, result *compatibility.Result) {
419486
oldNested := make(map[string]protoreflect.MessageDescriptor)

0 commit comments

Comments
 (0)