Give every Firestore DTO a no-arg constructor (Track C blocker) - #24
Merged
Conversation
Firestore's object mapper instantiates targets reflectively via toObject()/ toObjects(), which requires a public no-arg constructor. Kotlin only emits one for a data class when EVERY primary-constructor parameter has a default. All 15 *Dto types in IBackendService.kt had at least one parameter without a default, so none was constructible: every Firestore read threw and was swallowed into Result.Error by FirebaseBackendService's firestoreCall wrapper, while writes succeeded. Flipping USE_MOCK_BACKEND=false therefore produced a write-only app - reads all failed silently. This is Track C item 1 because nothing else in Track C is observable against a real Firestore until it is fixed. Adds a type-appropriate default to the 114 parameters that lacked one, across all 15 *Dto types. The 10 *Request types are deliberately unchanged: they are only ever written, never deserialized, so required parameters there remain a real safety net. All existing construction sites use named arguments, so none is affected. Adds FirestoreDtoContractTest, which encodes the contract without needing Firebase: it reflectively asserts every DTO has a public no-arg constructor, that invoking it actually succeeds, and that the registered list stays in sync with the source. These fail the moment a default is removed - the exact change that would break reads again. Known tradeoff, deliberately accepted: these DTOs are also @serializable, and SyncManager decodes BookingDto/ServiceDto/ClientDto from queued payloads. kotlinx previously threw MissingFieldException on a payload missing a required field, which surfaced as a FAILED op; it will now fall back to the default instead. That trades strictness for deserializability. It has no live impact today (queueOperation still has zero production callers), but if the queue is ever wired, mapper-level validation should replace the strictness this gives up.
There was a problem hiding this comment.
Pull request overview
Enables successful Firestore deserialization against the real backend by ensuring every Firestore-read DTO is reflectively constructible (public no-arg constructor), and adds a JVM-only contract test to prevent regressions.
Changes:
- Added type-appropriate default values to all primary-constructor parameters across the 15
*Dtotypes inIBackendService.ktso Kotlin emits a public no-arg constructor. - Added
FirestoreDtoContractTestto assert the no-arg constructor exists, is public, and is invokable via Java reflection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
data/src/main/java/com/example/diamonds/data/remote/backend/IBackendService.kt |
Makes all Firestore DTOs default-constructible by providing defaults for previously-required constructor parameters. |
data/src/test/java/com/example/diamonds/data/remote/backend/FirestoreDtoContractTest.kt |
Adds reflection-based tests intended to guard the Firestore DTO no-arg-constructor contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `the registered DTO list is complete` test asserted firestoreReadDtos.size == 15, which only compared the list against a hardcoded number. Adding a 16th *Dto to IBackendService.kt without registering it would have left the test green, despite its message claiming it would catch exactly that — a test that could not fail for the reason it advertised. It now parses IBackendService.kt for `data class \w+Dto(` declarations and asserts the declared set and the registered set match in both directions, so an unregistered DTO fails and a stale registration fails too. Verified against the current source (15 declared, 15 registered) and against a simulated unregistered DTO, which fails as intended. The source file is located relative to the module dir with repo-root fallbacks, and errors with the working directory if it cannot be found. Also rephrased the "no no-arg constructor" failure message, which read as a double negative. Both raised in Copilot review on PR #24.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
data/src/test/java/com/example/diamonds/data/remote/backend/FirestoreDtoContractTest.kt:125
- DTO_DECLARATION is currently anchored to
^data class ...(, so it will miss valid Kotlin declarations that have leading whitespace or a visibility modifier (e.g.,internal data class FooDto(). In that case a new DTO could be added without being detected, and this guard would silently pass.
private companion object {
val DTO_DECLARATION = Regex("""^data class (\w+Dto)\(""", RegexOption.MULTILINE)
}
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Track C item 1 — the blocker that gates the rest of Track C.
The bug
Firestore's object mapper instantiates targets reflectively via
toObject()/toObjects(), which requires a public no-arg constructor. Kotlin only emits one for adata classwhen every primary-constructor parameter has a default.All 15
*Dtotypes inIBackendService.kthad at least one parameter without a default, so none was constructible.FirebaseBackendServicecallstoObject/toObjects26 times, and itsfirestoreCallwrapper swallows the throw intoResult.Error. Writes succeeded; reads all failed silently.Net effect: flipping
USE_MOCK_BACKEND=falseproduced a write-only app. This is item 1 of Track C because nothing else in Track C — the 14 stubs, security rules, profile-doc creation — is even observable against a real Firestore until reads work.The fix
A type-appropriate default on the 114 parameters that lacked one, across all 15
*Dtotypes ("",0,0.0,0f,false,emptyList(),nullfor nullables).The 10
*Requesttypes are deliberately unchanged — they're only ever written, never deserialized, so required parameters there remain a genuine safety net. Verified the diff touches only*Dtoclasses: all 114 changed lines map to the 15 DTOs, zero to*Request.All existing construction sites use named arguments, so none is affected.
Regression guard
FirestoreDtoContractTestencodes the contract without needing Firebase:newInstance(), not justgetDeclaredConstructor())These fail the moment someone removes a default — precisely the change that would silently break reads again.
Known tradeoff — stated rather than buried
These DTOs are also
@Serializable, andSyncManagerdecodesBookingDto/ServiceDto/ClientDtofrom queued payloads (SyncManager.kt:250,292,305). kotlinx previously threwMissingFieldExceptionwhen a payload was missing a required field, surfacing as aFAILEDop; it will now fall back to the default instead.That trades strictness for deserializability, and it's the same fail-loudly-vs-silently concern #19–#21 were about, so I won't pretend it isn't there. It has no live impact today —
queueOperationstill has zero production callers — but if the queue is ever wired, mapper-level validation should replace the strictness this gives up.The alternative (keep DTOs strict, hand-write 15
DocumentSnapshotmappers) avoids that but adds ~150 lines of boilerplate and its own bug surface.TECH_LEAD_REVIEW.md§9.4 lists the defaults approach as the recommended option; happy to revisit if you'd prefer the mappers.Verification
No Android SDK locally, so CI is the check. Verified by inspection and scripted analysis: 15/15 DTOs now have all-default parameters, 0
*Requestclasses touched, parens/braces balanced, no mangled call sites, and every DTO named in the test cross-checked against the source declarations (15 declared, 15 referenced, 0 missing, 0 bogus).🤖 Generated with Claude Code
Generated by Claude Code