docs: add Javadoc to datastore core interfaces - #623
Conversation
Documents the six interfaces in the datastore package that carried an @author-only Javadoc block, and removes the @author tags per CONTRIBUTING. IResourceStore, IResourceStorage and IResourceFilter now state the versioned CRUD contract explicitly: id plus integer version, update returning a new version rather than mutating in place, delete marking a version while leaving it readable through readIncludingDeleted, and deleteAllPermanently as the only history-removing operation. The storage/store split and the shared query-limit ceiling are named where a caller would look for them. IDescriptorStore, IDocumentBuilder and IJsonSerialization describe what consumes them: descriptors back the UI's configuration listing without deserializing each document, the document builder is the mapping seam between storage and the models, and the serialization wrapper exists so mapper settings are declared once. IResourceStorageFactory already had prose Javadoc; only its @author tag is removed. Docs only, no behavior change. Fixes labsai#546
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExpanded class-level Javadocs across datastore resource and serialization interfaces, documenting filtering, persistence, mutation annotations, paging, document mapping, and JSON serialization. Existing author tags were removed; no declarations or behavior changed. ChangesDatastore Contracts
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/datastore/IResourceFilter.java`:
- Around line 13-16: Update the Javadoc in IResourceFilter to explicitly state
that groups of QueryFilters are combined with AND after each group applies its
own ConnectingType; clarify that group-level connectors do not control the outer
combination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fba3a6f-7fdf-410d-98e1-4e1a23e61073
📒 Files selected for processing (6)
src/main/java/ai/labs/eddi/datastore/IResourceFilter.javasrc/main/java/ai/labs/eddi/datastore/IResourceStorage.javasrc/main/java/ai/labs/eddi/datastore/IResourceStore.javasrc/main/java/ai/labs/eddi/datastore/serialization/IDescriptorStore.javasrc/main/java/ai/labs/eddi/datastore/serialization/IDocumentBuilder.javasrc/main/java/ai/labs/eddi/datastore/serialization/IJsonSerialization.java
Review feedback on labsai#623. The previous wording said the groups "are then combined" without saying how, which invites the reading that a group's ConnectingType also governs the outer combination. ResourceFilter.createQuery applies each group's ConnectingType via Filters.and/Filters.or, then wraps the resulting group expressions in Filters.and, so the outer join is always AND regardless of what the individual groups use.
|
Good catch, fixed in the latest push. I checked it against Worded it slightly more explicitly than the suggestion, since the misreading worth preventing is that a group's connector might govern the outer combination:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #546 by replacing @author-only class Javadocs with descriptive documentation across the datastore core abstraction interfaces, clarifying the responsibilities and contracts of the versioned CRUD store, storage backends, filtering/query surfaces, descriptor browsing, document mapping, and shared JSON serialization.
Changes:
- Added/expanded class-level Javadoc for the seven datastore core interfaces called out in #546, removing
@author-only blocks. - Documented key behavioral contracts (versioning, deletion/history semantics, paging/limit clamping, query filter grouping semantics) directly on the interfaces.
- Kept
IResourceStorageFactory’s existing prose and removed its@authortag.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/datastore/IResourceStore.java | Documents the versioned CRUD contract and nested contract types/exceptions. |
| src/main/java/ai/labs/eddi/datastore/IResourceStorage.java | Documents backend storage responsibilities and shared limit clamping. |
| src/main/java/ai/labs/eddi/datastore/IResourceFilter.java | Documents filter-group semantics and paging behavior for list endpoints. |
| src/main/java/ai/labs/eddi/datastore/IResourceStorageFactory.java | Preserves existing factory documentation while removing @author. |
| src/main/java/ai/labs/eddi/datastore/serialization/IDescriptorStore.java | Documents descriptor browsing/paging semantics and limit conventions. |
| src/main/java/ai/labs/eddi/datastore/serialization/IDocumentBuilder.java | Documents document↔model mapping responsibilities used by persistence. |
| src/main/java/ai/labs/eddi/datastore/serialization/IJsonSerialization.java | Documents the shared JSON serialization/deserialization wrapper contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Nested here because they belong to the contract rather than any one | ||
| * implementation: {@link IResourceId} (the id/version pair), the checked | ||
| * exceptions callers are expected to handle, and {@link ConfigurationUpdate}, | ||
| * the interceptor binding that fires when a store mutates a configuration. |
|
The Secret Scanning failure on this PR isn't from the diff — it's the fork-secrets boundary, and it will hit every external contributor. The job fails before scanning anything:
Evidence across the last 14 PRs:
Only fork PR, only failure. Everything else here is green — Build & Test, CodeQL, Trivy, Integration Tests, Preflight. It matters more than one red check, because Three options, all maintainer-side:
Happy to send option 1 as its own PR if you'd like it, though it's a CI change rather than docs so I didn't want to bundle it in here. |
No @interceptor implements the binding, so the annotation has no runtime behaviour today. Say that it marks the mutating store methods and that callers invalidate their own caches, instead of implying something fires.
|
Good catch, fixed in 4b1239f. I checked before changing it rather than taking the review at face value: no class in New wording: * exceptions callers are expected to handle, and {@link ConfigurationUpdate},
* which marks the store methods that mutate a stored configuration. It is
* declared as an {@link InterceptorBinding}, but nothing implements that
* interceptor today, so the annotation has no runtime behaviour: it documents
* intent, and callers that need a cache refreshed invalidate it themselves.I kept the "declared as an Unrelated to this PR, but the same claim exists elsewhere:
|
|
Thanks for the contribution, looks good to me! |
Fixes #546
Documents the seven interfaces listed in the issue. Six carried an
@author-only Javadoc block — the case the issue calls out as needing replacement — andIResourceStorageFactoryalready had prose, so only its@authortag is removed.Each one was written from the interface's own method set plus its implementation (
ResourceFilter,PostgresResourceStorage,HistorizedResourceStore/AbstractResourceStore,DescriptorStore,DocumentBuilder,JsonSerialization), following the house style already set byIResourceStorageFactory.IResourceStoreupdatereturns a new version rather than mutating,deletemarks a version while leaving it readable viareadIncludingDeleted,deleteAllPermanentlyis the only history-removing call. Also whyIResourceId, the checked exceptions andConfigurationUpdateare nested in the contract.IResourceStorageIResourceStorelayers on top, obtained through the factory rather than injected, and whyMAX_RESULT_LIMIT/resolveLimitlive here so backends can't drift.IResourceFilterQueryFiltersgroups combine viaConnectingType, index/limit paging, and that it backs the REST list endpoints.IDescriptorStoreNO_LIMIT/DEFAULT_LIMITexist.IDocumentBuilderIJsonSerializationVerification
Every
{@link}target was checked to resolve (members against the declaring interface, types against the source tree) — 20 references, none dangling.Docs only, no behavior change, no
@authortags left in the seven files.One note on the sibling issues
While scoping this I checked #544 and #545 against
mainas well. Their state is mixed rather than open or done:@author-only;IMcpCallsStorealready has proseIDeploymentStore,IDocumentDescriptorStore,IPropertySetterStore); the other five are already documentedWorth trimming those file lists so the next contributor doesn't rewrite documentation that already exists — that appears to be what happened on #547, where the interfaces turned out to be done. Happy to take either of them next if useful.
Summary by CodeRabbit